[llvm] 3c77eb3 - [DAGCombiner] Only fold shift+mask compare to rotate if the shift amount divides the bit width (#220743)
via llvm-commits
llvm-commits at lists.llvm.org
Sun Sep 13 05:02:40 PDT 2026
Author: Stanislav Bardyuk
Date: 2026-09-13T12:02:35Z
New Revision: 3c77eb3a3f2793a75e0ce86d96076637d5989bea
URL: https://github.com/llvm/llvm-project/commit/3c77eb3a3f2793a75e0ce86d96076637d5989bea
DIFF: https://github.com/llvm/llvm-project/commit/3c77eb3a3f2793a75e0ce86d96076637d5989bea.diff
LOG: [DAGCombiner] Only fold shift+mask compare to rotate if the shift amount divides the bit width (#220743)
DAGCombiner::visitSETCC can turn `(X << S) == (X & -(1 << S))` (or the
srl variant) into `rotl(X, S) == X`, and the other way around, when the
target prefers one form over the other. The two forms are only equivalent when
S divides the bit width. The check used `isPowerOf2(S)`, which is the same
thing for i8/i16/i32/i64 but not for odd widths like i3, where S = 2 is a
power of two that does not divide 3. On x86, which prefers the rotate,
the reporter's i3 function returned the wrong value for two of its eight
inputs.
This changes the `MayTransformRotate` flag passed to
`preferedOpcodeForCmpEqPiecesOfOperand` to check `NumBits % S == 0`
(with S != 0) and fixes the two comments that described the old rule. For legal
scalar types nothing changes. On illegal non power of two widths this
also lets a non power of two amount that divides the width use the rotate
(like i6 with S = 3); that is correct for the same reason and legalizes
through the usual rotate promotion, same as i6 with S = 2 did before. The x86
hook already honors the flag in both directions, so the fix lives in the
target independent code.
Tests: the i3 shl and lshr cases from the issue (must stay shift+mask), an
i6 / S = 3 case (3 divides 6, so the rotate is still allowed), and the
reporter's function.
Fixes #220542
Added:
Modified:
llvm/include/llvm/CodeGen/TargetLowering.h
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
llvm/lib/Target/X86/X86ISelLowering.cpp
llvm/test/CodeGen/X86/cmp-shiftX-maskX.ll
Removed:
################################################################################
diff --git a/llvm/include/llvm/CodeGen/TargetLowering.h b/llvm/include/llvm/CodeGen/TargetLowering.h
index 69f5e0e4e3011..4006ad3bda5d6 100644
--- a/llvm/include/llvm/CodeGen/TargetLowering.h
+++ b/llvm/include/llvm/CodeGen/TargetLowering.h
@@ -929,13 +929,13 @@ class LLVM_ABI TargetLoweringBase {
// Given:
// (icmp eq/ne (and X, C0), (shift X, C1))
// or
- // (icmp eq/ne X, (rotate X, CPow2))
+ // (icmp eq/ne X, (rotate X, C1))
// If C0 is a mask or shifted mask and the shift amt (C1) isolates the
// remaining bits (i.e something like `(x64 & UINT32_MAX) == (x64 >> 32)`)
// Do we prefer the shift to be shift-right, shift-left, or rotate.
- // Note: Its only valid to convert the rotate version to the shift version iff
- // the shift-amt (`C1`) is a power of 2 (including 0).
+ // Note: It's only valid to convert between the rotate and shift versions iff
+ // the shift-amt (`C1`) divides the bit width.
// If ShiftOpc (current Opcode) is returned, do nothing.
virtual unsigned preferedOpcodeForCmpEqPiecesOfOperand(
EVT VT, unsigned ShiftOpc, bool MayTransformRotate,
diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index ca292fc81afa6..0e69d7f118478 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -14980,8 +14980,8 @@ SDValue DAGCombiner::visitSETCC(SDNode *N) {
// If C0 is a mask or shifted mask and the shift amt (C1) isolates the
// remaining bits (i.e something like `(x64 & UINT32_MAX) == (x64 >> 32)`)
// Then:
- // If C1 is a power of 2, then the rotate and shift+and versions are
- // equivilent, so we can interchange them depending on target preference.
+ // If C1 divides the bit width, then the rotate and shift+and versions are
+ // equivalent, so we can interchange them depending on target preference.
// Otherwise, if we have the shift+and version we can interchange srl/shl
// which inturn affects the constant C0. We can use this to get better
// constants again determined by target preference.
@@ -15048,9 +15048,13 @@ SDValue DAGCombiner::visitSETCC(SDNode *N) {
ShiftOpc == ISD::SHL ? (~*AndCMask).isMask() : AndCMask->isMask();
}
+ // The rotate and shift+and forms are only equivalent if the shift
+ // amount divides the bit width.
+ bool MayTransformRotate =
+ !ShiftCAmt->isZero() && NumBits % ShiftCAmt->getZExtValue() == 0;
// See if target prefers another shift/rotate opcode.
unsigned NewShiftOpc = TLI.preferedOpcodeForCmpEqPiecesOfOperand(
- OpVT, ShiftOpc, ShiftCAmt->isPowerOf2(), *ShiftCAmt, AndCMask);
+ OpVT, ShiftOpc, MayTransformRotate, *ShiftCAmt, AndCMask);
// Transform is valid and we have a new preference.
if (CanTransform && NewShiftOpc != ShiftOpc) {
SDValue NewShiftOrRotate =
diff --git a/llvm/lib/Target/X86/X86ISelLowering.cpp b/llvm/lib/Target/X86/X86ISelLowering.cpp
index 11dbec24adb33..69eab53dca965 100644
--- a/llvm/lib/Target/X86/X86ISelLowering.cpp
+++ b/llvm/lib/Target/X86/X86ISelLowering.cpp
@@ -3836,9 +3836,10 @@ unsigned X86TargetLowering::preferedOpcodeForCmpEqPiecesOfOperand(
// best. Otherwise its not clear what the best so just don't make changed.
PreferRotate = Subtarget.hasAVX512() && (VT.getScalarType() == MVT::i32 ||
VT.getScalarType() == MVT::i64);
- } else {
+ } else if (isTypeLegal(VT)) {
// For scalar, if we have bmi prefer rotate for rorx. Otherwise prefer
- // rotate unless we have a zext mask+shr.
+ // rotate unless we have a zext mask+shr. Rotates on illegal types are
+ // expanded to shifts, so never prefer them there.
PreferRotate = Subtarget.hasBMI2();
if (!PreferRotate) {
unsigned MaskBits =
diff --git a/llvm/test/CodeGen/X86/cmp-shiftX-maskX.ll b/llvm/test/CodeGen/X86/cmp-shiftX-maskX.ll
index 227de9ad0ab69..9e329648a6cf0 100644
--- a/llvm/test/CodeGen/X86/cmp-shiftX-maskX.ll
+++ b/llvm/test/CodeGen/X86/cmp-shiftX-maskX.ll
@@ -1017,6 +1017,78 @@ define i32 @issue108722(i32 %0) {
ret i32 %4
}
+; The rotate form is only equivalent when the shift amount divides the bit
+; width; 2 does not divide 3, so these must keep the shift+mask form.
+define i1 @shl_to_rotl_eq_i3_s2_fail(i3 %x) {
+; CHECK-LABEL: shl_to_rotl_eq_i3_s2_fail:
+; CHECK: # %bb.0:
+; CHECK-NEXT: # kill: def $edi killed $edi def $rdi
+; CHECK-NEXT: leal (,%rdi,4), %eax
+; CHECK-NEXT: andb $4, %al
+; CHECK-NEXT: andb $4, %dil
+; CHECK-NEXT: cmpb %dil, %al
+; CHECK-NEXT: sete %al
+; CHECK-NEXT: retq
+ %shl = shl i3 %x, 2
+ %and = and i3 %x, -4
+ %r = icmp eq i3 %shl, %and
+ ret i1 %r
+}
+
+define i1 @shr_to_rotl_eq_i3_s2_fail(i3 %x) {
+; CHECK-LABEL: shr_to_rotl_eq_i3_s2_fail:
+; CHECK: # %bb.0:
+; CHECK-NEXT: movl %edi, %eax
+; CHECK-NEXT: andb $4, %al
+; CHECK-NEXT: shlb $2, %dil
+; CHECK-NEXT: andb $4, %dil
+; CHECK-NEXT: cmpb %dil, %al
+; CHECK-NEXT: sete %al
+; CHECK-NEXT: retq
+ %shr = lshr i3 %x, 2
+ %and = and i3 %x, 1
+ %r = icmp eq i3 %shr, %and
+ ret i1 %r
+}
+
+; 3 divides 6, so the rotate form would be valid here, but i6 has no legal
+; rotate (it would be expanded back to shifts), so keep the shift+mask form.
+define i1 @shr_to_rotl_eq_i6_s3(i6 %x) {
+; CHECK-LABEL: shr_to_rotl_eq_i6_s3:
+; CHECK: # %bb.0:
+; CHECK-NEXT: movl %edi, %eax
+; CHECK-NEXT: andb $56, %al
+; CHECK-NEXT: shlb $3, %dil
+; CHECK-NEXT: andb $56, %dil
+; CHECK-NEXT: cmpb %dil, %al
+; CHECK-NEXT: sete %al
+; CHECK-NEXT: retq
+ %shr = lshr i6 %x, 3
+ %and = and i6 %x, 7
+ %r = icmp eq i6 %shr, %and
+ ret i1 %r
+}
+
+define i64 @pr220542(i64 %0) {
+; CHECK-LABEL: pr220542:
+; CHECK: # %bb.0:
+; CHECK-NEXT: shrl $8, %edi
+; CHECK-NEXT: leal (,%rdi,4), %eax
+; CHECK-NEXT: andb $4, %al
+; CHECK-NEXT: andb $4, %dil
+; CHECK-NEXT: xorl %ecx, %ecx
+; CHECK-NEXT: cmpb %dil, %al
+; CHECK-NEXT: setne %cl
+; CHECK-NEXT: leaq 1(%rcx,%rcx), %rax
+; CHECK-NEXT: retq
+ %2 = lshr i64 %0, 8
+ %3 = trunc i64 %2 to i3
+ %4 = shl i3 %3, 2
+ %5 = and i3 %3, -4
+ %6 = icmp eq i3 %4, %5
+ %7 = select i1 %6, i64 1, i64 3
+ ret i64 %7
+}
;; NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line:
; CHECK-AVX: {{.*}}
More information about the llvm-commits
mailing list