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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

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

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


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

https://reviews.llvm.org/D109821