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

Sat Sep 18 02:10:29 PDT 2021

Peakulorain 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.)  And partially out of bounds accesses are specifically mentioned as legal in the alignment discussion.  At a minimum, that means there's an off by one in the max term.  Before we can move forward with this change, we need to discuss the desired semantics and clarify the langref.  Keep in mind that there's a major gap between "we can prove this safe to access" and "we can prove this will fault on access".
>
> On the more minor side, a couple of issues worth noting.
>
> First, the easy stuff.  The code mixes units resulting in incorrect results.  The code doesn't account for values larger than 64 bits.
>
> 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.

Thank you so much for your comments. I do really agree with that we should work incrementally. So I limited this patch in detecting stack array.
And I know we should prove that  "Out bound of memory accesses in loop will be fault" .  As your remind,  “Returning poison could be legal” while an out of bound LOAD was triggered,  I have something unclear about how does runtime environment handle such a "Poison Load".  Take a step back, if "Out of Bound Load" is to be legal,  but "Out of Bound Storage" is to be UB, I think. Could we prove that "Out bound of memory storage in loop will be fault" ? If we can, I will only do such an inferrer under MEMORY STORAGE.
Thanks again for your valuable comments, minor issues were fixed.  Looking forward to your reply.

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

https://reviews.llvm.org/D109821