[PATCH] D42290: [SCEV] Clear poison flags during expansion of SCEV

[Serguei Katkov via Phabricator via llvm-commits]

skatkov created this revision.
skatkov added reviewers: sanjoy, mkazantsev, sebpop, eastig.
Herald added a reviewer: dberlin.

This is an alternate solution to https://reviews.llvm.org/rL322058 which causes performance
degradation according to report from @eastig.

Generally when we expand a SCEV and re-use the instruction we
check whether SCEV has lost a poison flags but instruction has.
In this case we just clear poison flags from instruction.

This is pretty similar what GVN will do if we we literally expand the SCEV
without poison flags.

Potentially it might have negative performance impact due to we clear
nuw/nsw from instruction and possible loose some performance
opportunities.


https://reviews.llvm.org/D42290

Files:
  lib/Analysis/ScalarEvolution.cpp
  lib/Analysis/ScalarEvolutionExpander.cpp


Index: lib/Analysis/ScalarEvolutionExpander.cpp
===================================================================
--- lib/Analysis/ScalarEvolutionExpander.cpp
+++ lib/Analysis/ScalarEvolutionExpander.cpp
@@ -1688,9 +1688,28 @@
   return V;
 }
 
+/// Check whether value has nuw/nsw/exact set but SCEV does not.
+/// TODO: In reality it is better to check the poison recursevely
+/// but this is better than nothing.
+static bool SCEVLostPoisonFlags(const SCEV *S, const Value *V) {
+  if (auto *I = dyn_cast<Instruction>(V)) {
+    if (isa<OverflowingBinaryOperator>(I)) {
+      if (auto *NS = dyn_cast<SCEVNAryExpr>(S)) {
+        if (I->hasNoSignedWrap() && !NS->hasNoSignedWrap())
+          return true;
+        if (I->hasNoUnsignedWrap() && !NS->hasNoUnsignedWrap())
+          return true;
+      }
+    } else if (isa<PossiblyExactOperator>(I) && I->isExact())
+      return true;
+  }
+  return false;
+}
+
 ScalarEvolution::ValueOffsetPair
 SCEVExpander::FindValueInExprValueMap(const SCEV *S,
                                       const Instruction *InsertPt) {
+  ScalarEvolution::ValueOffsetPair Candidate = { nullptr, nullptr };
   SetVector<ScalarEvolution::ValueOffsetPair> *Set = SE.getSCEVValues(S);
   // If the expansion is not in CanonicalMode, and the SCEV contains any
   // sub scAddRecExpr type SCEV, it is required to expand the SCEV literally.
@@ -1709,11 +1728,24 @@
             EntInst->getFunction() == InsertPt->getFunction() &&
             SE.DT.dominates(EntInst, InsertPt) &&
             (SE.LI.getLoopFor(EntInst->getParent()) == nullptr ||
-             SE.LI.getLoopFor(EntInst->getParent())->contains(InsertPt)))
-          return {V, Offset};
+             SE.LI.getLoopFor(EntInst->getParent())->contains(InsertPt))) {
+          if (!SCEVLostPoisonFlags(S, V))
+            return { V, Offset };
+          // If SCEV lost poison flag we try to find better V if we can.
+          // if we fail to do so then we will strip the poison flags from V.
+          // Ignore current if we already have a candidate.
+          if (!Candidate.first)
+            Candidate = { V, Offset };
+        }
       }
     }
   }
+  if (Candidate.first) {
+    // We have a candidate and failed to find a better candidate we should
+    // strip poison flags from it due to SCEV does not have ones.
+    cast<Instruction>(Candidate.first)->dropPoisonGeneratingFlags();
+    return Candidate;
+  }
   return {nullptr, nullptr};
 }
 
Index: lib/Analysis/ScalarEvolution.cpp
===================================================================
--- lib/Analysis/ScalarEvolution.cpp
+++ lib/Analysis/ScalarEvolution.cpp
@@ -3774,24 +3774,6 @@
   }
 }
 
-/// Check whether value has nuw/nsw/exact set but SCEV does not.
-/// TODO: In reality it is better to check the poison recursevely
-/// but this is better than nothing.
-static bool SCEVLostPoisonFlags(const SCEV *S, const Value *V) {
-  if (auto *I = dyn_cast<Instruction>(V)) {
-    if (isa<OverflowingBinaryOperator>(I)) {
-      if (auto *NS = dyn_cast<SCEVNAryExpr>(S)) {
-        if (I->hasNoSignedWrap() && !NS->hasNoSignedWrap())
-          return true;
-        if (I->hasNoUnsignedWrap() && !NS->hasNoUnsignedWrap())
-          return true;
-      }
-    } else if (isa<PossiblyExactOperator>(I) && I->isExact())
-      return true;
-  }
-  return false;
-}
-
 /// Return an existing SCEV if it exists, otherwise analyze the expression and
 /// create a new one.
 const SCEV *ScalarEvolution::getSCEV(Value *V) {
@@ -3805,7 +3787,7 @@
     // ValueExprMap before insert S->{V, 0} into ExprValueMap.
     std::pair<ValueExprMapType::iterator, bool> Pair =
         ValueExprMap.insert({SCEVCallbackVH(V, this), S});
-    if (Pair.second && !SCEVLostPoisonFlags(S, V)) {
+    if (Pair.second) {
       ExprValueMap[S].insert({V, nullptr});
 
       // If S == Stripped + Offset, add Stripped -> {V, Offset} into


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D42290.130582.patch
Type: text/x-patch
Size: 3901 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180119/14589be4/attachment.bin>