[PATCH] D126617: [InstCombine] Optimize shl+lshr+and conversion pattern

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1892-1894
+    if (C->isPowerOf2() &&
+        match(Op0,
+              m_OneUse(m_LShr(m_Shl(m_APInt(C1), m_Value(X)), m_APInt(C2)))) &&
----------------
bcl5980 wrote:
> spatel wrote:
> > What happens if we reduce the pattern to:
> > https://alive2.llvm.org/ce/z/7snGRd
> > 
> > That's the same transform that I suggested in D126591, but invert the shift direction (lshr instead of shl).
> The latest version is based on your suggetion:
> https://alive2.llvm.org/ce/z/7snGRd
> https://alive2.llvm.org/ce/z/jA_tNb
> 
> I'm still worry if we can transform shift+and to cmp+select.
> Generally most highend cpu should prefer shift+and because the cmp instruction ports is less than shift/and.
> But in cmp instruction the immediate value can be imm operation but shift may need extra mov instruction on some mainstream backend.
> One other question is this transform can fix the case @shl_lshr_pow2_const.
> Can you help to review which way should I do ?
Codegen for any particular target is not the main concern here. The backend should be able to invert the transforms that we make here if that is profitable. 

We partially demonstrated that with the assembly examples in the other patch. You can try similar experiments for these patterns.

I looked at `@shl_lshr_pow2_const` for a while, and I don't see a very good generalization. We can add the larger pattern match for `and(shift(shift))`, or we can treat that as a special-case of demanding one bit only. 

If we view it as another demanded bits problem, then we could improve something like this:
https://alive2.llvm.org/ce/z/3oDagP
(but I don't have any evidence of that being important)


================
Comment at: llvm/test/Transforms/InstCombine/and.ll:1625
 
 define i16 @shl_lshr_pow2_const(i16 %x) {
 ; CHECK-LABEL: @shl_lshr_pow2_const(
----------------
This diff does not exist with the current test on "main", right? Is this review baselined against another patch?


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

https://reviews.llvm.org/D126617