[PATCH] D36395: [InstCombine] narrow rotate left/right patterns to eliminate zext/trunc (PR34046)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D36395#834263, @efriedma wrote:

> Also, on x86, it looks like we're missing an important pattern here: we generate the sequence `andb $15, %cl; rolw %cl, %ax`.


Yes, I saw that too. This was noted back in:
https://bugs.llvm.org/show_bug.cgi?id=17332#c6
...so we likely need to add to the tablegen pattern-matching.



================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:468
+  Value *X;
+  if (!match(ShVal, m_ZExt(m_Value(X))) || X->getType() != DestTy)
+    return nullptr;
----------------
efriedma wrote:
> Would it make sense to use known bits here rather than explicitly checking for zext?  This matters because we tend to transform `zext(trunc(x)) -> x & c`.
Yes, that would be a general more solution.


================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:492
+  // pattern does not include this guarantee, we could still do the transform,
+  // but we would need to generate this mask for the shift amount.
+  unsigned WideBitWidth = ShAmt->getType()->getScalarSizeInBits();
----------------
efriedma wrote:
> It seems like generating the mask is worth doing here, if we think it'll be eliminated by later lowering.
OK - I wasn't sure if the extra instruction would make this transform less desirable. But yes, if we add the mask ourselves, then I think we'll be able to handle more sloppy versions of the incoming code. I'll add more tests.


https://reviews.llvm.org/D36395