[PATCH] D24651: [SCEV] Try harder to find UB for NSW/NUW instr

[Sanjoy Das via llvm-commits]

sanjoy added a subscriber: chandlerc.
sanjoy added a comment.

> I've noticed the discussion of Sanjoy on llvm-dev that tries to address the defect I attempt to fix here in a different way.


+1 to proceeding with this in the mean time.  What I started on the mailing list is more of a long term solution and is still very speculative at this point.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4857
@@ +4856,3 @@
+          // guaranteed to be executed if I is executed we analyse
+          if (L && isGuaranteedExecutionPath(PDT, I, useI))
+              Loops.insert(L);
----------------
If you also check that `useI` is a `TerminatorInst` then you'll keep the `SmallPtrSet` smaller.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4862
@@ +4861,3 @@
+        if (isAlwaysLatchControlDependentOnPoison(I, L))
+            LatchControlDependentOnPoison = true;
+      if (!LatchControlDependentOnPoison) {
----------------
Use `llvm::any_of`.  That will also take care of the early exit (that is, you don't need to continue once `LatchControlDependentOnPoison` is `true`.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4943
@@ -4902,3 +4942,3 @@
   // If the add recurrence is poison in any iteration, it is poison on all
   // future iterations (since incrementing poison yields poison). If the result
   // of the add recurrence is fed into the loop latch condition and the loop
----------------
These comments need to be fixed, since IIUC `I` is no longer necessarily an add recurrence in `L`.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4995
@@ +4994,3 @@
+          for (unsigned i = 0; i < PHI->getNumIncomingValues(); i++) {
+            auto *InValue = PHI->getIncomingValue(i);
+            auto *InBB = PHI->getIncomingBlock(i);
----------------
What about:

```
loop:
  %iv = phi i32 [ %poison_inst, %entry ], [ 500; %loop ]
  ...
  br (%iv < ...), label %loop, label %leave
```

?

Since `%iv` is only poison on the first iteration the proof at the
start of the first function ("If the add recurrence is poison in any
iteration, it is poison on all future iterations (since incrementing
poison yields poison") doesn't hold.

================
Comment at: lib/Analysis/ValueTracking.cpp:3603
@@ +3602,3 @@
+    const BasicBlock *BB  = B->getParent();
+    auto *node = PDT.getNode(const_cast<BasicBlock *>(BB));
+
----------------
FYI: you need to follow LLVM coding style here.

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:939
@@ -937,1 +938,3 @@
   AU.addPreserved<DominatorTreeWrapperPass>();
+  AU.addRequired<PostDominatorTreeWrapperPass>();
+  //TODO: not all loop passes preserve the PostDominatorTree (LoopDeletion, ...)
----------------
I don't think a loop-pass can not preserve a function pass that it requires.  You should get this part reviewed by @chandlerc or @mzolotukhin.

I'm also fairly apprehensive of the compile time impact of computing the post-dom tree since it really isn't used in many other places at this time.  At the very least, the burden of proof is on you to show that this patch does not regress compile time.


Repository:
  rL LLVM

https://reviews.llvm.org/D24651