[llvm] [X86] combineAndLoadToBZHI - fix miscompiles (PR #221025)
Chris Kennelly via llvm-commits
llvm-commits at lists.llvm.org
Tue Sep 22 07:49:29 PDT 2026
https://github.com/ckennelly updated https://github.com/llvm/llvm-project/pull/221025
>From 98bcb19f254c48e736e20edfabc691fc7251955f Mon Sep 17 00:00:00 2001
From: Chris Kennelly <ckennelly at ckennelly.com>
Date: Thu, 10 Sep 2026 04:45:30 +0000
Subject: [PATCH] [X86] Remove combineAndLoadToBZHI
The low-bits mask table recognition it performed is now done in the
target-independent AggressiveInstCombine (394c8dd14933), which rewrites
x & tbl[i] to x & ((1 << i) - 1); the X86 backend already lowers that
arithmetic to bzhi via matchBitExtract. Removing the DAG fold also
retires its unchecked matching of scaled indices, folded constant
offsets and extending loads, which could miscompile under the static
relocation model.
replace-load-and-with-bzhi.ll is repurposed as an end-to-end pipeline
test (opt aggressive-instcombine,instcombine | llc) covering the cases
the DAG fold handled, PIC, which it could not handle, and the index
shapes it miscompiled.
Assisted-by: Claude Code
---
llvm/lib/Target/X86/X86ISelLowering.cpp | 105 -----------
.../CodeGen/X86/replace-load-and-with-bzhi.ll | 165 +++++++++++++++++-
2 files changed, 159 insertions(+), 111 deletions(-)
diff --git a/llvm/lib/Target/X86/X86ISelLowering.cpp b/llvm/lib/Target/X86/X86ISelLowering.cpp
index c8c58a310604d..6adaad52aaf0d 100644
--- a/llvm/lib/Target/X86/X86ISelLowering.cpp
+++ b/llvm/lib/Target/X86/X86ISelLowering.cpp
@@ -52876,28 +52876,6 @@ static SDValue combineAndMaskToShift(SDNode *N, const SDLoc &DL,
return DAG.getBitcast(N->getValueType(0), Shift);
}
-// Get the index node from the lowered DAG of a GEP IR instruction with one
-// indexing dimension.
-static SDValue getIndexFromUnindexedLoad(LoadSDNode *Ld) {
- if (Ld->isIndexed())
- return SDValue();
-
- SDValue Base = Ld->getBasePtr();
- if (Base.getOpcode() != ISD::ADD)
- return SDValue();
-
- SDValue ShiftedIndex = Base.getOperand(0);
- if (ShiftedIndex.getOpcode() != ISD::SHL)
- return SDValue();
-
- return ShiftedIndex.getOperand(0);
-}
-
-static bool hasBZHI(const X86Subtarget &Subtarget, MVT VT) {
- return Subtarget.hasBMI2() &&
- (VT == MVT::i32 || (VT == MVT::i64 && Subtarget.is64Bit()));
-}
-
/// Folds (and X, (or Y, ~Z)) --> (and X, ~(and ~Y, Z))
/// This undoes the inverse fold performed in InstCombine
static SDValue combineAndNotOrIntoAndNotAnd(SDNode *N, const SDLoc &DL,
@@ -52957,86 +52935,6 @@ static SDValue combineMaskBitOp(SDNode *N, const SDLoc &DL, SelectionDAG &DAG) {
return SDValue();
}
-// This function recognizes cases where X86 bzhi instruction can replace and
-// 'and-load' sequence.
-// In case of loading integer value from an array of constants which is defined
-// as follows:
-//
-// int array[SIZE] = {0x0, 0x1, 0x3, 0x7, 0xF ..., 2^(SIZE-1) - 1}
-//
-// then applying a bitwise and on the result with another input.
-// It's equivalent to performing bzhi (zero high bits) on the input, with the
-// same index of the load.
-static SDValue combineAndLoadToBZHI(SDNode *Node, SelectionDAG &DAG,
- const X86Subtarget &Subtarget) {
- MVT VT = Node->getSimpleValueType(0);
- SDLoc dl(Node);
-
- // Check if subtarget has BZHI instruction for the node's type
- if (!hasBZHI(Subtarget, VT))
- return SDValue();
-
- // Try matching the pattern for both operands.
- for (unsigned i = 0; i < 2; i++) {
- // continue if the operand is not a load instruction
- auto *Ld = dyn_cast<LoadSDNode>(Node->getOperand(i));
- if (!Ld)
- continue;
- const Value *MemOp = Ld->getMemOperand()->getValue();
- if (!MemOp)
- continue;
- // Get the Node which indexes into the array.
- SDValue Index = getIndexFromUnindexedLoad(Ld);
- if (!Index)
- continue;
-
- if (auto *GEP = dyn_cast<GetElementPtrInst>(MemOp)) {
- if (auto *GV = dyn_cast<GlobalVariable>(GEP->getOperand(0))) {
- if (GV->isConstant() && GV->hasDefinitiveInitializer()) {
- Constant *Init = GV->getInitializer();
- Type *Ty = Init->getType();
- if (!isa<ConstantDataArray>(Init) ||
- !Ty->getArrayElementType()->isIntegerTy() ||
- Ty->getArrayElementType()->getScalarSizeInBits() !=
- VT.getSizeInBits() ||
- Ty->getArrayNumElements() >
- Ty->getArrayElementType()->getScalarSizeInBits())
- continue;
-
- // Check if the array's constant elements are suitable to our case.
- uint64_t ArrayElementCount = Init->getType()->getArrayNumElements();
- bool ConstantsMatch = true;
- for (uint64_t j = 0; j < ArrayElementCount; j++) {
- auto *Elem = cast<ConstantInt>(Init->getAggregateElement(j));
- if (Elem->getZExtValue() != (((uint64_t)1 << j) - 1)) {
- ConstantsMatch = false;
- break;
- }
- }
- if (!ConstantsMatch)
- continue;
-
- // Do the transformation (For 32-bit type):
- // -> (and (load arr[idx]), inp)
- // <- (and (srl 0xFFFFFFFF, (sub 32, idx)))
- // that will be replaced with one bzhi instruction.
- SDValue Inp = Node->getOperand(i == 0 ? 1 : 0);
- SDValue SizeC = DAG.getConstant(VT.getSizeInBits(), dl, MVT::i32);
-
- Index = DAG.getZExtOrTrunc(Index, dl, MVT::i32);
- SDValue Sub = DAG.getNode(ISD::SUB, dl, MVT::i32, SizeC, Index);
- Sub = DAG.getNode(ISD::TRUNCATE, dl, MVT::i8, Sub);
-
- SDValue AllOnes = DAG.getAllOnesConstant(dl, VT);
- SDValue LShr = DAG.getNode(ISD::SRL, dl, VT, AllOnes, Sub);
- return DAG.getNode(ISD::AND, dl, VT, Inp, LShr);
- }
- }
- }
- }
- return SDValue();
-}
-
// Look for (and (bitcast (vXi1 (concat_vectors (vYi1 setcc), undef,))), C)
// Where C is a mask containing the same number of bits as the setcc and
// where the setcc will freely 0 upper bits of k-register. We can replace the
@@ -53479,9 +53377,6 @@ static SDValue combineAnd(SDNode *N, SelectionDAG &DAG,
if (SDValue ShiftRight = combineAndMaskToShift(N, dl, DAG, Subtarget))
return ShiftRight;
- if (SDValue R = combineAndLoadToBZHI(N, DAG, Subtarget))
- return R;
-
if (SDValue R = combineAndNotOrIntoAndNotAnd(N, dl, DAG))
return R;
diff --git a/llvm/test/CodeGen/X86/replace-load-and-with-bzhi.ll b/llvm/test/CodeGen/X86/replace-load-and-with-bzhi.ll
index a0bd35d5d219b..7260442cdd698 100644
--- a/llvm/test/CodeGen/X86/replace-load-and-with-bzhi.ll
+++ b/llvm/test/CodeGen/X86/replace-load-and-with-bzhi.ll
@@ -1,6 +1,14 @@
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+bmi2 | FileCheck %s -check-prefix=X64
-; RUN: llc < %s -mtriple=i686-unknown-unknown -mattr=+bmi2 | FileCheck %s -check-prefix=X86
+; RUN: opt < %s -mtriple=x86_64-unknown-unknown -passes=aggressive-instcombine,instcombine -S | llc -mtriple=x86_64-unknown-unknown -mattr=+bmi2 | FileCheck %s -check-prefixes=X64,X64-STATIC
+; RUN: opt < %s -mtriple=x86_64-unknown-unknown -passes=aggressive-instcombine,instcombine -S | llc -mtriple=x86_64-unknown-unknown -mattr=+bmi2 -relocation-model=pic | FileCheck %s -check-prefixes=X64,X64-PIC
+; RUN: opt < %s -mtriple=i686-unknown-unknown -passes=aggressive-instcombine,instcombine -S | llc -mtriple=i686-unknown-unknown -mattr=+bmi2 | FileCheck %s -check-prefix=X86
+
+; End-to-end test for the "low bits mask" table idiom
+; x & tbl[i] where tbl[j] == (1 << j) - 1
+; AggressiveInstCombine rewrites the table load to (1 << i) - 1, InstCombine
+; canonicalizes that, and the X86 backend selects a single bzhi -- also under
+; PIC, which the former combineAndLoadToBZHI DAG fold could not handle.
+; i64 tables are left alone on i686, where i64 is not a legal integer.
@fill_table32 = internal unnamed_addr constant [32 x i32] [i32 0, i32 1, i32 3, i32 7, i32 15, i32 31, i32 63, i32 127, i32 255, i32 511, i32 1023, i32 2047, i32 4095, i32 8191, i32 16383, i32 32767, i32 65535, i32 131071, i32 262143, i32 524287, i32 1048575, i32 2097151, i32 4194303, i32 8388607, i32 16777215, i32 33554431, i32 67108863, i32 134217727, i32 268435455, i32 536870911, i32 1073741823, i32 2147483647], align 16
@fill_table32_partial = internal unnamed_addr constant [17 x i32] [i32 0, i32 1, i32 3, i32 7, i32 15, i32 31, i32 63, i32 127, i32 255, i32 511, i32 1023, i32 2047, i32 4095, i32 8191, i32 16383, i32 32767, i32 65535], align 16
@@ -15,7 +23,7 @@ define i32 @f32_bzhi(i32 %x, i32 %y) local_unnamed_addr {
;
; X86-LABEL: f32_bzhi:
; X86: # %bb.0: # %entry
-; X86-NEXT: movl {{[0-9]+}}(%esp), %eax
+; X86-NEXT: movzbl {{[0-9]+}}(%esp), %eax
; X86-NEXT: bzhil %eax, {{[0-9]+}}(%esp), %eax
; X86-NEXT: retl
entry:
@@ -34,7 +42,7 @@ define i32 @f32_bzhi_commute(i32 %x, i32 %y) local_unnamed_addr {
;
; X86-LABEL: f32_bzhi_commute:
; X86: # %bb.0: # %entry
-; X86-NEXT: movl {{[0-9]+}}(%esp), %eax
+; X86-NEXT: movzbl {{[0-9]+}}(%esp), %eax
; X86-NEXT: bzhil %eax, {{[0-9]+}}(%esp), %eax
; X86-NEXT: retl
entry:
@@ -53,7 +61,7 @@ define i32 @f32_bzhi_partial(i32 %x, i32 %y) local_unnamed_addr {
;
; X86-LABEL: f32_bzhi_partial:
; X86: # %bb.0: # %entry
-; X86-NEXT: movl {{[0-9]+}}(%esp), %eax
+; X86-NEXT: movzbl {{[0-9]+}}(%esp), %eax
; X86-NEXT: bzhil %eax, {{[0-9]+}}(%esp), %eax
; X86-NEXT: retl
entry:
@@ -72,7 +80,7 @@ define i32 @f32_bzhi_partial_commute(i32 %x, i32 %y) local_unnamed_addr {
;
; X86-LABEL: f32_bzhi_partial_commute:
; X86: # %bb.0: # %entry
-; X86-NEXT: movl {{[0-9]+}}(%esp), %eax
+; X86-NEXT: movzbl {{[0-9]+}}(%esp), %eax
; X86-NEXT: bzhil %eax, {{[0-9]+}}(%esp), %eax
; X86-NEXT: retl
entry:
@@ -166,3 +174,148 @@ entry:
%and = and i64 %x, %0
ret i64 %and
}
+
+; The removed DAG fold also matched the shapes below and got them wrong: it
+; never checked the index scale, accepted extending loads, and ignored a
+; constant offset folded into the global address. None of them is x masked to
+; its low y bits.
+
+; fill_table32[2 * y].
+define i32 @f32_bzhi_wrong_scale(i32 %x, i64 %y) local_unnamed_addr {
+; X64-STATIC-LABEL: f32_bzhi_wrong_scale:
+; X64-STATIC: # %bb.0: # %entry
+; X64-STATIC-NEXT: movl %edi, %eax
+; X64-STATIC-NEXT: andl fill_table32(,%rsi,8), %eax
+; X64-STATIC-NEXT: retq
+;
+; X64-PIC-LABEL: f32_bzhi_wrong_scale:
+; X64-PIC: # %bb.0: # %entry
+; X64-PIC-NEXT: movl %edi, %eax
+; X64-PIC-NEXT: leaq fill_table32(%rip), %rcx
+; X64-PIC-NEXT: andl (%rcx,%rsi,8), %eax
+; X64-PIC-NEXT: retq
+;
+; X86-LABEL: f32_bzhi_wrong_scale:
+; X86: # %bb.0: # %entry
+; X86-NEXT: movl {{[0-9]+}}(%esp), %eax
+; X86-NEXT: movl fill_table32(,%eax,8), %eax
+; X86-NEXT: andl {{[0-9]+}}(%esp), %eax
+; X86-NEXT: retl
+entry:
+ %offset = shl nuw nsw i64 %y, 3
+ %arrayidx = getelementptr inbounds i8, ptr @fill_table32, i64 %offset
+ %0 = load i32, ptr %arrayidx, align 4
+ %and = and i32 %0, %x
+ ret i32 %and
+}
+
+; Only the low half of fill_table32[y].
+define i32 @f32_bzhi_extload(i32 %x, i64 %y) local_unnamed_addr {
+; X64-STATIC-LABEL: f32_bzhi_extload:
+; X64-STATIC: # %bb.0: # %entry
+; X64-STATIC-NEXT: movzwl fill_table32(,%rsi,4), %eax
+; X64-STATIC-NEXT: andl %edi, %eax
+; X64-STATIC-NEXT: retq
+;
+; X64-PIC-LABEL: f32_bzhi_extload:
+; X64-PIC: # %bb.0: # %entry
+; X64-PIC-NEXT: leaq fill_table32(%rip), %rax
+; X64-PIC-NEXT: movzwl (%rax,%rsi,4), %eax
+; X64-PIC-NEXT: andl %edi, %eax
+; X64-PIC-NEXT: retq
+;
+; X86-LABEL: f32_bzhi_extload:
+; X86: # %bb.0: # %entry
+; X86-NEXT: movl {{[0-9]+}}(%esp), %eax
+; X86-NEXT: movzwl fill_table32(,%eax,4), %eax
+; X86-NEXT: andl {{[0-9]+}}(%esp), %eax
+; X86-NEXT: retl
+entry:
+ %offset = shl nuw nsw i64 %y, 2
+ %arrayidx = getelementptr inbounds i8, ptr @fill_table32, i64 %offset
+ %0 = load i16, ptr %arrayidx, align 4
+ %ext = zext i16 %0 to i32
+ %and = and i32 %ext, %x
+ ret i32 %and
+}
+
+; fill_table32[y + 1]: the mask of y + 1 bits.
+define i32 @f32_bzhi_index_add(i32 %x, i32 %y) local_unnamed_addr {
+; X64-LABEL: f32_bzhi_index_add:
+; X64: # %bb.0: # %entry
+; X64-NEXT: incb %sil
+; X64-NEXT: bzhil %esi, %edi, %eax
+; X64-NEXT: retq
+;
+; X86-LABEL: f32_bzhi_index_add:
+; X86: # %bb.0: # %entry
+; X86-NEXT: movzbl {{[0-9]+}}(%esp), %eax
+; X86-NEXT: incb %al
+; X86-NEXT: bzhil %eax, {{[0-9]+}}(%esp), %eax
+; X86-NEXT: retl
+entry:
+ %inc = add nsw i32 %y, 1
+ %idxprom = sext i32 %inc to i64
+ %arrayidx = getelementptr inbounds [32 x i32], ptr @fill_table32, i64 0, i64 %idxprom
+ %0 = load i32, ptr %arrayidx, align 4
+ %and = and i32 %0, %x
+ ret i32 %and
+}
+
+; fill_table32[y + 1] again, with the + 1 as a byte offset.
+define i32 @f32_bzhi_i8_gep_offset(i32 %x, i64 %y) local_unnamed_addr {
+; X64-STATIC-LABEL: f32_bzhi_i8_gep_offset:
+; X64-STATIC: # %bb.0: # %entry
+; X64-STATIC-NEXT: movl %edi, %eax
+; X64-STATIC-NEXT: andl fill_table32+4(,%rsi,4), %eax
+; X64-STATIC-NEXT: retq
+;
+; X64-PIC-LABEL: f32_bzhi_i8_gep_offset:
+; X64-PIC: # %bb.0: # %entry
+; X64-PIC-NEXT: movl %edi, %eax
+; X64-PIC-NEXT: leaq fill_table32(%rip), %rcx
+; X64-PIC-NEXT: andl 4(%rcx,%rsi,4), %eax
+; X64-PIC-NEXT: retq
+;
+; X86-LABEL: f32_bzhi_i8_gep_offset:
+; X86: # %bb.0: # %entry
+; X86-NEXT: movl {{[0-9]+}}(%esp), %eax
+; X86-NEXT: movl fill_table32+4(,%eax,4), %eax
+; X86-NEXT: andl {{[0-9]+}}(%esp), %eax
+; X86-NEXT: retl
+entry:
+ %o = shl nuw nsw i64 %y, 2
+ %o2 = add nuw nsw i64 %o, 4
+ %p = getelementptr inbounds i8, ptr @fill_table32, i64 %o2
+ %0 = load i32, ptr %p, align 4
+ %and = and i32 %0, %x
+ ret i32 %and
+}
+
+; A whole array past fill_table32.
+define i32 @f32_bzhi_row1(i32 %x, i64 %y) local_unnamed_addr {
+; X64-STATIC-LABEL: f32_bzhi_row1:
+; X64-STATIC: # %bb.0: # %entry
+; X64-STATIC-NEXT: movl %edi, %eax
+; X64-STATIC-NEXT: andl fill_table32+128(,%rsi,4), %eax
+; X64-STATIC-NEXT: retq
+;
+; X64-PIC-LABEL: f32_bzhi_row1:
+; X64-PIC: # %bb.0: # %entry
+; X64-PIC-NEXT: movl %edi, %eax
+; X64-PIC-NEXT: leaq fill_table32(%rip), %rcx
+; X64-PIC-NEXT: andl 128(%rcx,%rsi,4), %eax
+; X64-PIC-NEXT: retq
+;
+; X86-LABEL: f32_bzhi_row1:
+; X86: # %bb.0: # %entry
+; X86-NEXT: movl {{[0-9]+}}(%esp), %eax
+; X86-NEXT: movl fill_table32+128(,%eax,4), %eax
+; X86-NEXT: andl {{[0-9]+}}(%esp), %eax
+; X86-NEXT: retl
+entry:
+ %p = getelementptr [32 x i32], ptr @fill_table32, i64 1, i64 %y
+ %0 = load i32, ptr %p, align 4
+ %and = and i32 %0, %x
+ ret i32 %and
+}
More information about the llvm-commits
mailing list