Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[SelectionDAG] Deal with POISON for INSERT_VECTOR_ELT/INSERT_SUBVECTOR (part 1) #143102

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
Loading
from
Open

[SelectionDAG] Deal with POISON for INSERT_VECTOR_ELT/INSERT_SUBVECTOR (part 1) #143102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
004e26e
[test][AArch64] Adjust vector insertion lit tests
bjope Jun 3, 2025
b3a6687
[SelectionDAG] Deal with POISON for INSERT_VECTOR_ELT/INSERT_SUBVECTO…
bjope May 31, 2025
f6ffe48
clang-format fixes
bjope Jun 6, 2025
33ebb39
Fixup: Use getFreeze in DAGCombiner.
bjope Jun 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 50 additions & 13 deletions 63 llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22905,6 +22905,7 @@ SDValue DAGCombiner::visitINSERT_VECTOR_ELT(SDNode *N) {
auto *IndexC = dyn_cast<ConstantSDNode>(EltNo);

// Insert into out-of-bounds element is undefined.
// Code below relies on that we handle this special case early.
if (IndexC && VT.isFixedLengthVector() &&
IndexC->getZExtValue() >= VT.getVectorNumElements())
return DAG.getUNDEF(VT);
Expand All @@ -22915,14 +22916,28 @@ SDValue DAGCombiner::visitINSERT_VECTOR_ELT(SDNode *N) {
InVec == InVal.getOperand(0) && EltNo == InVal.getOperand(1))
return InVec;

if (!IndexC) {
// If this is variable insert to undef vector, it might be better to splat:
// inselt undef, InVal, EltNo --> build_vector < InVal, InVal, ... >
if (InVec.isUndef() && TLI.shouldSplatInsEltVarIndex(VT))
return DAG.getSplat(VT, DL, InVal);
return SDValue();
// If this is variable insert to undef vector, it might be better to splat:
// inselt undef, InVal, EltNo --> build_vector < InVal, InVal, ... >
if (!IndexC && InVec.isUndef() && TLI.shouldSplatInsEltVarIndex(VT))
return DAG.getSplat(VT, DL, InVal);

// Try to drop insert of UNDEF/POISON elements. This is also done in getNode,
// but we also do it as a DAG combine since for example simplifications into
// SPLAT_VECTOR/BUILD_VECTOR may turn poison elements into undef/zero etc, and
// then suddenly the InVec is guaranteed to not be poison.
if (InVal.isUndef()) {
if (IndexC && VT.isFixedLengthVector()) {
APInt EltMask = APInt::getOneBitSet(VT.getVectorNumElements(),
IndexC->getZExtValue());
if (DAG.isGuaranteedNotToBePoison(InVec, EltMask))
return InVec;
}
return DAG.getFreeze(InVec);
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not sure why we can't do this:

  if (InVal.isUndef()) {
    if (IndexC && VT.isFixedLengthVector()) {
      APInt EltMask = APInt::getOneBitSet(VT.getVectorNumElements(),
                                          IndexC->getZExtValue());
      if (DAG.isGuaranteedNotToBePoison(InVec, EltMask))
        return InVec;
    }
    return DAG.getFreeze(InVec);
  }

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that we want to do that. For example to avoid that we attempt to do this DAG combine multiple times.
Doing it in this patch would result in several more lit test diffs. I think we at least want "part 2" (#143103) first and maybe also "part 3" (#143105).
I could put those commits first (as preparations), but then I might need to figure out how to motivate the various changes in those patches. Now this "part 1" patch kind of give enough regressions to motivate the other fixups, but without touching a lot of lit tests.

Not sure what is preferred from a reviewers point of view? One idea is to add another patch at the end of the stack that use getFreeze.

(Juggling with stacked pull requests in github is still something that I'm still not very confortable with. It really messes up my local workflow as I can't just "git rebase -i" to move between the patch stack.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Juggling with stacked pull requests in github is still something that I'm still not very confortable with. It really messes up my local workflow as I can't just "git rebase -i" to move between the patch stack.)

You need to use one of the tools (graphite or spr), these manually stacked with pre-commits are as much of a pain to review as to submit

if (!IndexC)
return SDValue();

if (VT.isScalableVector())
return SDValue();

Expand Down Expand Up @@ -27355,18 +27370,40 @@ SDValue DAGCombiner::visitINSERT_SUBVECTOR(SDNode *N) {
SDValue N2 = N->getOperand(2);
uint64_t InsIdx = N->getConstantOperandVal(2);

// If inserting an UNDEF, just return the original vector.
if (N1.isUndef())
return N0;
// If inserting an UNDEF, just return the original vector (unless it makes the
// result more poisonous).
if (N1.isUndef()) {
if (VT.isFixedLengthVector()) {
unsigned SubVecNumElts = N1.getValueType().getVectorNumElements();
APInt EltMask = APInt::getBitsSet(VT.getVectorNumElements(), InsIdx,
InsIdx + SubVecNumElts);
if (DAG.isGuaranteedNotToBePoison(N0, EltMask))
return N0;
}
return DAG.getFreeze(N0);
}

// If this is an insert of an extracted vector into an undef vector, we can
// just use the input to the extract if the types match, and can simplify
// If this is an insert of an extracted vector into an undef/poison vector, we
// can just use the input to the extract if the types match, and can simplify
// in some cases even if they don't.
if (N0.isUndef() && N1.getOpcode() == ISD::EXTRACT_SUBVECTOR &&
N1.getOperand(1) == N2) {
EVT N1VT = N1.getValueType();
EVT SrcVT = N1.getOperand(0).getValueType();
if (SrcVT == VT)
return N1.getOperand(0);
if (SrcVT == VT) {
// Need to ensure that result isn't more poisonous if skipping both the
// extract+insert.
if (N0.getOpcode() == ISD::POISON)
return N1.getOperand(0);
if (VT.isFixedLengthVector() && N1VT.isFixedLengthVector()) {
unsigned SubVecNumElts = N1VT.getVectorNumElements();
APInt EltMask = APInt::getBitsSet(VT.getVectorNumElements(), InsIdx,
InsIdx + SubVecNumElts);
if (DAG.isGuaranteedNotToBePoison(N1.getOperand(0), ~EltMask))
return N1.getOperand(0);
} else if (DAG.isGuaranteedNotToBePoison(N1.getOperand(0)))
return N1.getOperand(0);
}
// TODO: To remove the zero check, need to adjust the offset to
// a multiple of the new src type.
if (isNullConstant(N2)) {
Expand Down
65 changes: 55 additions & 10 deletions 65 llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7900,23 +7900,42 @@ SDValue SelectionDAG::getNode(unsigned Opcode, const SDLoc &DL, EVT VT,
// INSERT_VECTOR_ELT into out-of-bounds element is an UNDEF, except
// for scalable vectors where we will generate appropriate code to
// deal with out-of-bounds cases correctly.
if (N3C && N1.getValueType().isFixedLengthVector() &&
N3C->getZExtValue() >= N1.getValueType().getVectorNumElements())
if (N3C && VT.isFixedLengthVector() &&
N3C->getZExtValue() >= VT.getVectorNumElements())
return getUNDEF(VT);

// Undefined index can be assumed out-of-bounds, so that's UNDEF too.
if (N3.isUndef())
return getUNDEF(VT);

// If the inserted element is an UNDEF, just use the input vector.
if (N2.isUndef())
// If inserting poison, just use the input vector.
if (N2.getOpcode() == ISD::POISON)
return N1;

// Inserting undef into undef/poison is still undef.
if (N2.getOpcode() == ISD::UNDEF && N1.isUndef())
return getUNDEF(VT);

// If the inserted element is an UNDEF, just use the input vector.
// But not if skipping the insert could make the result more poisonous.
if (N2.isUndef()) {
if (N3C && VT.isFixedLengthVector()) {
APInt EltMask =
APInt::getOneBitSet(VT.getVectorNumElements(), N3C->getZExtValue());
if (isGuaranteedNotToBePoison(N1, EltMask))
return N1;
} else if (isGuaranteedNotToBePoison(N1))
return N1;
}
break;
}
case ISD::INSERT_SUBVECTOR: {
// Inserting undef into undef is still undef.
if (N1.isUndef() && N2.isUndef())
// If inserting poison, just use the input vector,
if (N2.getOpcode() == ISD::POISON)
return N1;

// Inserting undef into undef/poison is still undef.
if (N2.getOpcode() == ISD::UNDEF && N1.isUndef())
return getUNDEF(VT);

EVT N2VT = N2.getValueType();
Expand Down Expand Up @@ -7945,11 +7964,37 @@ SDValue SelectionDAG::getNode(unsigned Opcode, const SDLoc &DL, EVT VT,
if (VT == N2VT)
return N2;

// If this is an insert of an extracted vector into an undef vector, we
// can just use the input to the extract.
// If this is an insert of an extracted vector into an undef/poison vector,
// we can just use the input to the extract. But not if skipping the
// extract+insert could make the result more poisonous.
if (N1.isUndef() && N2.getOpcode() == ISD::EXTRACT_SUBVECTOR &&
N2.getOperand(1) == N3 && N2.getOperand(0).getValueType() == VT)
return N2.getOperand(0);
N2.getOperand(1) == N3 && N2.getOperand(0).getValueType() == VT) {
if (N1.getOpcode() == ISD::POISON)
return N2.getOperand(0);
if (VT.isFixedLengthVector() && N2VT.isFixedLengthVector()) {
unsigned LoBit = N3->getAsZExtVal();
unsigned HiBit = LoBit + N2VT.getVectorNumElements();
APInt EltMask =
APInt::getBitsSet(VT.getVectorNumElements(), LoBit, HiBit);
if (isGuaranteedNotToBePoison(N2.getOperand(0), ~EltMask))
return N2.getOperand(0);
} else if (isGuaranteedNotToBePoison(N2.getOperand(0)))
return N2.getOperand(0);
}

// If the inserted subvector is UNDEF, just use the input vector.
// But not if skipping the insert could make the result more poisonous.
if (N2.isUndef()) {
if (VT.isFixedLengthVector()) {
unsigned LoBit = N3->getAsZExtVal();
unsigned HiBit = LoBit + N2VT.getVectorNumElements();
APInt EltMask =
APInt::getBitsSet(VT.getVectorNumElements(), LoBit, HiBit);
if (isGuaranteedNotToBePoison(N1, EltMask))
return N1;
} else if (isGuaranteedNotToBePoison(N1))
return N1;
}
break;
}
case ISD::BITCAST:
Expand Down
4 changes: 2 additions & 2 deletions 4 llvm/test/CodeGen/AArch64/arm64-build-vector.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ define void @widen_f16_build_vector(ptr %addr) {
; CHECK-LABEL: widen_f16_build_vector:
; CHECK: // %bb.0:
; CHECK-NEXT: mov w8, #13294 // =0x33ee
; CHECK-NEXT: movk w8, #13294, lsl #16
; CHECK-NEXT: str w8, [x0]
; CHECK-NEXT: dup v0.4h, w8
; CHECK-NEXT: str s0, [x0]
; CHECK-NEXT: ret
store <2 x half> <half 0xH33EE, half 0xH33EE>, ptr %addr, align 2
ret void
Expand Down
65 changes: 65 additions & 0 deletions 65 llvm/test/CodeGen/AArch64/arm64-vector-insertion.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,35 @@ define <16 x i8> @test_insert_v16i8_insert_2_undef_base(i8 %a) {
%v.6 = insertelement <16 x i8> %v.4, i8 %a, i32 6
%v.7 = insertelement <16 x i8> %v.6, i8 %a, i32 7
%v.8 = insertelement <16 x i8> %v.7, i8 %a, i32 8
%v.10 = insertelement <16 x i8> %v.8, i8 %a, i32 10
%v.11 = insertelement <16 x i8> %v.10, i8 %a, i32 11
%v.12 = insertelement <16 x i8> %v.11, i8 %a, i32 12
%v.13 = insertelement <16 x i8> %v.12, i8 %a, i32 13
%v.14 = insertelement <16 x i8> %v.13, i8 %a, i32 14
%v.15 = insertelement <16 x i8> %v.14, i8 %a, i32 15
ret <16 x i8> %v.15
}

; Similar to above, but we leave element 8 as undef. One interesting part with
; this test case is that %a may be poison, so simply inserting %a also at
; index 8 would make the result vector more poisonous.
define <16 x i8> @test_insert_v16i8_insert_2_undef_base_skip8(i32 %a0) {
; CHECK-LABEL: test_insert_v16i8_insert_2_undef_base_skip8:
; CHECK: // %bb.0:
; CHECK-NEXT: lsr w8, w0, #5
; CHECK-NEXT: dup.16b v0, w8
; CHECK-NEXT: mov.b v0[5], wzr
; CHECK-NEXT: mov.b v0[9], wzr
; CHECK-NEXT: ret
%a1 = lshr exact i32 %a0, 5
%a = trunc i32 %a1 to i8
%v.0 = insertelement <16 x i8> <i8 undef, i8 undef, i8 undef, i8 undef, i8 undef, i8 0, i8 undef, i8 undef, i8 undef, i8 0, i8 undef, i8 undef, i8 undef, i8 undef, i8 undef, i8 undef> , i8 %a, i32 0
%v.1 = insertelement <16 x i8> %v.0, i8 %a, i32 1
%v.2 = insertelement <16 x i8> %v.1, i8 %a, i32 2
%v.3 = insertelement <16 x i8> %v.2, i8 %a, i32 3
%v.4 = insertelement <16 x i8> %v.3, i8 %a, i32 4
%v.6 = insertelement <16 x i8> %v.4, i8 %a, i32 6
%v.7 = insertelement <16 x i8> %v.6, i8 %a, i32 7
%v.10 = insertelement <16 x i8> %v.7, i8 %a, i32 10
%v.11 = insertelement <16 x i8> %v.10, i8 %a, i32 11
%v.12 = insertelement <16 x i8> %v.11, i8 %a, i32 12
Expand Down Expand Up @@ -94,6 +123,42 @@ define <16 x i8> @test_insert_v16i8_insert_2_undef_base_different_valeus(i8 %a,
%v.6 = insertelement <16 x i8> %v.4, i8 %a, i32 6
%v.7 = insertelement <16 x i8> %v.6, i8 %b, i32 7
%v.8 = insertelement <16 x i8> %v.7, i8 %a, i32 8
%v.10 = insertelement <16 x i8> %v.8, i8 %a, i32 10
%v.11 = insertelement <16 x i8> %v.10, i8 %a, i32 11
%v.12 = insertelement <16 x i8> %v.11, i8 %b, i32 12
%v.13 = insertelement <16 x i8> %v.12, i8 %a, i32 13
%v.14 = insertelement <16 x i8> %v.13, i8 %a, i32 14
%v.15 = insertelement <16 x i8> %v.14, i8 %b, i32 15
ret <16 x i8> %v.15
}

; Similar to above, but we leave element 8 as undef. One interesting part with
; this test case is that %a and %b may be poison, so simply inserting %a or %b
; at index 8 would make the result vector more poisonous.
define <16 x i8> @test_insert_v16i8_insert_2_undef_base_different_valeus_skip8(i32 %a0, i32 %b0) {
; CHECK-LABEL: test_insert_v16i8_insert_2_undef_base_different_valeus_skip8:
; CHECK: // %bb.0:
; CHECK-NEXT: lsr w8, w0, #5
; CHECK-NEXT: dup.16b v0, w8
; CHECK-NEXT: lsr w8, w1, #5
; CHECK-NEXT: mov.b v0[2], w8
; CHECK-NEXT: mov.b v0[5], wzr
; CHECK-NEXT: mov.b v0[7], w8
; CHECK-NEXT: mov.b v0[9], wzr
; CHECK-NEXT: mov.b v0[12], w8
; CHECK-NEXT: mov.b v0[15], w8
; CHECK-NEXT: ret
%a1 = lshr exact i32 %a0, 5
%a = trunc i32 %a1 to i8
%b1 = lshr exact i32 %b0, 5
%b = trunc i32 %b1 to i8
%v.0 = insertelement <16 x i8> <i8 undef, i8 undef, i8 undef, i8 undef, i8 undef, i8 0, i8 undef, i8 undef, i8 undef, i8 0, i8 undef, i8 undef, i8 undef, i8 undef, i8 undef, i8 undef> , i8 %a, i32 0
%v.1 = insertelement <16 x i8> %v.0, i8 %a, i32 1
%v.2 = insertelement <16 x i8> %v.1, i8 %b, i32 2
%v.3 = insertelement <16 x i8> %v.2, i8 %a, i32 3
%v.4 = insertelement <16 x i8> %v.3, i8 %a, i32 4
%v.6 = insertelement <16 x i8> %v.4, i8 %a, i32 6
%v.7 = insertelement <16 x i8> %v.6, i8 %b, i32 7
%v.10 = insertelement <16 x i8> %v.7, i8 %a, i32 10
%v.11 = insertelement <16 x i8> %v.10, i8 %a, i32 11
%v.12 = insertelement <16 x i8> %v.11, i8 %b, i32 12
Expand Down
19 changes: 8 additions & 11 deletions 19 llvm/test/CodeGen/AArch64/concat-vector-add-combine.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,17 +93,14 @@ define i32 @combine_add_8xi32(i32 %a, i32 %b, i32 %c, i32 %d, i32 %e, i32 %f, i3
define i32 @combine_undef_add_8xi32(i32 %a, i32 %b, i32 %c, i32 %d) local_unnamed_addr #0 {
; CHECK-LABEL: combine_undef_add_8xi32:
; CHECK: // %bb.0:
; CHECK-NEXT: fmov s1, w0
; CHECK-NEXT: movi v0.2d, #0000000000000000
; CHECK-NEXT: mov v1.s[1], w1
; CHECK-NEXT: uhadd v0.4h, v0.4h, v0.4h
; CHECK-NEXT: mov v1.s[2], w2
; CHECK-NEXT: mov v1.s[3], w3
; CHECK-NEXT: xtn v2.4h, v1.4s
; CHECK-NEXT: shrn v1.4h, v1.4s, #16
; CHECK-NEXT: uhadd v1.4h, v2.4h, v1.4h
; CHECK-NEXT: mov v1.d[1], v0.d[0]
; CHECK-NEXT: uaddlv s0, v1.8h
; CHECK-NEXT: fmov s0, w0
; CHECK-NEXT: mov v0.s[1], w1
; CHECK-NEXT: mov v0.s[2], w2
; CHECK-NEXT: mov v0.s[3], w3
; CHECK-NEXT: uzp2 v1.8h, v0.8h, v0.8h
; CHECK-NEXT: uzp1 v0.8h, v0.8h, v0.8h
; CHECK-NEXT: uhadd v0.8h, v0.8h, v1.8h
; CHECK-NEXT: uaddlv s0, v0.8h
; CHECK-NEXT: fmov w0, s0
; CHECK-NEXT: ret
%a1 = insertelement <8 x i32> poison, i32 %a, i32 0
Expand Down
Loading
Loading
Morty Proxy This is a proxified and sanitized view of the page, visit original site.