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

[Bin Cheng via Phabricator via llvm-commits]

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

Hi reames, thanks very much for commenting.

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

Yes, the motivation comes from C/C++ UB where pointer shall not pass the element next to the last element, and if points the one pass the last element, it shall not be dereferenced.
And I understand that LLVM IR LangRef is totally a different language to C/C++.

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

Shall the langref be clarified (or even strengthened) here?  Based on below reasons:
1\ Most programming languages have this behavior as UB, is it better to define IR in line with supported languages?
2\ Since poison value is described under constant category in langref.  It's hard to return poison value for the load instruction (in loop).  I can only think of an explicit runtime boundary check and return poison value if the check fails.
3\ not sure IIUC, poison value is introduced for speculation of possible UB.  Looks to me the computation of invalid(out of bound, and let's ignore the one pass last element issue) pointer is the point where UB should be speculated, while memory reference using the invalid pointer is the point UB should be triggered immediately.  Which is better in line with C/C++'s description?

> And partially out of bounds accesses are specifically mentioned as legal in the alignment discussion.

This can be addressed by checking alignment and the size of load type.

> At a minimum, that means there's an off by one in the max term.

With over-alignment issue addressed, this won't be issue anymore?

Furthermore, this can be further restricted by only checking store instructions, which is enough for memset/memcpy loop idiom transformation.

> Before we can move forward with this change, we need to discuss the desired semantics and clarify the langref.

I totally agree that langref should be discussed in order that agreement can be reached.  On the other hand, review of langref could take quite long time, shall we improve the current patch by putting more restrictions (such as I mentioned above), and by addressing other bugs.  Then land the patch.

Thanks again for your suggestions.

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




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

https://reviews.llvm.org/D109821