[llvm] Update foldFMulReassoc to respect absent fast-math flags (PR #88589)

Andy Kaylor via llvm-commits llvm-commits at lists.llvm.org
Tue Apr 16 13:41:37 PDT 2024


================
@@ -636,26 +636,43 @@ Instruction *InstCombinerImpl::foldFMulReassoc(BinaryOperator &I) {
   // expression.
   if (match(Op1, m_Constant(C)) && C->isFiniteNonZeroFP()) {
     Constant *C1;
-    if (match(Op0, m_OneUse(m_FDiv(m_Constant(C1), m_Value(X))))) {
+    if (match(Op0,
+              m_AllowReassoc(m_OneUse(m_FDiv(m_Constant(C1), m_Value(X)))))) {
       // (C1 / X) * C --> (C * C1) / X
       Constant *CC1 =
           ConstantFoldBinaryOpOperands(Instruction::FMul, C, C1, DL);
-      if (CC1 && CC1->isNormalFP())
-        return BinaryOperator::CreateFDivFMF(CC1, X, &I);
+      if (CC1 && CC1->isNormalFP()) {
+        // Preserve only fast-math flags that were set on both of the original
+        // instructions
+        auto *NewDiv = BinaryOperator::CreateFDivFMF(CC1, X, &I);
+        NewDiv->andIRFlags(Op0);
----------------
andykaylor wrote:

I refactored the handling a bit to use a `FastMathFlagGuard` on the Builder object and added new overloads of `BinaryOperator::Create*FMF()` to take the flags directly. I thought about possibly introducing a new class that would provide a façade for `IRBuilder` and the `BinaryOperator::Create*FMF()` methods to combine the FMF handling into a single place. That felt like overkill, but maybe this pattern occurs often enough to justify it.

The thing I didn't like here is the fact that I needed to mix instructions created with `IRBuilder` (because intermediate instructions needed to be inserted in the IR) and instructions created directly with `BinaryOperator`'s static methods (because `InstCombine` expects me to return an instruction that hasn't been inserted. This feels kind of clunky.

It also bothers me a bit that `IRBuilder::CreateFDivFMF()` and similar don't follow the insertion rules for the rest of the `IRBuilder` routine. I think that the insert point will already be set to the instruction I am replacing in the code I modified, but if I wanted to replace code that was using `IRBuilder::CreateFDivFMF()` to insert instructions somewhere else, I'd need to manually change the insert point to use `FastMathFlagGuard` the way I did.

Here's what I was thinking for the wrapper to combine FMF handling between `IRBuilder` and `BinaryOperator` calls:

```
class FMFBuilder {
public:
  FMFHelper(IRBuilder B, FastMathFlags F) : Builder(B), FMFGuard(B), FMF(F) {
    Builder.setFastMathFlags(FMF);
  }

  // Functions that wrap IRBuilder calls get the fast math flags from IRBuilder, as set by this wrapper class
  Value *createFDiv(Value *L, Value *R, const Twine &Name="", MDNode *FPMD=nullptr) {
    return Builder.CreateFDiv(L, R, Name, FPMD);
  }

  // Functions that wrap BinaryOperator static calls need the FMF we have stored
  // The "Inst" suffix indicate that we want to return an instruction
  BinaryOperator *CreateFDivInst(Value *V1, Value *V2, const Twine &Name = "") {
    return BinaryOperator::CreateFDivFMF(V1, V2, FMF, Name);
  }

private:
  IRBuilder &Builder;
  FastMathFlagGuard FMFGuard;
  FastMathFlags FMF;
}

```

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


More information about the llvm-commits mailing list