[FPEnv Core 12/14] Prevent InstCombine from hanging

[majnemer]

test/Other/fpenv-constant-fold.ll

19

You need to check the operands here as well.

[sdmitrouk]

Flags rename and checks of arguments.

[sdmitrouk]

Ping.

[mehdi_amini]

I wonder why this would still be needed now that ConstExprs have FMF?

[sdmitrouk]

Right, looks like it's not necessary anymore, thanks.

[mehdi_amini]

Can we close this then?

lib/Transforms/InstCombine/InstCombineAddSub.cpp

if (isa<PHINode>(LHS))
       if (Instruction *NV = FoldOpIntoPhi(I))
         return NV;
 
     if (SelectInst *SI = dyn_cast<SelectInst>(LHS))
       if (Instruction *NV = FoldOpIntoSelect(I, SI))
         return NV;
   }
 
+  // Skip this to break implicit infinite loop with "fsub" transformations of
+  // the form: (a + b) => (a - (-b)) => (a + b) => etc
+  if (!I.hasKeepExceptions() && !I.hasKeepRounding()) {
     // -A + B  -->  B - A
     // -A + -B  -->  -(A + B)
     if (Value *LHSV = dyn_castFNegVal(LHS)) {
       Instruction *RI = BinaryOperator::CreateFSub(RHS, LHSV);
       RI->copyFastMathFlags(&I);
       return RI;
     }
 
     // A + -B  -->  A - B
     if (!isa<Constant>(RHS))
       if (Value *V = dyn_castFNegVal(RHS)) {
         Instruction *RI = BinaryOperator::CreateFSub(LHS, V);
         RI->copyFastMathFlags(&I);
         return RI;
       }
+  }
 
   // Check for (fadd double (sitofp x), y), see if we can merge this into an
   // integer add followed by a promotion.
   if (SIToFPInst *LHSConv = dyn_cast<SIToFPInst>(LHS)) {
     // (fadd double (sitofp x), fpcst) --> (sitofp (add int x, intcst))
     // ... if the constant fits in the integer value.  This is useful for things
     // like (double)(x & 1234) + 4.0 -> (double)((X & 1234)+4) which no longer
     // requires a constant pool load, and generally allows the add to be better

test/Other/fpenv-constant-fold.ll

-; InstCombine is used just for its calls to constant folder.
+; InstCombine is used just for its calls to constant folder, it is the cause of
+; fsub -> fadd transformation.
 ; RUN: opt -S -instcombine -o - < %s | FileCheck %s
 
 ; Target independent constant folder should not fold floating-point operations
 ; that have constant expression as their operands when FEnv access is requested
 ; and such folding leads to differences in observable effects at runtime.
 
 define double @do-not-fold-fadd-that-can-trap() {
 ; CHECK-LABEL: @do-not-fold-fadd-that-can-trap
 ; CHECK: fadd
 entry:
   %val = fadd kexc double 1.000000e+308, 1.000000e+308
   ret double %val
 }
 
 define double @do-not-fold-fsub-that-can-trap() {
 ; CHECK-LABEL: @do-not-fold-fsub-that-can-trap
-; CHECK: fsub
+; CHECK: fadd
 entry:
   %val = fsub kexc double 1.000000e-308, 1.000000e+308
   ret double %val
 }
 
 define double @do-not-fold-fmul-that-can-trap() {
 ; CHECK-LABEL: @do-not-fold-fmul-that-can-trap
 ; CHECK: fmul

 ; CHECK: fadd
 entry:
   %val = fadd kround double 0.101, 0.93
   ret double %val
 }
 
 define double @do-not-fold-fsub-because-of-rounding() {
 ; CHECK-LABEL: @do-not-fold-fsub-because-of-rounding
-; CHECK: fsub
+; CHECK: fadd
 entry:
   %val = fsub kround double 0.101, 0.93
   ret double %val
 }
 
 define double @do-not-fold-fmul-because-of-rounding() {
 ; CHECK-LABEL: @do-not-fold-fmul-because-of-rounding
 ; CHECK: fmul
entry:
   %val = fadd kexc kround double 1.0, 1.0
   ret double %val
 }
 
 define double @fold-fsub-that-cant-trap() {
 ; CHECK-LABEL: @fold-fsub-that-cant-trap
 ; CHECK-NOT: fsub
 entry:
-  %val = fsub kexc kround double 1.0, 1.0
+  %val = fadd kexc kround double 1.0, 1.0
   ret double %val
 }
 
 define double @fold-fmul-that-cant-trap() {
 ; CHECK-LABEL: @fold-fmul-that-cant-trap
 ; CHECK-NOT: fmul
 entry:
   %val = fmul kexc kround double 1.0, 1.0

test/Transforms/InstCombine/do-not-hand-on-fpops-with-fpenv-access.ll

+; RUN: opt < %s -instcombine -S
+
+; InstCombine shouldn't hang in non-terminating sequence of conversions like:
+;    (a + b) => (a - (-b)) => (a + b) => etc.
+
+define double @test1() {
+entry:
+  %add = fadd kexc double 0x3FF921FB54442D18, 0x3870000000000000
+  ret double %add
+}
+
+define double @test2(double %x) {
+entry:
+  %sub = fsub kexc kround double -0.000000e+00, 1.000000e+00
+  %div = fdiv kexc kround double %sub, %x
+  ret double %div
+}
+
+define double @test3() {
+entry:
+  %sub = fsub kexc kround double -0.000000e+00, 0x400921FB54442D18
+  ret double %sub
+}
+
+define double @test4(i32 %y) {
+entry:
+  %tobool = icmp ne i32 %y, 0
+  br i1 %tobool, label %cond.true, label %cond.false
+
+cond.true:
+  %sub = fsub kexc kround double -0.000000e+00, 0x400921FB54442D18
+  %div = fdiv kexc kround double %sub, 2.000000e+00
+  br label %cond.end
+
+cond.false:
+  %div1 = fdiv kexc kround double 0x400921FB54442D18, 2.000000e+00
+  br label %cond.end
+
+cond.end:
+  %cond = phi double [ %div, %cond.true ], [ %div1, %cond.false ]
+  ret double %cond
+}