This is an archive of the discontinued LLVM Phabricator instance.

[LoopInstSimplify] Discard SCEV if simplification happened.
AbandonedPublic

Authored by lebedev.ri on Jun 26 2018, 8:53 AM.

Details

Summary

Fixes PR37936.

The problem is as follows:

  1. indvars marks %dec as NUW.
  2. loop-instsimplify runs instsimplify, which constant-folds %dec to -1 (D47908)
  3. loop-reduce tries to do some further modification, but crashes with an type assertion in cast, because %dec is no longer an Instruction,

If the runline is split into two, i.e. you first run -indvars -loop-instsimplify,
store that into a file, and then run -loop-reduce, there is no crash.

So it looks like the problem is due to -loop-instsimplify not discarding SCEV.
This does fix the crash (without the patch, that test file crashes),
although i do not know whether there is some other problem that should be solved.

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.Jun 26 2018, 8:53 AM

IMO this doesn't seem like an invalidation bug -- SCEV Expander needs to be smarter about dealing with these kinds of simplifications.

IMO this doesn't seem like an invalidation bug -- SCEV Expander needs to be smarter about dealing with these kinds of simplifications.

Does this sound better?

diff --git a/lib/Analysis/ScalarEvolutionExpander.cpp b/lib/Analysis/ScalarEvolutionExpander.cpp
index f2ce0f4aa86..2058e591a77 100644
--- a/lib/Analysis/ScalarEvolutionExpander.cpp
+++ b/lib/Analysis/ScalarEvolutionExpander.cpp
@@ -1169,8 +1169,10 @@ SCEVExpander::getAddRecExprPHILiterally(const SCEVAddRecExpr *Normalized,
       if (!IsMatchingSCEV && !TryNonMatchingSCEV)
           continue;
 
-      Instruction *TempIncV =
-          cast<Instruction>(PN.getIncomingValueForBlock(LatchBlock));
+      Value* IncomingValue = PN.getIncomingValueForBlock(LatchBlock);
+      if(!isa<Instruction>(IncomingValue))
+        continue;
+      Instruction *TempIncV = cast<Instruction>(IncomingValue);
 
       // Check whether we can reuse this PHI node.
       if (LSRMode) {

IMO this doesn't seem like an invalidation bug -- SCEV Expander needs to be smarter about dealing with these kinds of simplifications.

Does this sound better?

Yes -- that seems better to me.

diff --git a/lib/Analysis/ScalarEvolutionExpander.cpp b/lib/Analysis/ScalarEvolutionExpander.cpp
index f2ce0f4aa86..2058e591a77 100644
--- a/lib/Analysis/ScalarEvolutionExpander.cpp
+++ b/lib/Analysis/ScalarEvolutionExpander.cpp
@@ -1169,8 +1169,10 @@ SCEVExpander::getAddRecExprPHILiterally(const SCEVAddRecExpr *Normalized,
       if (!IsMatchingSCEV && !TryNonMatchingSCEV)
           continue;
 
-      Instruction *TempIncV =
-          cast<Instruction>(PN.getIncomingValueForBlock(LatchBlock));
+      Value* IncomingValue = PN.getIncomingValueForBlock(LatchBlock);
+      if(!isa<Instruction>(IncomingValue))
+        continue;
+      Instruction *TempIncV = cast<Instruction>(IncomingValue);
 
       // Check whether we can reuse this PHI node.
       if (LSRMode) {
lebedev.ri abandoned this revision.Jun 26 2018, 10:31 AM

IMO this doesn't seem like an invalidation bug -- SCEV Expander needs to be smarter about dealing with these kinds of simplifications.

Does this sound better?

Yes -- that seems better to me.

Superseded by https://reviews.llvm.org/D48599
Although that is a less general fix, so there may be other problems.

This change indeed makes the crash go away but I have no idea if this is the right solution or if https://reviews.llvm.org/D48599 is better.