[PATCH] D61675: [WIP] Update IRBuilder::CreateFNeg(...) to return a UnaryOperator

Mon Sep 16 06:49:12 PDT 2019

cameron.mcinally added a comment.

In D61675#1670603 <https://reviews.llvm.org/D61675#1670603>, @spatel wrote:

> On 2nd thought, that doesn't really make sense for CSE. The real question is whether we should canonicalize the binary fneg to unary fneg in instcombine. And should that happen before/after/concurrent with this change to clang?

Tough question...

1. The biggest problem I see with canonicalization of binary FNeg to unary FNeg is if DAZ/FTZ are set. With those set, a binary FNeg could be used to zero insignificant data. Otherwise, with a unary FNeg, the sign bit would just be flipped, regardless if it's a denormal or not. We have a weather code that uses an explicit binary FNeg to sanitize their noisy input, so any canonicalization there would disturb the results.

Currently, if I'm not mistaken, Clang only enables DAZ/FTZ with -Ofast and -ffast-math, so no problem there. But, there's nothing stopping a user from compiling with -O0 and flipping the DAZ/FTZ bits themselves.

Conversely, if would be easy to argue that setting DAZ/FTZ breaks IEEE-754 compliance, so it doesn't matter what we do from that point on. Although, this seems like a weird grey-area to me.

2. Another concern is if LLVM intends to respect the hardware-specified sign of a NaN or not. That is, IEEE-754 does not specify the sign on a NaN result, but the hardware may. E.g. x86 always returns the a -NaN, called `QNaN floating-point indefinite`.

It would be easy to argue that IEEE-754 doesn't specify the sign, so the compiler can do whatever it wants. IMO, I think we can do better and should allow the hardware to choose the NaN it wants.

3. @arsenm also had a concern about source modifiers on AMD GPUs. I don't know much about those, so will leave that discussion to him...

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61675/new/

https://reviews.llvm.org/D61675