-
Notifications
You must be signed in to change notification settings - Fork 100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Global "NaN" Is Lowered to 0.0
During LLVM Conversion
#559
Comments
Seems to be more fundamental:
So the parsesr seems to throw the NaN away. I will add a LIT test in IR/globals.cir and fix the issue 😇 |
Thanks @orbiri :D |
While trying to understand the root cause for this issue, I came to the realization that we are reimplementing the builtin dialect in CIR. I understand the merit of not relying on upstream dialects in the core of CIR (as long as it is downstream) as @lanza explained in #62.
I believe that the merit for using the builtin dialect is to avoid bugs exactly like this one which comes from the maintenance and potholes that have been tripped by the many users of MLIR in the last 4 years. —— |
I couldn't find a way to avoid re-implementing the logic of the builtin float attribute parser in
Adding more rigorous tests to cover all edge cases and will post PR by (or over) the weekend. |
Builtin dialects are great, but it's easier overall to handle ours, which should only care about C/C++ (with its own specific semantics and target variations), and lower to these builtin types whenever it makes sense for a specific lowering story. |
Depending on how big that is, probably a non-starter.
I'm not sure I grasped what's going on here entirely, perhaps it will be clear with the PR up. Anyways, it's also possible we could have a specific attribute to represent NaNs just like we have |
I will not bore you with the many different methods that I tried until I finally got to the only solution that worked (validated with multitudes of tests). I tried to confine myself to the MLIR tools available, but they are unfortunately lacking as it seems very difficult to extend the AsmParser implementation downstream. Therefore the only solution I found was to extend the API surface of the AsmParser upstream by creating a small refactor of I will upload draft PR here soon while I work on the MLIR upstream PR (requires adding an attribute to the Test dialect which mimics cir::FPAttr parsing behavior to test the new parsing API) |
Uploaded "draft" PR to #572 . The CIR commit is ready-for-review and it would be appreciated :) |
Thanks for taking the time to improve things in MLIR upstream so we can use it, awesome! |
While skimming through failing SingleSource files, I randomly picked
GCC-C-execute-ieee-fp-cmp-3
. While bisecting the issue with it, I came across a cute bug with an extremely slow reproducer:cir-opt --cir-to-llvm
=>I hope to address this issue myself in the coming days :)
The text was updated successfully, but these errors were encountered: