Global "NaN" Is Lowered to `0.0` During LLVM Conversion about clangir HOT 9 OPEN

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 😇

from clangir.

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)

from clangir.

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.

from clangir.

Thanks @orbiri :D

from clangir.

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 :)

from clangir.

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.

from clangir.

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.

from clangir.

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.

from clangir.

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

from clangir.