[PATCH] D85969: [SCEV] Model (xor (shl x, C), (-1 << C)) as (shl (xor x, -1), C)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D85969#2219937 <https://reviews.llvm.org/D85969#2219937>, @bjope wrote:

> In D85969#2218048 <https://reviews.llvm.org/D85969#2218048>, @spatel wrote:
>
>> I don't have much experience with SCEV either, but I think your draft change for instcombine made sense:
>> https://bugs.llvm.org/show_bug.cgi?id=47136#c8
>>
>> As hinted there, that's the same direction as:
>> D32255 <https://reviews.llvm.org/D32255> / rG23bd33c6acc4 <https://reviews.llvm.org/rG23bd33c6acc4fa0ddc097d3d0767860cc014f6e0>
>>
>> So I'm not sure if we would consider this a worthwhile improvement independently and still make a change to SCEV or only change instcombine (or if there's some reason to *not* change instcombine, I'd like to understand that).
>
> Well, I'm not exactly sure why instcombine is folding the shift over the operands of binary operators in the first place. Afaict it does that regardless of it being beneficial or not (e.g. triggering more combines). So maybe it is some kind of canonicalization?

Yes, it looks like it is just a canonicalization, so we standardize on a single pattern. I don't think there's anything inherently better/worse about one form or the other. If we reverse the canonicalization, we may find regressions because we have come to expect the current patterns.

> If it is a canonicalization it seem weird to be assymetric and do different things depending on the value of the constant operand (and which binary operator that is used), isn't it?

I agree with that in general, but 'not' is treated as a special-case in instcombine and other passes, so there's precedence to do that for this case too.

> If we decide that the canoncial form of `(xor (shl x, C), (-1 << C))` should be `(shl (xor x, -1), C)`, then I figure we need to inplement that reverse transform instead of the one present in instcombine today (specially considering that ScalarEvolution only undestand `(shl (xor x, -1), C)` without this patch).

Yes, we should implement the reverse transform in instcombine to make things canonical - even if that form is not entirely consistent. :)

> So all-in-all, I figured that patching ScalarEvolution would be a more general solution (being a bit more independent on what instcombine and other passes are doing). However, it still depends on the shift being present directly as the xor operand, so it is still not perfect in that sense.
>
> One more thing. I wrote PR47136 because of regressions in three test cases. This patch restore things to the old cycle counts. When testing the other patch from PR47136, simply avoiding some shift folding in instcombine, it also resolved the three regressions. But with the instcombine patch I got another regressions instead. So it seemed like that solution had other implications.

Would that be resolved by adding the reverse canonicalization?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85969