[PATCH] D60935: [IndVarSimplify] Fixup nowrap flags during LFTR when moving to post-inc (PR31181)

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

(Separating responses into two threads since these are largely separate points now.)

In D60935#1520604 <https://reviews.llvm.org/D60935#1520604>, @nikic wrote:

> In D60935#1520163 <https://reviews.llvm.org/D60935#1520163>, @reames wrote:
>
> > The third is the dead IV case mentioned in patch comments.  Good catch, I'd missed that one until rereading the patch again.  Nikita, can you list a specific test case which exercises this?  I'd have thought that hasConcreteDef case would have caught this?
>
>
> These are the switch_to_different_iv_pos_inc and switch_to_different_iv_pre_inc test cases. I'm assuming you mean AlmostDeadIV here rather than hasConcreteDef. AlmostDeadIV doesn't trigger here because the IV is only dynamically dead, it has static uses.


Ah, yes.  I see your point.   We're still adding a branch on a value which previously could have been poison without triggering UB.  Dang, that's a much more general problem than the post-increment one I'd been mostly focused on.

So, how about this?  What if we implemented the following:

1. When changing IVs, always clear the flags.  All of the other options (selective clearing, changing hasConcreteDef, bailing out) appear difficult to implement.  If we can get away without doing so performance wise, we should.
2. When reusing the same IV, only switch to post increment if we can do so without clearing flags.



================
Comment at: llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:2388
+  Value *IncVar = IndVar->getIncomingValueForBlock(L->getLoopLatch());
+  if (auto *BO = dyn_cast<BinaryOperator>(IncVar)) {
+    const SCEVAddRecExpr *AR = cast<SCEVAddRecExpr>(SE->getSCEV(IncVar));
----------------
nikic wrote:
> reames wrote:
> > This test can never fail given the definition of LoopCounter used.
> This can also be a GEP.
You are correct... Though, does that mean we have the same problem with inbounds?  I think it may actually.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60935/new/

https://reviews.llvm.org/D60935