[PATCH] D17936: [LICM] Don't silently ignore constant expressions in promoteLoopAccessesToScalars()

[Michael Kuperstein via llvm-commits]

mkuper created this revision.
mkuper added reviewers: bruno, hfinkel.
mkuper added a subscriber: llvm-commits.
Herald added a subscriber: aemerson.

Right now, the loop that iterates over the pointers in each alias set ignores constantexpr uses of these pointers - which is unfortunate, since it misses the case where the pointer is a constant, and a constant expression that uses the pointer is present inside the loop.

I'm not too happy with this patch - it seems like there should be a simpler way to do this. Any suggestions are welcome, especially if I'm missing an obvious one.

Also, it is intentionally conservative, since it'll bail even when the constantexpr is harmless (e.g. a bitcast from i32* to float* that goes into a load). Ideally, it should probably just feed the instructions that use the constantexpr into the same loop as the rest of the pointer's users, but I'm afraid that may introduce other edge cases.

This fixes PR26843.

http://reviews.llvm.org/D17936

Files:
  include/llvm/Transforms/Utils/LoopUtils.h
  lib/Transforms/Scalar/LICM.cpp
  test/Transforms/LICM/pr26843.ll

Index: test/Transforms/LICM/pr26843.ll
===================================================================
--- test/Transforms/LICM/pr26843.ll
+++ test/Transforms/LICM/pr26843.ll
@@ -0,0 +1,32 @@
+; RUN: opt -S -basicaa -licm < %s | FileCheck %s
+
+target datalayout = "e-m:x-p:32:32-i64:64-f80:32-n8:16:32-a:0:32-S32"
+target triple = "i686-pc-windows-msvc18.0.0"
+
+ at v = common global [4 x i32] zeroinitializer, align 4 
+
+; Make sure the store to v[1] is not sunk past the memset to v[1]-v[3].
+; CHECK-LABEL: @main
+; CHECK: for.body:
+; CHECK-NEXT: store i32 1, i32* getelementptr inbounds ([4 x i32], [4 x i32]* @v, i32 0, i32 1)
+; CHECK-NEXT: tail call void @llvm.memset
+; CHECK: end:
+; CHECK-NEXT: ret i32 0
+
+define i32 @main(i1 %k) {
+entry:
+  br label %for.body
+ 
+for.body:
+  store i32 1, i32* getelementptr inbounds ([4 x i32], [4 x i32]* @v, i32 0, i32 1), align 4
+  tail call void @llvm.memset.p0i8.i32(i8* bitcast (i32* getelementptr inbounds ([4 x i32], [4 x i32]* @v, i32 0, i32 1) to i8*), i8 0, i32 12, i32 4, i1 false)
+  br label %for.latch
+  
+for.latch:
+  br i1 %k, label %for.body, label %end
+
+end:
+  ret i32 0
+}
+
+declare void @llvm.memset.p0i8.i32(i8* nocapture, i8, i32, i32, i1)
Index: lib/Transforms/Scalar/LICM.cpp
===================================================================
--- lib/Transforms/Scalar/LICM.cpp
+++ lib/Transforms/Scalar/LICM.cpp
@@ -837,6 +837,28 @@
   };
 } // end anon namespace
 
+/// Check whether a constant expression is used by any instruction
+/// in a given loop. This, unforunately, requires a recursive lookup.
+bool llvm::isConstantExprUsedInLoop(ConstantExpr *CE, Loop *CurLoop) {
+  SmallVector<ConstantExpr *, 4> WorkList;
+  WorkList.push_back(CE);
+
+  while (!WorkList.empty()) {
+    auto Curr = WorkList.pop_back_val();
+    // We only care if the user is an instruction or a constant expression.
+    // Check if instructions are in the loop, and recurse down ConstantExpr.
+    for (auto U : Curr->users()) {
+      if (auto UI = dyn_cast<Instruction>(U)) {
+        if (CurLoop->contains(UI))
+          return true;
+      } else if (auto UCE = dyn_cast<ConstantExpr>(U))
+        WorkList.push_back(UCE);
+    }
+  }
+
+  return false;
+}
+
 /// Try to promote memory values to scalars by sinking stores out of the
 /// loop and moving loads to before the loop.  We do this by looping over
 /// the stores in the loop, looking for stores to Must pointers which are
@@ -907,6 +929,14 @@
       return Changed;
 
     for (User *U : ASIV->users()) {
+      // If we encounter a constant expression which is used by an instruction
+      // inside the loop, bail.
+      // FIXME: This is somewhat too conservative, but for now, err on the side
+      // of correctness.
+      if (auto UCE = dyn_cast<ConstantExpr>(U))
+        if (isConstantExprUsedInLoop(UCE, CurLoop))
+          return Changed;
+
       // Ignore instructions that are outside the loop.
       Instruction *UI = dyn_cast<Instruction>(U);
       if (!UI || !CurLoop->contains(UI))
Index: include/llvm/Transforms/Utils/LoopUtils.h
===================================================================
--- include/llvm/Transforms/Utils/LoopUtils.h
+++ include/llvm/Transforms/Utils/LoopUtils.h
@@ -376,6 +376,10 @@
                                   DominatorTree *, Loop *, AliasSetTracker *,
                                   LICMSafetyInfo *);
 
+/// \brief Check whether a constant expression is used by any instruction
+/// in a given loop.
+bool isConstantExprUsedInLoop(ConstantExpr* CE, Loop *CurLoop);
+
 /// \brief Computes safety information for a loop
 /// checks loop body & header for the possibility of may throw
 /// exception, it takes LICMSafetyInfo and loop as argument.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D17936.49985.patch
Type: text/x-patch
Size: 3835 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160307/67e812ee/attachment.bin>