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

Mon Sep 16 09:11:21 PDT 2019

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

In D61675#1671277 <https://reviews.llvm.org/D61675#1671277>, @cameron.mcinally wrote:

> 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.

Yeah, that sounds like we may get pushback from users depending on hardware target.

> 
> 
> 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.

I think we would sacrifice target-specific / implementation-defined behavior if it meant code could be optimized better, but we'd want some evidence of that improvement before making a change.

> 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...

Ok. I have no objections/comments to this patch then. As long as clang is consistent in creating the unary fneg, the CSE concern is probably moot. LGTM, but give other reviewers another chance (1-2 days?) to comment before committing.

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

https://reviews.llvm.org/D61675