[PATCH] D151796: [SCEV] Skip min/max expressions when normalizing/denormalizing SCEV expressions

Thu Jun 1 03:37:36 PDT 2023

dmakogon added a comment.

In D151796#4384028 <https://reviews.llvm.org/D151796#4384028>, @nikic wrote:

> First time I hear about SCEV normalization/denormalization. The entire approach looks highly dubious.
>
> I'm concerned that this is not a complete fix and there are other cases where normalization/denormalization do not round-trip. What is the criterion that distinguishes min/max from other SCEV expressions? Could something similar affect udiv expressions for example?
>
> I also noticed that there is a check in IVUsers for normalization/denormalization roundtrip at https://github.com/llvm/llvm-project/blob/0a3dc73e700b4a37bc435bf7c02213161b27f54a/llvm/lib/Analysis/IVUsers.cpp#L221-L231.

Thanks for replying!

I agree that this transformation is fishy. There is no clear documentation on it. And I'm not sure whether the round-trip property must hold, but seems like in places where it was used when it was added initially authors assumed it did hold.

I looked at the check you mentioned and at the issue that check is supposed to solve (https://bugs.llvm.org/show_bug.cgi?id=17473).
And it seems to me that it was added just as a quick workaround for this exact issue that sometimes the round-trip property is not preserved.
The test case described in PR17473 has a loop with exactly 127 backedge-taken count and there's a value there with such SCEV: `(sext i8 {1,+,1}<nuw><%for.body> to i32)`.
On the last iteration the addrec overflows and its value is -128, so sext is needed there. Normalizing would subtract 1 from the addrec start value and so it cannot overflow anymore (`{0,+,1}` so `sext` can be safely discarded. If we denormalize, we get just `{1,+,1}`.

So yes, you're right. The current patch doesn't cover all the cases. Clearly we have to prohibit normalization of sext and zext as well.
For udiv I think the following example works. Let `S` be `i32 ({1,+,1}<nuw><%loop> /u -1)` and the loop runs until `{1,+,1}` is equal -1. So it's always zero except for the last iteration when it's value is one. `S_norm = ({0,+,1}<nuw><%loop> /u -1)` and here SCEV doesn't turn into constant zero, but I think it could. And if it did, then obviously denormalization would give a wrong result.

And also turning the whole thing off causes many LSR tests to regress, mainly there are just more IVs left in the code after LSR. There's a considerable amount of code in that pass that depends on normalization, and I think it would take some time to review all such places.

I propose that we state this clearly somewhere that the round-trip property must be preserved and fix the current normalization/denormalization code accordingly (only allow normalization in cases when round-trip is preserved).

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

https://reviews.llvm.org/D151796