[PATCH] D109821: [ScalarEvolution] Infer loop max trip count from array accesses

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

In D109821#3004257 <https://reviews.llvm.org/D109821#3004257>, @nikic wrote:

> In D109821#3004149 <https://reviews.llvm.org/D109821#3004149>, @reames wrote:
>
>> This change as posted has a number of issues, some minor, some major.
>>
>> The core issue I see with this change is that it's making assumptions about out of bounds accesses being immediate UB which are not necessarily true.  Skimming the LangRef for the load instruction, I find nothing that requires an out of bounds load to be immediate UB.  (Returning poison could be legal.)
>
> Out of bounds accesses are immediate UB per our aliasing rules (https://llvm.org/docs/LangRef.html#pointer-aliasing-rules). Basically, if you perform an out of bounds access you are not accessing an address that is associated with the object you are based on, which is UB. (Loads can't be spec'd to return poison, because in the general case, out of bound accesses may trap. Similar reason for why division has immediate UB.)

This appears to contradict the wording describing alignment in the description of load instruction.  That wording appears to say that a *partially* out of bound access is well defined if the access is sufficiently aligned.

To be clear here, I think the entirely out of bounds case *should* be immediate UB.  My point is that the langref is not clear about that, and we need to make sure we're building on a solid foundation here.

>> Second, I recommend we go back to something closer to the original patch and work incrementally.  Specifically, I recommend that we do *not* make the suggested getObjectSize change and integration into general SCEV trip count logic.  Both of those can come later.  I see strong value in minimalism to a) allow easier review and b) minimize fallout.  Once checked in and enabled, doing each of those enhancement seems well justified.
>
> Depending on what specifically you have in mind here, I'd like to push back on that. I think it's important that if we introduce something like this, it's not going to be a separate API that one LoopIdiom transform uses for a profitability heuristic (what was initially proposed here). That's a good way to make sure the code receives approximately zero practical testing. I'm okay with landing this behind a flag to start with, but I think it's important that it does integrate with normal trip count calculation.

There are many reasonable paths here.  What I was thinking was 1) land the new API as proposed in this patch, not used by SCEV otherwise, but with appropriate tests, 2a) use in loop idiom, and 2b) integrate logic into primary trip count logic, and 3) remove dedicated logic from loop idiom in favor of generic interface.  (2a and 2b are concurrent.)  Key point is that I want to be able to iterate on properly tested code when we integrate into the main trip count logic, and not block the motivating use behind non-trivial SCEV work.  I'll comment that I'd like to see this, and am willing to do some of that SCEV integration work myself once the langref stuff has been nailed down.


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

https://reviews.llvm.org/D109821