[PATCH] D129370: [SimplifyCFG] Allow SimplifyCFG hoisting to skip over non-matching instructions

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:1558
+      if (I1->getType()->isTokenTy())
         return Changed;
 
----------------
This check should not be present (likely rebase mistake).


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:1638
+      if (I2->mayHaveSideEffects())
+        ForceNoReadMemOrSideEffectsBB2 = true;
+      if (!isGuaranteedToTransferExecutionToSuccessor(I1))
----------------
This check is still incorrect. Consider this example:
```
define i16 @f7(i1 %c, ptr %a, ptr %b) {
; CHECK-LABEL: @f7(
; CHECK-NEXT:  entry:
; CHECK-NEXT:    store i16 0, ptr [[B:%.*]], align 2
; CHECK-NEXT:    br i1 [[C:%.*]], label [[IF_THEN:%.*]], label [[IF_ELSE:%.*]]
; CHECK:       if.then:
; CHECK-NEXT:    [[VA:%.*]] = load i16, ptr [[A:%.*]], align 2
; CHECK-NEXT:    br label [[IF_END:%.*]]
; CHECK:       if.else:
; CHECK-NEXT:    [[VB:%.*]] = load i16, ptr [[B]], align 2
; CHECK-NEXT:    br label [[IF_END]]
; CHECK:       if.end:
; CHECK-NEXT:    [[V:%.*]] = phi i16 [ [[VA]], [[IF_THEN]] ], [ [[VB]], [[IF_ELSE]] ]
; CHECK-NEXT:    ret i16 [[V]]
;
entry:
  br i1 %c, label %if.then, label %if.else

if.then:
  %va = load i16, ptr %a, align 2
  store i16 0, ptr %b, align 2
  br label %if.end

if.else:
  %vb = load i16, ptr %b, align 2
  store i16 0, ptr %b, align 2
  br label %if.end

if.end:
  %v = phi i16 [ %va, %if.then ], [ %vb, %if.else ]
  ret i16 %v
}
```
You are currently hoisting the store above the loads.

What you want to check here is mayReadOrWriteMemory, not mayHaveSideEffects. (The non-memory effects are modeled by the other check now).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129370/new/

https://reviews.llvm.org/D129370