[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.

(Response part 2)

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

> In D60935#1520163 <https://reviews.llvm.org/D60935#1520163>, @reames wrote:
>
> > At the moment, I believe the patch posted is correct, but is unnecessarily conservative.  Instead of only stripping nsw/nuw where necessary, it *always* strips.  A simple, but noteworthy improvement would be to use SCEV's getSignedRange/getUnsignedRange to ensure the increment doesn't overflow and then only stripping on cases where it does.
>
>
> No, this patch only strips where necessary. It uses the nowrap flag computed by SCEV on the post-inc addrec, which are determined through a number of ways, including both range analysis, as well as nowrap flags on the original IR in conjunction with poison based reasoning. From my experiments this analysis seems to be pretty good, which is also why there is no fallout on existing tests.
>
> See test_no_drop_nuw and test_no_drop_nsw for cases where flags are not dropped based on range analysis in particular, despite a change to post-inc IV.


Yeah, you're right I simply misread the code.  You are using the flags off the AR.

At first, I thought your placement was problematic (since it might use cached information), but after further digging, I've convinced myself that SCEV would return a result which must be valid in this case.  SCEV appears to only set flags if those flags would be valid for *all locations* within the loop.  Given we haven't inserted the new use yet (the compare), the analysis can't go wrong based on the presence of the nsw/nuw bits either.  So, I think what you have here is actually 100% correct.  :)

Hm, elsewhere in this thread, I'd encouraged you not to do the pre-to-post conversion if it causes us to drop flags.  I think I'm going to drop that request given the further complexities involved in changing IVs.  Let's go with the simple scheme (yours of dropping flags with re-inference) and only come back to the more complicated ones if performance shows us we need to.  Let's just focus on getting a correct bug fix checked in for the moment.

Given that change in strategy, we only have two open items before I can LGTM this.  1) is the GEP/inbounds question and 2) the TODO question about hasConcreteDef.



================
Comment at: llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:2383
+  // instruction, while SCEV has to explicitly prove the post-inc nowrap flags.
+  // TODO: This handling may be inaccurate for one case: If we switch to a
+  // dynamically dead IV that wraps on the first loop iteration only, which is
----------------
I don't think this is possible given hasConcreteDef.  It ensures the value on the first iteration is non-poison.  


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

https://reviews.llvm.org/D60935