Skip to content
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

Open
orbiri opened this issue Apr 22, 2024 · 9 comments · May be fixed by #572
Open

Global "NaN" Is Lowered to 0.0 During LLVM Conversion #559

orbiri opened this issue Apr 22, 2024 · 9 comments · May be fixed by #572

Comments

@orbiri
Copy link
Contributor

orbiri commented Apr 22, 2024

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:

module @m attributes {cir.lang = #cir.lang<c>, llvm.data_layout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"} {
  cir.global external @fnan = #cir.fp<0x7FC00000> : !cir.float
}

cir-opt --cir-to-llvm =>

module @m attributes {llvm.data_layout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"} {
  llvm.mlir.global external @fnan(0.000000e+00 : f32) {addr_space = 0 : i32} : f32
}

I hope to address this issue myself in the coming days :)

@orbiri
Copy link
Contributor Author

orbiri commented Apr 22, 2024

Seems to be more fundamental:

$ Debug/bin/cir-opt /tmp/test.cir        
module @m attributes {cir.lang = #cir.lang<c>} {
  cir.global external @fnan = #cir.fp<0.000000e+00> : !cir.float
}

So the parsesr seems to throw the NaN away. I will add a LIT test in IR/globals.cir and fix the issue 😇

@bcardosolopes
Copy link
Member

Thanks @orbiri :D

@orbiri
Copy link
Contributor Author

orbiri commented Apr 22, 2024

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.
Still, it is harder for me to understand that decision w.r.t specifically the builtin dialect which is (builtin dialect docs)

The builtin dialect contains a core set of Attributes, Operations, and Types that have wide applicability across a very large number of domains and abstractions. Many of the components of this dialect are also instrumental in the implementation of the core IR

Given the far-reaching nature of this dialect and the fact that MLIR is extensible by design, any potential additions are heavily scrutinized.
Moreover, now that we are on the way to upstream CIR, it will be almost impossible to commit a breaking change in the builtin dialect which will break CIR!

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 still need to play around with it, but I might experiment with replacing the floating point attribute (not the type) in CIR with the builtin one and see how it looks like :)

@orbiri
Copy link
Contributor Author

orbiri commented Apr 24, 2024

I couldn't find a way to avoid re-implementing the logic of the builtin float attribute parser in CIRAttr.cpp :/
The intermediate result (no pun intended) of the roundtrip tests looks like this:

// Note - type info is attached only to bitcasted integer literals, following the guidelines of the builtin FloatAttr
// CHECK: cir.global external @fhalf = #cir.fp<5.000000e-01> : !cir.float
// CHECK: cir.global external @fnan = #cir.fp<0x7FC00000 : !cir.float> : !cir.float
// CHECK: cir.global external @dnan = #cir.fp<0x7FF8000000000000 : !cir.double> : !cir.double

Adding more rigorous tests to cover all edge cases and will post PR by (or over) the weekend.

@bcardosolopes
Copy link
Member

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.

@bcardosolopes
Copy link
Member

I couldn't find a way to avoid re-implementing the logic of the builtin float attribute parser in CIRAttr.cpp

Depending on how big that is, probably a non-starter.

So the parsesr seems to throw the NaN away

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 #cir.zero for zeros.

@orbiri
Copy link
Contributor Author

orbiri commented Apr 28, 2024

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 AsmParserImpl::parseFloat to support return of arbitrary APFloat result as well as the original double support. On the way, I also extended upstream MLIR support for parsing f80/f128 literals that must be printed in hex format, tested upstream. Without this extended parsing support, testing long double lowering to LLVM would be possible only "in memory" and not through any pipeline which prints and parses.

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)

@orbiri orbiri linked a pull request Apr 28, 2024 that will close this issue
@orbiri
Copy link
Contributor Author

orbiri commented Apr 28, 2024

Uploaded "draft" PR to #572 . The CIR commit is ready-for-review and it would be appreciated :)
The MLIR commit is lacking the Test Dialect addition as mentioned above.

@bcardosolopes
Copy link
Member

Thanks for taking the time to improve things in MLIR upstream so we can use it, awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants