[Mlir-commits] [mlir] [MLIR][Arith] Add denormal attribute to binary/unary operations (PR #112700)

[Renato Golin]

rengolin wrote:

> My comment is that without being able to clearly map this change to the "charter" for arith, we can't just land this as-is without proper RFC/discussion

Unfortunately, there is no charter for arith. Once we get the governance ready (I started writing the document, will share with you once it reached the draft state), we will get to it. Right now, I don't think anyone can claim they know what the charter is.

> The fact that you missed this discussion is a good indication that this didn't get enough visibility.

That's a stretch. Especially because...

> the PR has been open for nearly two months and discussed multiple times offline with Jakub and others, including F2F at LLVM Dev. The proposal incorporated the feedback provided so we felt confident enough to proceed.

This is a perfectly valid course for LLVM, a bit on the cautious side. I cannot claim "because I didn't see, it wasn't properly discussed" with this history. Reverting now would warrant a stronger reason than "I didn't see" or "I wasn't convinced".

That's why we so desperately need a charter!

> This is applicable to any hardware and even includes denormal modes which are not supported by our hardware.

Agree, and we gave a few examples before it was approved and merged.

> Denormals (unfortunately!) are a fundamental part of the FP model, and the lack of representation is a gap in our ecosystem which is impacting everyone.

Yup.

> It seems more reasonable to me to learn from LLVM and create something that better aligns with today's hardware requirements, which is what we were aiming for.

+1 to this point. I'd hate to repeat the same mistakes. My patch referenced by Andrzej was a blunt instrument to something that should have been better planned from the start, but we didn't know better. Now we do.

> Consequently, it should be low risk to classify the denormal mode as something experimental while investigating/fixing any potential issues.

Exactly. No one that doesn't know what they're doing would ever pick those attributes, and the compiler should never add them without really knowing what it's doing. 

I also don't see the current implementation as problematic. Maybe naive, but not wrong or in need of revert.

https://github.com/llvm/llvm-project/pull/112700