[Transforms] Resolve FIXME: No Lowering of FNeg to FMul unless it is safe

[AtariDreams]

Or association flags are specified.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: AtariDreams (AtariDreams)

Changes

Or association flags are specified.

---

Full diff: https://github.com/llvm/llvm-project/pull/85252.diff

1 Files Affected:

• (modified) llvm/lib/Transforms/Scalar/Reassociate.cpp (+9-3)

diff --git a/llvm/lib/Transforms/Scalar/Reassociate.cpp b/llvm/lib/Transforms/Scalar/Reassociate.cpp
index d91320863e241d..f44608624cf3df 100644
--- a/llvm/lib/Transforms/Scalar/Reassociate.cpp
+++ b/llvm/lib/Transforms/Scalar/Reassociate.cpp
@@ -287,11 +287,17 @@ static Instruction *CreateNeg(Value *S1, const Twine &Name,
 static BinaryOperator *LowerNegateToMultiply(Instruction *Neg) {
   assert((isa<UnaryOperator>(Neg) || isa<BinaryOperator>(Neg)) &&
          "Expected a Negate!");
-  // FIXME: It's not safe to lower a unary FNeg into a FMul by -1.0.
   unsigned OpNo = isa<BinaryOperator>(Neg) ? 1 : 0;
   Type *Ty = Neg->getType();
-  Constant *NegOne = Ty->isIntOrIntVectorTy() ?
-    ConstantInt::getAllOnesValue(Ty) : ConstantFP::get(Ty, -1.0);
+
+  // Only lower to FMul if the operation is not a unary FNeg or Neg has the
+  // correct flags.
+  if (Ty->isFloatingPointTy() && isa<UnaryOperator>(Neg) &&
+      !hasFPAssociativeFlags(Neg))
+    return nullptr;
+
+  Constant *NegOne = Ty->isIntOrIntVectorTy() ? ConstantInt::getAllOnesValue(Ty)
+                                              : ConstantFP::get(Ty, -1.0);
 
   BinaryOperator *Res =
       CreateMul(Neg->getOperand(OpNo), NegOne, "", Neg->getIterator(), Neg);

[dtcxzyw]

Could you please provide the proof? I cannot prove the correctness with alive2.

[dtcxzyw]

https://alive2.llvm.org/ce/z/Ki3Rf2

nnan is all you need :)

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[arsenm]

https://alive2.llvm.org/ce/z/Ki3Rf2

nnan is all you need :)

In this context, it's not simply doing this transform in isolation. It's being consumed by another canonicalizing operation. The problem is this is introducing a canonicalization, which doesn't matter if it's only used by operations that canonicalize the inputs. nnan would not be sufficient to introduce this, since operations may also do other bit pattern changing side effects (like denormal flush). Since this is consumed by a canonicalizing operation, it's either correct to always do (in the has one use case) (or it's wrong to ever do).

[arsenm]

Better to directly check for fneg? isa that's floating point type isn't really obvious

[arsenm]

Don't need the full set of fast flags. Also don't need it on every instruction. Should target testing missing specific flags contexts

[arsenm]

Also add test where it's know not-nan from the source

[arsenm]

which doesn't matter if it's only used by operations that canonicalize the inputs.

I guess we technically don't guarantee canonicalization on FP operations, but I think we can assert that it may happen