GVN-hoist: fix early exit logic

[sebpop]

Summary

The patch splits a complex && if condition into easier to read and understand logic.
That wrong early exit condition was letting some instructions with not all operands available pass through when HoistingGeps was true.

The patch also adds an extra early exit for TerminatorInst that should not be added to the analysis in the first place: these are branch instructions, invoke, etc.
I found it strange that we were looking at this kind of instructions when running gdb on the testcase.

I'm testing this patch with 3stage bootstrap and test-suite.
I will commit after it passes, and reenable the pass by default.

[• dberlin]

lib/Transforms/Scalar/GVNHoist.cpp

897 ↗

(On Diff #66845)

I hope this is not a correctness fix :)

[sebpop]

lib/Transforms/Scalar/GVNHoist.cpp

897 ↗

(On Diff #66845)

Nop. This is a compilation time fix: as I was saying in the commit message, we should not try to analyze to hoist this kind of "scalar instructions".

[sebpop]

When I tested the 3stage bootstrap was broken without my patch.
Patch passed test-suite and check-all on x86_64-linux.

llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp

for (const HoistingPointInfo &HP : HPL) {
         assert(allOperandsAvailable(Repl, HoistPt) &&
                "instruction depends on operands that are not available");
       } else {
         // When we do not find Repl in HoistPt, select the first in the list
         // and move it to HoistPt.
         Repl = InstructionsToHoist.front();
 
         // We can move Repl in HoistPt only when all operands are available.
-        // When not HoistingGeps we need to copy the GEPs now.
         // The order in which hoistings are done may influence the availability
         // of operands.
-        if (!allOperandsAvailable(Repl, HoistPt) && !HoistingGeps &&
-            !makeGepOperandsAvailable(Repl, HoistPt, InstructionsToHoist))
+        if (!allOperandsAvailable(Repl, HoistPt)) {
+
+          // When HoistingGeps there is nothing more we can do to make the
+          // operands available: just continue.
+          if (HoistingGeps)
+            continue;
+
+          // When not HoistingGeps we need to copy the GEPs.
+          if (!makeGepOperandsAvailable(Repl, HoistPt, InstructionsToHoist))
             continue;
+        }
 
         // Move the instruction at the end of HoistPt.
         Instruction *Last = HoistPt->getTerminator();
         Repl->moveBefore(Last);
 
         DFSNumber[Repl] = DFSNumber[Last]++;
       }
 

llvm/trunk/test/Transforms/GVN/hoist-call.ll

+; RUN: opt -S -gvn-hoist < %s | FileCheck %s
+
+; Check that the call and fcmp are hoisted.
+; CHECK-LABEL: define void @fun(
+; CHECK: call float
+; CHECK: fcmp oeq
+; CHECK-NOT: call float
+; CHECK-NOT: fcmp oeq
+
+define void @fun(float %__b) minsize {
+entry:
+  br label %if.then
+
+if.then:                                          ; preds = %entry
+  br i1 undef, label %if.then8, label %lor.lhs.false
+
+lor.lhs.false:                                    ; preds = %if.then
+  %0 = call float @llvm.fabs.f32(float %__b) #2
+  %cmpinf7 = fcmp oeq float %0, 0x7FF0000000000000
+  unreachable
+
+if.then8:                                         ; preds = %if.then
+  %1 = call float @llvm.fabs.f32(float %__b) #2
+  %cmpinf10 = fcmp oeq float %1, 0x7FF0000000000000
+  ret void
+}
+
+declare float @llvm.fabs.f32(float)

llvm/trunk/test/Transforms/GVN/hoist-recursive-geps.ll

; <label>:36:                                     ; preds = %4
   store double %50, double* %12, align 8
   br label %51
 
 ; <label>:51:                                     ; preds = %36, %21
   %52 = load double, double* %12, align 8
   %53 = load double, double* %9, align 8
   %54 = fcmp olt double %52, %53
   ret i1 %54
-}
+}
 

llvm/trunk/test/Transforms/GVN/hoist.ll

 
 if.end:
   %r = phi float [ %g, %if.then ], [ %k, %if.else ]
   ret float %r
 }
 
 ; Check that all independent expressions are hoisted.
 ; CHECK-LABEL: @independentScalarsHoisting
-; CHECK: fadd
 ; CHECK: fsub
 ; CHECK: fdiv
 ; CHECK: fmul
+; CHECK: fadd
 ; CHECK-NOT: fsub
 ; CHECK-NOT: fdiv
 ; CHECK-NOT: fmul
 define float @independentScalarsHoisting(float %a, float %b, i1 %c) {
 entry:
   br i1 %c, label %if.then, label %if.else
 
 if.then: