[PATCH] D109283: [InstCombine] ror/rol(X, RotAmt) == C --> X == rol/ror(C, RotAmt) (PR51567)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3238
+
+      const APInt *RotAmt;
+      // ror(X, RotAmt) == C --> X == rol(C, RotAmt)
----------------
Calling this "RotC" or "RotAmtC" makes it more obvious that we are matching a constant.


================
Comment at: llvm/test/Transforms/InstCombine/icmp-rotate.ll:115
+;
+  %f = tail call i8 @llvm.fshl.i8(i8 %x, i8 %x, i8 28)
+  %r = icmp eq i8 %f, 2
----------------
The over-shifting isn't relevant - please reduce to a valid amount (for example 28 -> 4) in this test and others.
But don't use "4" with a bitwidth of 8 either because that makes the test ambiguous about whether it is rotating in the wrong direction.


================
Comment at: llvm/test/Transforms/InstCombine/icmp-rotate.ll:150
+
+define <2 x i1> @rol_eq_cst_undef(<2 x i5> %x) {
+; CHECK-LABEL: @rol_eq_cst_undef(
----------------
xbolva00 wrote:
> Enough tests?
We're missing a fshl with ne. Also at least one extra-use test should be here.
Please pre-commit baseline tests.


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

https://reviews.llvm.org/D109283