[PATCH] D141850: [SCEV] Preserve divisibility and min/max information in applyLoopGuards

[Joshua Cao via Phabricator via llvm-commits]

caojoshua added a comment.

In D141850#4064581 <https://reviews.llvm.org/D141850#4064581>, @alonkom wrote:

> In D141850#4064477 <https://reviews.llvm.org/D141850#4064477>, @caojoshua wrote:
>
>> The improved trip multiples from the test results look good. Ordering in applyLoopGuards is an issue. However, I think we can simplify this down a bit. What if we always applied min/max first, before we apply divisibility guards? For example, given:
>>
>>   __builtin_assume(TC % 8 == 0);
>>   __builtin_assume(TC > 0);
>>   __builtin_assume(TC < 100);
>>
>>
>>
>> 1. apply max: `umax(1, TC)`
>> 2. apply min: `umin(100, umax(1, TC))`
>> 3. apply divisibility info: `8 * (umin(100, umax(1, TC))) / 8`
>>
>> This makes divisibility info obvious. And traversing the SCEV, we can still see `TC > 0` and `TC < 100`. I believe that if we always apply max/min first, we will never lose this info. This approach seems much simpler and easier to understand.
>>
>> FYI. I haven't been around that long, and this change is non-trivial enough where I prefer a longstanding developer to give the final approval. I will still be around to give my thoughts.
>
> Thanks for the reply.
> This works in this example, but what would happen if we had only these assumes:
>
>   __builtin_assume(TC % 8 == 0);
>   __builtin_assume(TC > 0);
>
> In that case, we would have created the following SCEV:
>
>   8 * (umax(1, TC) / 8)
>
> since umax(1, TC) may still be between [1,7], when we divide it by 8, and multiply by 8, we get 0. So this doesn't preserve the fact that TC > 0.
> In order to do that we must align up 1 to 8, and then umax(8, TC) / 8 is always > 0.

`umax(1, TC)` implies TC <= 1, which holds in this case.

I think what you meant is in the case that, TC > 0, we get: `8 * (umin(1, TC)) / 8`. If TC is in [0, 7], the expression evaluates to 0, and TC > 0 is lost.

Your point comes across that we can't just apply guards in a certain order. I have some thoughts, but I'll think about it and write it down later.


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

https://reviews.llvm.org/D141850