[llvm] e81334a - [LICM] Remove MaybePromotable set (PR50367)

[Nikita Popov via llvm-commits]

Author: Nikita Popov
Date: 2021-05-18T20:26:01+02:00
New Revision: e81334a75401f3af3b10f64e307d705d97637a03

URL: https://github.com/llvm/llvm-project/commit/e81334a75401f3af3b10f64e307d705d97637a03
DIFF: https://github.com/llvm/llvm-project/commit/e81334a75401f3af3b10f64e307d705d97637a03.diff

LOG: [LICM] Remove MaybePromotable set (PR50367)

The MaybePromotable set keeps track of loads/stores for which
promotion was not attempted yet. Normally, any load/stores that
are promoted in the current iteration will be removed from this
set, because they naturally MustAlias with the promoted value.
However, if the source program has UB with metadata claiming that
a store is NoAlias, while it is actually MustAlias, and multiple
different pointers are promoted in the same iteration, it can
happen that a store is removed that is still in the MaybePromotable
set, causing a use-after-free.

While this could be fixed by explicitly invalidating values in
MaybePromotable in the LoopPromoter, I'm going with the more
radical option of dropping the set entirely here and check all
load/stores on each promotion iteration. As promotion, and especially
repeated promotion, are quite rare, this doesn't seem to have any
impact on compile-time.

Fixes https://bugs.llvm.org/show_bug.cgi?id=50367.

Added: 
    llvm/test/Transforms/LICM/pr50367.ll

Modified: 
    llvm/lib/Transforms/Scalar/LICM.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/Scalar/LICM.cpp b/llvm/lib/Transforms/Scalar/LICM.cpp
index a2bda9d5805ad..22ed6ae6c538d 100644
--- a/llvm/lib/Transforms/Scalar/LICM.cpp
+++ b/llvm/lib/Transforms/Scalar/LICM.cpp
@@ -189,8 +189,7 @@ static void moveInstructionBefore(Instruction &I, Instruction &Dest,
 static void foreachMemoryAccess(MemorySSA *MSSA, Loop *L,
                                 function_ref<void(Instruction *)> Fn);
 static SmallVector<SmallSetVector<Value *, 8>, 0>
-collectPromotionCandidates(MemorySSA *MSSA, AliasAnalysis *AA, Loop *L,
-                           SmallVectorImpl<Instruction *> &MaybePromotable);
+collectPromotionCandidates(MemorySSA *MSSA, AliasAnalysis *AA, Loop *L);
 
 namespace {
 struct LoopInvariantCodeMotion {
@@ -473,11 +472,6 @@ bool LoopInvariantCodeMotion::runOnLoop(
               DT, TLI, L, CurAST.get(), MSSAU.get(), &SafetyInfo, ORE);
         }
       } else {
-        SmallVector<Instruction *, 16> MaybePromotable;
-        foreachMemoryAccess(MSSA, L, [&](Instruction *I) {
-          MaybePromotable.push_back(I);
-        });
-
         // Promoting one set of accesses may make the pointers for another set
         // loop invariant, so run this in a loop (with the MaybePromotable set
         // decreasing in size over time).
@@ -485,7 +479,7 @@ bool LoopInvariantCodeMotion::runOnLoop(
         do {
           LocalPromoted = false;
           for (const SmallSetVector<Value *, 8> &PointerMustAliases :
-               collectPromotionCandidates(MSSA, AA, L, MaybePromotable)) {
+               collectPromotionCandidates(MSSA, AA, L)) {
             LocalPromoted |= promoteLoopAccessesToScalars(
                 PointerMustAliases, ExitBlocks, InsertPts, MSSAInsertPts, PIC,
                 LI, DT, TLI, L, /*AST*/nullptr, MSSAU.get(), &SafetyInfo, ORE);
@@ -2279,8 +2273,7 @@ static void foreachMemoryAccess(MemorySSA *MSSA, Loop *L,
 }
 
 static SmallVector<SmallSetVector<Value *, 8>, 0>
-collectPromotionCandidates(MemorySSA *MSSA, AliasAnalysis *AA, Loop *L,
-                           SmallVectorImpl<Instruction *> &MaybePromotable) {
+collectPromotionCandidates(MemorySSA *MSSA, AliasAnalysis *AA, Loop *L) {
   AliasSetTracker AST(*AA);
 
   auto IsPotentiallyPromotable = [L](const Instruction *I) {
@@ -2294,13 +2287,11 @@ collectPromotionCandidates(MemorySSA *MSSA, AliasAnalysis *AA, Loop *L,
   // Populate AST with potentially promotable accesses and remove them from
   // MaybePromotable, so they will not be checked again on the next iteration.
   SmallPtrSet<Value *, 16> AttemptingPromotion;
-  llvm::erase_if(MaybePromotable, [&](Instruction *I) {
+  foreachMemoryAccess(MSSA, L, [&](Instruction *I) {
     if (IsPotentiallyPromotable(I)) {
       AttemptingPromotion.insert(I);
       AST.add(I);
-      return true;
     }
-    return false;
   });
 
   // We're only interested in must-alias sets that contain a mod.

diff  --git a/llvm/test/Transforms/LICM/pr50367.ll b/llvm/test/Transforms/LICM/pr50367.ll
new file mode 100644
index 0000000000000..e660d9f626e13
--- /dev/null
+++ b/llvm/test/Transforms/LICM/pr50367.ll
@@ -0,0 +1,43 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -passes='loop-mssa(licm)' < %s | FileCheck %s
+ at e = external dso_local global i32*, align 8
+
+define void @main() {
+; CHECK-LABEL: @main(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br label [[LOOP1:%.*]]
+; CHECK:       loop1:
+; CHECK-NEXT:    br label [[LOOP2:%.*]]
+; CHECK:       loop2:
+; CHECK-NEXT:    br i1 false, label [[LOOP2_LATCH:%.*]], label [[LOOP_LATCH:%.*]]
+; CHECK:       loop2.latch:
+; CHECK-NEXT:    br label [[LOOP2]]
+; CHECK:       loop.latch:
+; CHECK-NEXT:    br label [[LOOP1]]
+;
+entry:
+  br label %loop1
+
+loop1:
+  br label %loop2
+
+loop2:
+  br i1 undef, label %loop2.latch, label %loop.latch
+
+loop2.latch:
+  store i32 0, i32* null, align 4
+  br label %loop2
+
+loop.latch:
+  store i32* null, i32** @e, align 8, !tbaa !0
+  %ptr = load i32*, i32** @e, align 8, !tbaa !0
+  store i32 0, i32* %ptr, align 4, !tbaa !4
+  br label %loop1
+}
+
+!0 = !{!1, !1, i64 0}
+!1 = !{!"any pointer", !2, i64 0}
+!2 = !{!"omnipotent char", !3, i64 0}
+!3 = !{!"Simple C/C++ TBAA"}
+!4 = !{!5, !5, i64 0}
+!5 = !{!"int", !2, i64 0}