[PATCH] D84399: [SCEVExpander] Avoid re-sing existing casts if it means updating users.

[Florian Hahn via Phabricator via llvm-commits]

fhahn created this revision.
fhahn added reviewers: efriedma, mkazantsev, reames, sanjoy.google.
Herald added subscribers: javed.absar, hiraditya.
Herald added a project: LLVM.

Currently the SCEVExpander tries to re-use existing casts, even if they
are not exactly at the insertion point it was asked to create the cast.
To do so in some case, it creates a new cast at the insertion point and
updates all users to use the new cast.

This behavior is problematic, because it changes the IR outside of the
instructions created during the expansion. Therefore we cannot
completely undo all changes made during expansion.

This re-use should be only an extra optimization, so only using the new
cast in the expanded instructions should not be a correctness issue.
There are many cases equivalent instructions are created during
expansion.

This has minor impact on the tests, but there is one LoopDistribute test
that checks explicitly that only the new value is used. For now I just
XFAIL'd the test.

Once SCEVExpanderCleaner is added, we could keep a mapping of
values to replace and apply them once the result is used.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D84399

Files:
  llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
  llvm/test/Transforms/LoopDistribute/bounds-expansion-bug.ll
  llvm/test/Transforms/LoopIdiom/reuse-cast.ll
  llvm/test/Transforms/LoopStrengthReduce/pr27056.ll


Index: llvm/test/Transforms/LoopStrengthReduce/pr27056.ll
===================================================================
--- llvm/test/Transforms/LoopStrengthReduce/pr27056.ll
+++ llvm/test/Transforms/LoopStrengthReduce/pr27056.ll
@@ -37,7 +37,7 @@
 ; CHECK-NEXT:    br label [[FOR_BODY:%.*]]
 ; CHECK:       for.body:
 ; CHECK-NEXT:    [[TMP5:%.*]] = inttoptr i64 [[LSR_IV]] to %struct.L*
-; CHECK-NEXT:    [[CMP:%.*]] = icmp eq %struct.L* [[UGLYGEP1]], [[TMP]]
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq %struct.L* [[UGLYGEP1]], [[TMP5]]
 ; CHECK-NEXT:    br i1 [[CMP]], label [[FOR_END_LOOPEXIT:%.*]], label [[FOR_BODY]]
 ; CHECK:       for.end.loopexit:
 ; CHECK-NEXT:    br label [[FOR_END]]
Index: llvm/test/Transforms/LoopIdiom/reuse-cast.ll
===================================================================
--- llvm/test/Transforms/LoopIdiom/reuse-cast.ll
+++ llvm/test/Transforms/LoopIdiom/reuse-cast.ll
@@ -84,9 +84,8 @@
 ; CHECK-LABEL: @reuse_cast_2(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[STACK:%.*]] = alloca [2 x i32], align 4
-; CHECK-NEXT:    [[CAST_TO_REUSE:%.*]] = bitcast [2 x i32]* [[STACK]] to i8*
 ; CHECK-NEXT:    [[C_0:%.*]] = icmp sgt i32 [[X:%.*]], 0
-; CHECK-NEXT:    [[TMP0:%.*]] = bitcast [2 x i32]* [[STACK]] to i8*
+; CHECK-NEXT:    [[CAST_TO_REUSE:%.*]] = bitcast [2 x i32]* [[STACK]] to i8*
 ; CHECK-NEXT:    [[PTR_2_START:%.*]] = getelementptr inbounds [2 x i32], [2 x i32]* [[STACK]], i64 0, i64 0
 ; CHECK-NEXT:    call void @use.i8(i8* [[CAST_TO_REUSE]])
 ; CHECK-NEXT:    br label [[LOOP:%.*]]
Index: llvm/test/Transforms/LoopDistribute/bounds-expansion-bug.ll
===================================================================
--- llvm/test/Transforms/LoopDistribute/bounds-expansion-bug.ll
+++ llvm/test/Transforms/LoopDistribute/bounds-expansion-bug.ll
@@ -1,5 +1,12 @@
 ; RUN: opt -basic-aa -loop-distribute -enable-loop-distribute -S < %s | FileCheck %s
 
+; XFAIL: *
+
+; SCEVExpander now less aggressively re-uses existing casts, in order to avoid
+; touching IR outside of the expanded instructions. Re-using existing casts
+; should only be a small optimization and can be re-introduced once we can
+; properly clean up expansion results, if they are unused.
+
 ; When emitting the memchecks for:
 ;
 ;   for (i = 0; i < n; i++) {
Index: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
===================================================================
--- llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
+++ llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
@@ -38,9 +38,7 @@
 using namespace PatternMatch;
 
 /// ReuseOrCreateCast - Arrange for there to be a cast of V to Ty at IP,
-/// reusing an existing cast if a suitable one exists, moving an existing
-/// cast if a suitable one exists but isn't in the right place, or
-/// creating a new one.
+/// reusing an existing cast if a suitable one exists, or creating a new one.
 Value *SCEVExpander::ReuseOrCreateCast(Value *V, Type *Ty,
                                        Instruction::CastOps Op,
                                        BasicBlock::iterator IP) {
@@ -66,11 +64,8 @@
           // Likewise, do not reuse a cast at BIP because it must dominate
           // instructions that might be inserted before BIP.
           if (BasicBlock::iterator(CI) != IP || BIP == IP) {
-            // Create a new cast, and leave the old cast in place in case
-            // it is being used as an insert point.
             Ret = CastInst::Create(Op, V, Ty, "", &*IP);
-            Ret->takeName(CI);
-            CI->replaceAllUsesWith(Ret);
+            rememberInstruction(Ret);
             break;
           }
           Ret = CI;
@@ -78,15 +73,16 @@
         }
 
   // Create a new cast.
-  if (!Ret)
+  if (!Ret) {
     Ret = CastInst::Create(Op, V, Ty, V->getName(), &*IP);
+    rememberInstruction(Ret);
+  }
 
   // We assert at the end of the function since IP might point to an
   // instruction with different dominance properties than a cast
   // (an invoke for example) and not dominate BIP (but the cast does).
   assert(SE.DT.dominates(Ret, &*BIP));
 
-  rememberInstruction(Ret);
   return Ret;
 }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D84399.280090.patch
Type: text/x-patch
Size: 4151 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200723/50bc4bb4/attachment.bin>