GVN-hoist: disable hoisting stores (PR30216)

[sebpop]

Summary

This patch disables hoisting of stores until we implement a proper dependence
analysis, or we may just decide to always sink the stores. The number of stores
hoisted was low: on the test-suite we were only hoisting 15 stores.

[vkalintiris]

Hi Sebastian. I tried this on one of our BB that tests recursive builds and test/Transforms/GVN/pr28626.ll is failing. I'm getting the same failure on an X86_64 host.

-- Testing: 1 tests, 1 threads --                                                 
FAIL: LLVM :: Transforms/GVN/pr28626.ll (1 of 1)                                  
******************** TEST 'LLVM :: Transforms/GVN/pr28626.ll' FAILED *************
*******                                                                           
Script:                                                                           
--                                                                                
/home/vk/build/llvm/debug/./bin/opt -S -gvn-hoist < /home/vk/repos/llvm/test/Trans
forms/GVN/pr28626.ll | /home/vk/build/llvm/debug/./bin/FileCheck /home/vk/repos/ll
vm/test/Transforms/GVN/pr28626.ll                                                 
--                                                                                
Exit Code: 1                                                                      
                                                                                  
Command Output (stderr):                                                          
--                                                                                
<stdin>:28:2: error: CHECK-NOT: string occurred!                                  
 store i1 %c.0, i1* %rc5, align 4                                                 
 ^                                                                                
/home/vk/repos/llvm/test/Transforms/GVN/pr28626.ll:42:14: note: CHECK-NOT: pattern
 specified here                                                                   
; CHECK-NOT: store                                                                
             ^                                                                    
                                                                                  
--                                                                                
                                                                                  
********************                                                              
Testing Time: 0.08s                                                               
********************                                                              
Failing Tests (1):                                                                
    LLVM :: Transforms/GVN/pr28626.ll                                             
                                                                                  
  Unexpected Failures: 1

[sebpop]

A proper fix is in review: https://reviews.llvm.org/D24517

llvm/lib/Transforms/Scalar/GVNHoist.cpp

for (BasicBlock *BB : depth_first(&F.getEntryBlock())) {
           if (isRelevantForHoisting(&I1))
             II.insert(&I1, VN);
       }
     }
 
     HoistingPointList HPL;
     computeInsertionPoints(II.getVNTable(), HPL, InsKind::Scalar);
     computeInsertionPoints(LI.getVNTable(), HPL, InsKind::Load);
-    computeInsertionPoints(SI.getVNTable(), HPL, InsKind::Store);
     computeInsertionPoints(CI.getScalarVNTable(), HPL, InsKind::Scalar);
     computeInsertionPoints(CI.getLoadVNTable(), HPL, InsKind::Load);
+
+    // Hoisting stores is difficult: see PR30216; disabled until either we fix
+    // the dependence analysis, or we decide to only sink the stores.  In
+    // principle stores should be by default sinked, unless doing so would
+    // increase the live range of stored values.
+    if (0) {
+      computeInsertionPoints(SI.getVNTable(), HPL, InsKind::Store);
       computeInsertionPoints(CI.getStoreVNTable(), HPL, InsKind::Store);
+    }
     return hoist(HPL);
   }
 };
 
 class GVNHoistLegacyPass : public FunctionPass {
 public:
   static char ID;
 

llvm/test/Transforms/GVNHoist/hoist-md.ll

 if.else:                                          ; preds = %entry
   store i32 2, i32* %x, align 4, !tbaa !5
   br label %if.end
 
 if.end:                                           ; preds = %if.else, %if.then
   ret void
 }
 ; CHECK-LABEL: define void @test1(
-; CHECK: store i32 2, i32* %x, align 4
-; CHECK-NEXT: br i1 %b
+; Store hoisting is disabled.
 
 define void @test2(i1 %b, i32* %x) {
 entry:
   br i1 %b, label %if.then, label %if.else
 
 if.then:                                          ; preds = %entry
   %gep1 = getelementptr inbounds i32, i32* %x, i64 1
   store i32 2, i32* %gep1, align 4, !tbaa !1
   br label %if.end
 
 if.else:                                          ; preds = %entry
   %gep2 = getelementptr inbounds i32, i32* %x, i64 1
   store i32 2, i32* %gep2, align 4, !tbaa !5
   br label %if.end
 
 if.end:                                           ; preds = %if.else, %if.then
   ret void
 }
 ; CHECK-LABEL: define void @test2(
-; CHECK: %[[gep:.*]] = getelementptr inbounds i32, i32* %x, i64 1
-; CHECK: store i32 2, i32* %[[gep]], align 4
-; CHECK-NEXT: br i1 %b
+; Store hoisting is disabled.
 
 define void @test3(i1 %b, i32* %x) {
 entry:
   br i1 %b, label %if.then, label %if.else
 
 if.then:                                          ; preds = %entry
   %gep1 = getelementptr inbounds i32, i32* %x, i64 1
   store i32 2, i32* %gep1, align 4, !tbaa !1
   br label %if.end
 
 if.else:                                          ; preds = %entry
   %gep2 = getelementptr i32, i32* %x, i64 1
   store i32 2, i32* %gep2, align 4, !tbaa !5
   br label %if.end
 
 if.end:                                           ; preds = %if.else, %if.then
   ret void
 }
 ; CHECK-LABEL: define void @test3(
-; CHECK: %[[gep:.*]] = getelementptr i32, i32* %x, i64 1
-; CHECK: store i32 2, i32* %[[gep]], align 4
-; CHECK-NEXT: br i1 %b
+; Store hoisting is disabled.
 
 !1 = !{!2, !2, i64 0}
 !2 = !{!"int", !3, i64 0}
 !3 = !{!"omnipotent char", !4, i64 0}
 !4 = !{!"Simple C++ TBAA"}
 !5 = !{!6, !6, i64 0}
 !6 = !{!"_ZTS1e", !3, i64 0}
 

llvm/test/Transforms/GVNHoist/hoist-mssa.ll

 ; RUN: opt -S -gvn-hoist < %s | FileCheck %s
 
-; Check that store hoisting works: there should be only one store left.
+; Store hoisting is disabled.
 ; CHECK-LABEL: @getopt
 ; CHECK: store i32
-; CHECK-NOT: store i32
+; CHECK: store i32
 
 @optind = external global i32, align 4
 
 define void @getopt() {
 bb:
   br label %bb1
 
 bb1:                                              ; preds = %bb
bb12:                                             ; preds = %bb9, %bb5
   br label %bb13
 
 bb13:                                             ; preds = %bb12, %bb7, %bb2
   ret void
 }
 
 @GlobalVar = internal global float 1.000000e+00
 
-; Check that we hoist stores and remove the MSSA phi node.
+; Store hoisting is disabled.
 ; CHECK-LABEL: @hoistStoresUpdateMSSA
 ; CHECK: store float
-; CHECK-NOT: store float
+; CHECK: store float
 define float @hoistStoresUpdateMSSA(float %d) {
 entry:
   store float 0.000000e+00, float* @GlobalVar
   %cmp = fcmp oge float %d, 0.000000e+00
   br i1 %cmp, label %if.then, label %if.end
 
 if.then:
   store float 0.000000e+00, float* @GlobalVar
   br label %if.end
 
 if.end:
   %tmp = load float, float* @GlobalVar, align 4
   ret float %tmp
 }

llvm/test/Transforms/GVNHoist/hoist-pr28606.ll

 ; %argmem, which would break the inalloca contract.
 ;
 ; CHECK-LABEL: @test
 ; CHECK: alloca i8
 ; CHECK: stacksave
 ; CHECK: alloca inalloca
 ; CHECK-NOT: alloca i8
 
-; Check that store instructions are hoisted.
+; Store hoisting is disabled.
+; CHECK: store i8
 ; CHECK: store i8
-; CHECK-NOT: store i8
 ; CHECK: stackrestore
 
 define void @test(i1 %b) {
 entry:
   %x = alloca i8
   %inalloca.save = call i8* @llvm.stacksave()
   %argmem = alloca inalloca <{ %struct.S }>, align 4
   %0 = getelementptr inbounds <{ %struct.S }>, <{ %struct.S }>* %argmem, i32 0, i32 0

llvm/test/Transforms/GVNHoist/hoist-pr28933.ll

 ; RUN: opt -S -gvn-hoist -verify-memoryssa < %s | FileCheck %s
 
-; Check that we end up with one load and one store, in the right order
+; Store hoisting is disabled.
 ; CHECK-LABEL:  define void @test_it(
 ; CHECK: store
 ; CHECK-NEXT: load
-; CHECK-NOT: store
-; CHECK-NOT: load
+; CHECK: store
+; CHECK: load
         
 %rec894.0.1.2.3.12 = type { i16 }
 
 @a = external global %rec894.0.1.2.3.12
 
 define void @test_it() {
 bb2:
   store i16 undef, i16* getelementptr inbounds (%rec894.0.1.2.3.12, %rec894.0.1.2.3.12* @a, i16 0, i32 0), align 1
   %_tmp61 = load i16, i16* getelementptr inbounds (%rec894.0.1.2.3.12, %rec894.0.1.2.3.12* @a, i16 0, i32 0), align 1
   store i16 undef, i16* getelementptr inbounds (%rec894.0.1.2.3.12, %rec894.0.1.2.3.12* @a, i16 0, i32 0), align 1
   %_tmp92 = load i16, i16* getelementptr inbounds (%rec894.0.1.2.3.12, %rec894.0.1.2.3.12* @a, i16 0, i32 0), align 1
   ret void
 }

llvm/test/Transforms/GVNHoist/hoist.ll

 if.end:
   %tmax.0 = phi float [ %mul2, %while.cond ], [ %mul6, %if.else ]
   %tmin.0 = phi float [ %mul, %while.cond ], [ %mul4, %if.else ]
 
   %add = fadd float %tmax.0, %tmin.0
   ret float %add
 }
 
-; Check that we hoist stores: all the instructions from the then branch
-; should be hoisted.
+; Store hoisting is disabled.
 ; CHECK-LABEL: @hoistStores
-; CHECK: zext
-; CHECK-NEXT: trunc
-; CHECK-NEXT: getelementptr
-; CHECK-NEXT: load
-; CHECK-NEXT: getelementptr
-; CHECK-NEXT: getelementptr
-; CHECK-NEXT: store
-; CHECK-NEXT: load
-; CHECK-NEXT: load
-; CHECK-NEXT: zext
-; CHECK-NEXT: add
-; CHECK-NEXT: store
-; CHECK-NEXT: br
-; CHECK: if.then
-; CHECK: br
 
 %struct.foo = type { i16* }
 
 define void @hoistStores(%struct.foo* %s, i32* %coord, i1 zeroext %delta) {
 entry:
   %frombool = zext i1 %delta to i8
   %tobool = trunc i8 %frombool to i1
   br i1 %tobool, label %if.then, label %if.else

llvm/test/Transforms/GVNHoist/pr30216.ll

+; RUN: opt -S -gvn-hoist < %s | FileCheck %s
+
+; Make sure the two stores @B do not get hoisted past the load @B.
+
+; CHECK-LABEL: define i8* @Foo
+; CHECK: store
+; CHECK: store
+; CHECK: load
+; CHECK: store
+
+@A = external global i8
+@B = external global i8*
+
+define i8* @Foo() {
+  store i8 0, i8* @A
+  br i1 undef, label %if.then, label %if.else
+
+if.then:
+  store i8* null, i8** @B
+  ret i8* null
+
+if.else:
+  %1 = load i8*, i8** @B
+  store i8* null, i8** @B
+  ret i8* %1
+}