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

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

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

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85969