Unverified Commit c24fc717 authored by Nikita Popov's avatar Nikita Popov Committed by GitHub
Browse files

Revert [LICM] Remove unnecessary check during store hoisting (#195606) 

This check is needed after all, to handle the case where the load
aliases only on the first iteration. Even with correct cross-iteration
handling in MSSA, it's legal to return an out of loop clobbering memory
accesses in this case.

Reverts https://github.com/llvm/llvm-project/pull/187529.
Fixes https://github.com/llvm/llvm-project/issues/195513.
parent c859a273

llvm/lib/Transforms/Scalar/LICM.cpp

+6 −0
Original line number Diff line number Diff line
@@ -2336,6 +2336,12 @@ static bool noConflictingReadWrites(Instruction *I, MemorySSA *MSSA, 
                                             const_cast<MemoryUse *>(MU));

        if (!MSSA->isLiveOnEntryDef(MD) && CurLoop->contains(MD->getBlock()))

          return false;

        // Disable hoisting past potentially interfering loads. Optimized

        // Uses may point to an access outside the loop, as getClobbering

        // checks the previous iteration when walking the backedge.

        // FIXME: More precise: no Uses that alias I.

        if (!Flags.getIsSink() && !MSSA->dominates(IMD, MU))

          return false;

      } else if (const auto *MD = dyn_cast<MemoryDef>(&MA)) {

        if (auto *LI = dyn_cast<LoadInst>(MD->getMemoryInst())) {

          (void)LI; // Silence warning.

llvm/test/Transforms/LICM/call-hoisting.ll

+2 −1
Original line number Diff line number Diff line
@@ -277,16 +277,17 @@ exit: 
  ret i32 %val

}



; FIXME: It's safe to hoist @store(), because @load() does not alias.

define i32 @unrelated_read(ptr noalias %loc, ptr noalias %otherloc) {

; CHECK-LABEL: define i32 @unrelated_read(

; CHECK-SAME: ptr noalias [[LOC:%.*]], ptr noalias [[OTHERLOC:%.*]]) {

; CHECK-NEXT:  [[ENTRY:.*]]:

; CHECK-NEXT:    call void @store(i32 0, ptr [[LOC]])

; CHECK-NEXT:    br label %[[LOOP:.*]]

; CHECK:       [[LOOP]]:

; CHECK-NEXT:    [[IV:%.*]] = phi i32 [ 0, %[[ENTRY]] ], [ [[IV_NEXT:%.*]], %[[LOOP]] ]

; CHECK-NEXT:    [[OTHERLOC_GEP:%.*]] = getelementptr i32, ptr [[OTHERLOC]], i32 [[IV]]

; CHECK-NEXT:    [[VAL:%.*]] = call i32 @load(ptr [[OTHERLOC_GEP]])

; CHECK-NEXT:    call void @store(i32 0, ptr [[LOC]])

; CHECK-NEXT:    [[IV_NEXT]] = add i32 [[IV]], 1

; CHECK-NEXT:    [[CMP:%.*]] = icmp slt i32 [[IV]], 200

; CHECK-NEXT:    br i1 [[CMP]], label %[[LOOP]], label %[[EXIT:.*]]

llvm/test/Transforms/LICM/hoist-inaccesiblemem-call.ll

+5 −3
Original line number Diff line number Diff line
@@ -4,16 +4,18 @@ 
;; It should hoist fn_write_inaccessible_mem

;; because there is no conflict between inaccessible memory

;; fn_read_inaccessible_mem is a nice side effect

; FIXME: fn_write_inaccessible_mem is currently not hoisted due to the preceding

; load, even though it does not alias.

define i32 @loop_alias(i64 %x, ptr %start) {

; CHECK-LABEL: define i32 @loop_alias(

; CHECK-SAME: i64 [[X:%.*]], ptr [[START:%.*]]) {

; CHECK-NEXT:  [[ENTRY:.*]]:

; CHECK-NEXT:    call void @fn_write_inaccessible_mem()

; CHECK-NEXT:    br label %[[LOOP:.*]]

; CHECK:       [[LOOP]]:

; CHECK-NEXT:    [[PHI:%.*]] = phi ptr [ [[START]], %[[ENTRY]] ], [ [[GEP:%.*]], %[[LOOP]] ]

; CHECK-NEXT:    [[LOAD:%.*]] = load i32, ptr [[PHI]]

; CHECK-NEXT:    [[LOAD:%.*]] = load i32, ptr [[PHI]], align 4

; CHECK-NEXT:    [[VAL:%.*]] = call i32 @fn_args(i32 [[LOAD]])

; CHECK-NEXT:    call void @fn_write_inaccessible_mem()

; CHECK-NEXT:    call void @fn_read_inaccessible_mem(i32 [[LOAD]])

; CHECK-NEXT:    [[GEP]] = getelementptr inbounds nuw i32, ptr [[PHI]], i64 [[X]]

; CHECK-NEXT:    [[ACC:%.*]] = add nuw nsw i32 [[VAL]], 1
@@ -48,7 +50,7 @@ define i32 @ne_loop_alias(i64 %x, ptr %start) { 
; CHECK-NEXT:    br label %[[LOOP:.*]]

; CHECK:       [[LOOP]]:

; CHECK-NEXT:    [[PHI:%.*]] = phi ptr [ [[START]], %[[ENTRY]] ], [ [[GEP:%.*]], %[[LOOP]] ]

; CHECK-NEXT:    [[LOAD:%.*]] = load i32, ptr [[PHI]]

; CHECK-NEXT:    [[LOAD:%.*]] = load i32, ptr [[PHI]], align 4

; CHECK-NEXT:    [[VAL:%.*]] = call i32 @fn_read_inaccessible_mem_2(i32 [[LOAD]])

; CHECK-NEXT:    call void @fn_write_inaccessible_mem()

; CHECK-NEXT:    call void @fn_read_inaccessible_mem(i32 [[VAL]])

llvm/test/Transforms/LICM/pr54495.ll

+1 −1
Original line number Diff line number Diff line
@@ -6,7 +6,6 @@ 
define void @test(ptr %p1, ptr %p2, ptr noalias %p3) {

; CHECK-LABEL: @test(

; CHECK-NEXT:  entry:

; CHECK-NEXT:    store ptr [[P3:%.*]], ptr [[P3]], align 8

; CHECK-NEXT:    br label [[LOOP:%.*]]

; CHECK:       loop:

; CHECK-NEXT:    [[P:%.*]] = phi ptr [ [[P1:%.*]], [[ENTRY:%.*]] ], [ [[P2:%.*]], [[LOOP]] ]
@@ -14,6 +13,7 @@ define void @test(ptr %p1, ptr %p2, ptr noalias %p3) { 
; CHECK-NEXT:    [[CMP:%.*]] = icmp eq i64 [[V]], 0

; CHECK-NEXT:    br i1 [[CMP]], label [[LOOP]], label [[LOOP_EXIT:%.*]]

; CHECK:       loop.exit:

; CHECK-NEXT:    store ptr [[P3:%.*]], ptr [[P3]], align 8

; CHECK-NEXT:    ret void

;

entry:

llvm/test/Transforms/LICM/store-hoisting.ll

+35 −0
Original line number Diff line number Diff line
@@ -689,3 +689,38 @@ merge: 
exit:

  ret void

}



@g1 = external global i8

@g2 = external global i8



; The store should not be hoisted here, because the location is accessed by the

; preceding load. However, the clobbering memory access for that load is

; liveOnEntry, because the pointer on the backedge does not alias. As such,

; only checking whether the clobbering access is outside the loop is

; insufficient.

define i8 @load_before_store_with_out_of_loop_def(i1 %c) {

; CHECK-LABEL: define i8 @load_before_store_with_out_of_loop_def(

; CHECK-SAME: i1 [[C:%.*]]) {

; CHECK-NEXT:  [[ENTRY:.*]]:

; CHECK-NEXT:    br label %[[LOOP:.*]]

; CHECK:       [[LOOP]]:

; CHECK-NEXT:    [[PHI:%.*]] = phi ptr [ @g1, %[[ENTRY]] ], [ @g2, %[[LOOP]] ]

; CHECK-NEXT:    [[V:%.*]] = load i8, ptr [[PHI]], align 1

; CHECK-NEXT:    store i8 1, ptr @g1, align 1

; CHECK-NEXT:    br i1 [[C]], label %[[LOOP]], label %[[EXIT:.*]]

; CHECK:       [[EXIT]]:

; CHECK-NEXT:    [[V_LCSSA:%.*]] = phi i8 [ [[V]], %[[LOOP]] ]

; CHECK-NEXT:    ret i8 [[V_LCSSA]]

;

entry:

  br label %loop



loop:

  %phi = phi ptr [ @g1, %entry ], [ @g2, %loop ]

  %v = load i8, ptr %phi

  store i8 1, ptr @g1

  br i1 %c, label %loop, label %exit



exit:

  ret i8 %v

}