[SelectionDAG] Deal with POISON for INSERT_VECTOR_ELT/INSERT_SUBVECTOR (part 1) #143102
Conversation
llvmbot
added
backend:AArch64
backend:AMDGPU
backend:X86
llvm:SelectionDAG
|
@llvm/pr-subscribers-backend-x86 @llvm/pr-subscribers-backend-amdgpu Author: Björn Pettersson (bjope) ChangesAs reported in #141034 SelectionDAG::getNode had some unexpected Same kind of bug existed in DAGCombiner::visitINSERT_SUBVECTOR. Here are some examples: This fold was done even if vec[idx] was POISON: This fold was done even if any of vec[idx..idx+size] was POISON: This fold was done even if the elements not extracted from vec could With this patch we avoid such folds unless we can prove that the This patch in itself result in some regressions. Goal is to try to Fixes #141034 Patch is 219.99 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/143102.diff 20 Files Affected:
diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
index aba3c0f80a024..a52edca64dbce 100644
--- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -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);
@@ -22915,14 +22916,29 @@ 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;
+ } else if (DAG.isGuaranteedNotToBePoison(InVec)) {
+ return InVec;
+ }
}
+ if (!IndexC)
+ return SDValue();
+
if (VT.isScalableVector())
return SDValue();
@@ -27355,18 +27371,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;
+ } else if (DAG.isGuaranteedNotToBePoison(N0))
+ return 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)) {
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
index 279c7daf71c33..6bff2d90b91ac 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -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();
@@ -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:
diff --git a/llvm/test/CodeGen/AArch64/aarch64-neon-vector-insert-uaddlv.ll b/llvm/test/CodeGen/AArch64/aarch64-neon-vector-insert-uaddlv.ll
index 91eda8d552397..83e35599c4e9e 100644
--- a/llvm/test/CodeGen/AArch64/aarch64-neon-vector-insert-uaddlv.ll
+++ b/llvm/test/CodeGen/AArch64/aarch64-neon-vector-insert-uaddlv.ll
@@ -229,9 +229,10 @@ define void @insert_vec_v3i16_uaddlv_from_v8i16(ptr %0) {
; CHECK: ; %bb.0: ; %entry
; CHECK-NEXT: movi.2d v0, #0000000000000000
; CHECK-NEXT: movi.2d v1, #0000000000000000
-; CHECK-NEXT: add x8, x0, #8
; CHECK-NEXT: uaddlv.8h s0, v0
; CHECK-NEXT: mov.h v1[0], v0[0]
+; CHECK-NEXT: mov.h v1[3], w8
+; CHECK-NEXT: add x8, x0, #8
; CHECK-NEXT: ushll.4s v1, v1, #0
; CHECK-NEXT: ucvtf.4s v1, v1
; CHECK-NEXT: st1.s { v1 }[2], [x8]
diff --git a/llvm/test/CodeGen/AArch64/arm64-vector-insertion.ll b/llvm/test/CodeGen/AArch64/arm64-vector-insertion.ll
index 94074d1689f6a..2c44f56316801 100644
--- a/llvm/test/CodeGen/AArch64/arm64-vector-insertion.ll
+++ b/llvm/test/CodeGen/AArch64/arm64-vector-insertion.ll
@@ -66,6 +66,36 @@ 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[8], w8
+; 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
@@ -94,6 +124,43 @@ 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[8], 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
diff --git a/llvm/test/CodeGen/AArch64/sve-fixed-length-fp-vselect.ll b/llvm/test/CodeGen/AArch64/sve-fixed-length-fp-vselect.ll
index 9efe0b33910c8..2905d707bdd09 100644
--- a/llvm/test/CodeGen/AArch64/sve-fixed-length-fp-vselect.ll
+++ b/llvm/test/CodeGen/AArch64/sve-fixed-length-fp-vselect.ll
@@ -37,6 +37,10 @@ define void @select_v16f16(ptr %a, ptr %b) vscale_range(2,0) #0 {
; CHECK-NEXT: ld1h { z0.h }, p0/z, [x0]
; CHECK-NEXT: ld1h { z1.h }, p0/z, [x1]
; CHECK-NEXT: fcmeq p1.h, p0/z, z0.h, z1.h
+; CHECK-NEXT: mov z2.h, p1/z, #-1 // =0xffffffffffffffff
+; CHECK-NEXT: ptrue p1.h
+; CHECK-NEXT: and z2.h, z2.h, #0x1
+; CHECK-NEXT: cmpne p1.h, p1/z, z2.h, #0
; CHECK-NEXT: sel z0.h, p1, z0.h, z1.h
; CHECK-NEXT: st1h { z0.h }, p0, [x0]
; CHECK-NEXT: ret
@@ -59,8 +63,15 @@ define void @select_v32f16(ptr %a, ptr %b) #0 {
; VBITS_GE_256-NEXT: ld1h { z3.h }, p0/z, [x1]
; VBITS_GE_256-NEXT: fcmeq p1.h, p0/z, z0.h, z1.h
; VBITS_GE_256-NEXT: fcmeq p2.h, p0/z, z2.h, z3.h
-; VBITS_GE_256-NEXT: sel z0.h, p1, z0.h, z1.h
-; VBITS_GE_256-NEXT: sel z1.h, p2, z2.h, z3.h
+; VBITS_GE_256-NEXT: mov z4.h, p1/z, #-1 // =0xffffffffffffffff
+; VBITS_GE_256-NEXT: ptrue p1.h
+; VBITS_GE_256-NEXT: mov z5.h, p2/z, #-1 // =0xffffffffffffffff
+; VBITS_GE_256-NEXT: and z4.h, z4.h, #0x1
+; VBITS_GE_256-NEXT: and z5.h, z5.h, #0x1
+; VBITS_GE_256-NEXT: cmpne p2.h, p1/z, z4.h, #0
+; VBITS_GE_256-NEXT: cmpne p1.h, p1/z, z5.h, #0
+; VBITS_GE_256-NEXT: sel z0.h, p2, z0.h, z1.h
+; VBITS_GE_256-NEXT: sel z1.h, p1, z2.h, z3.h
; VBITS_GE_256-NEXT: st1h { z0.h }, p0, [x0, x8, lsl #1]
; VBITS_GE_256-NEXT: st1h { z1.h }, p0, [x0]
; VBITS_GE_256-NEXT: ret
@@ -71,6 +82,10 @@ define void @select_v32f16(ptr %a, ptr %b) #0 {
; VBITS_GE_512-NEXT: ld1h { z0.h }, p0/z, [x0]
; VBITS_GE_512-NEXT: ld1h { z1.h }, p0/z, [x1]
; VBITS_GE_512-NEXT: fcmeq p1.h, p0/z, z0.h, z1.h
+; VBITS_GE_512-NEXT: mov z2.h, p1/z, #-1 // =0xffffffffffffffff
+; VBITS_GE_512-NEXT: ptrue p1.h
+; VBITS_GE_512-NEXT: and z2.h, z2.h, #0x1
+; VBITS_GE_512-NEXT: cmpne p1.h, p1/z, z2.h, #0
; VBITS_GE_512-NEXT: sel z0.h, p1, z0.h, z1.h
; VBITS_GE_512-NEXT: st1h { z0.h }, p0, [x0]
; VBITS_GE_512-NEXT: ret
@@ -89,6 +104,10 @@ define void @select_v64f16(ptr %a, ptr %b) vscale_range(8,0) #0 {
; CHECK-NEXT: ld1h { z0.h }, p0/z, [x0]
; CHECK-NEXT: ld1h { z1.h }, p0/z, [x1]
; CHECK-NEXT: fcmeq p1.h, p0/z, z0.h, z1.h
+; CHECK-NEXT: mov z2.h, p1/z, #-1 // =0xffffffffffffffff
+; CHECK-NEXT: ptrue p1.h
+; CHECK-NEXT: and z2.h, z2.h, #0x1
+; CHECK-NEXT: cmpne p1.h, p1/z, z2.h, #0
; CHECK-NEXT: sel z0.h, p1, z0.h, z1.h
; CHECK-NEXT: st1h { z0.h }, p0, [x0]
; CHECK-NEXT: ret
@@ -107,6 +126,10 @@ define void @select_v128f16(ptr %a, ptr %b) vscale_range(16,0) #0 {
; CHECK-NEXT: ld1h { z0.h }, p0/z, [x0]
; CHECK-NEXT: ld1h { z1.h }, p0/z, [x1]
; CHECK-NEXT: fcmeq p1.h, p0/z, z0.h, z1.h
+; CHECK-NEXT: mov z2.h, p1/z, #-1 // =0xffffffffffffffff
+; CHECK-NEXT: ptrue p1.h
+; CHECK-NEXT: and z2.h, z2.h, #0x1
+; CHECK-NEXT: cmpne p1.h, p1/z, z2.h, #0
; CHECK-NEXT: sel z0.h, p1, z0.h, z1.h
; CHECK-NEXT: st1h { z0.h }, p0, [x0]
; CHECK-NEXT: ret
@@ -150,6 +173,10 @@ define void @select_v8f32(ptr %a, ptr %b) vscale_range(2,0) #0 {
; CHECK-NEXT: ld1w { z0.s }, p0/z, [x0]
; CHECK-NEXT: ld1w { z1.s }, p0/z, [x1]
; CHECK-NEXT: fcmeq p1.s, p0/z, z0.s, z1.s
+; CHECK-NEXT: mov z2.s, p1/z, #-1 // =0xffffffffffffffff
+; CHECK-NEXT: ptrue p1.s
+; CHECK-NEXT: and z2.s, z2.s, #0x1
+; CHECK-NEXT: cmpne p1.s, p1/z, z2.s, #0
; CHECK-NEXT: sel z0.s, p1, z0.s, z1.s
; CHECK-NEXT: st1w { z0.s }, p0, [x0]
; CHECK-NEXT: ret
@@ -172,8 +199,15 @@ define void @select_v16f32(ptr %a, ptr %b) #0 {
; VBITS_GE_256-NEXT: ld1w { z3.s }, p0/z, [x1]
; VBITS_GE_256-NEXT: fcmeq p1.s, p0/z, z0.s, z1.s
; VBITS_GE_256-NEXT: fcmeq p2.s, p0/z, z2.s, z3.s
-; VBITS_GE_256-NEXT: sel z0.s, p1, z0.s, z1.s
-; VBITS_GE_256-NEXT: sel z1.s, p2, z2.s, z3.s
+; VBITS_GE_256-NEXT: mov z4.s, p1/z, #-1 // =0xffffffffffffffff
+; VBITS_GE_256-NEXT: ptrue p1.s
+; VBITS_GE_256-NEXT: mov z5.s, p2/z, #-1 // =0xffffffffffffffff
+; VBITS_GE_256-NEXT: and z4.s, z4.s, #0x1
+; VBITS_GE_256-NEXT: and z5.s, z5.s, #0x1
+; VBITS_GE_256-NEXT: cmpne p2.s, p1/z, z4.s, #0
+; VBITS_GE_256-NEXT: cmpne p1.s, p1/z, z5.s, #0
+; VBITS_GE_256-NEXT: sel z0.s, p2, z0.s, z1.s
+; VBITS_GE_256-NEXT: sel z1.s, p1, z2.s, z3.s
; VBITS_GE_256-NEXT: st1w { z0.s }, p0, [x0, x8, lsl #2]
; VBITS_GE_256-NEXT: st1w { z1.s }, p0, [x0]
; VBITS_GE_256-NEXT: ret
@@ -184,6 +218,10 @@ define void @select_v16f32(ptr %a, ptr %b) #0 {
; VBITS_GE_512-NEXT: ld1w { z0.s }, p0/z, [x0]
; VBITS_GE_512-NEXT: ld1w { z1.s }, p0/z, [x1]
; VBITS_GE_512-NEXT: fcmeq p1.s, p0/z, z0.s, z1.s
+; VBITS_GE_512-NEXT: mov z2.s, p1/z, #-1 // =0xffffffffffffffff
+; VBITS_GE_512-NEXT: ptrue p1.s
+; VBITS_GE_512-NEXT: and z2.s, z2.s, #0x1
+; VBITS_GE_512-NEXT: cmpne p1.s, p1/z, z2.s, #0
; VBITS_GE_512-NEXT: sel z0.s, p1, z0.s, z1.s
; VBITS_GE_512-NEXT: st1w { z0.s }, p0, [x0]
; VBITS_GE_512-NEXT: ret
@@ -202,6 +240,10 @@ define void @select_v32f32(ptr %a, ptr %b) vscale_range(8,0) #0 {
; CHECK-NEXT: ld1w { z0.s }, p0/z, [x0]
; CHECK-NEXT: ld1w { z1.s }, p0/z, [x1]
; CHECK-NEXT: fcmeq p1.s, p0/z, z0.s, z1.s
+; CHECK-NEXT: mov z2.s, p1/z, #-1 // =0xffffffffffffffff
+; CHECK-NEXT: ptrue p1.s
+; CHECK-NEXT: and z2.s, z2.s, #0x1
+; CHECK-NEXT: cmpne p1.s, p1/z, z2.s, #0
; CHECK-NEXT: sel z0.s, p1, z0.s, z1.s
; CHECK-NEXT: st1w { z0.s }, p0, [x0]
; CHECK-NEXT: ret
@@ -220,6 +262,10 @@ define void @select_v64f32(ptr %a, ptr %b) vscale_range(16,0) #0 {
; CHECK-NEXT: ld1w { z0.s }, p0/z, [x0]
; CHECK-NEXT: ld1w { z1.s }, p0/z, [x1]
; CHECK-NEXT: fcmeq p1.s, p0/z, z0.s, z1.s
+; CHECK-NEXT: mov z2.s, p1/z, #-1 // =0xffffffffffffffff
+; CHECK-NEXT: ptrue p1.s
+; CHECK-NEXT: and z2.s, z2.s, #0x1
+; CHECK-NEXT: cmpne p1.s, p1/z, z2.s, #0
; CHECK-NEXT: sel z0.s, p1, z0.s, z1.s
; CHECK-NEXT: st1w { z0.s }, p0, [x0]
; CHECK-NEXT: ret
@@ -264,6 +310,10 @@ define void @select_v4f64(ptr %a, ptr %b) vscale_range(2,0) #0 {
; CHECK-NEXT: ld1d { z0.d }, p0/z, [x0]
; CHECK-NEXT: ld1d { z1.d }, p0/z, [x1]
; CHECK-NEXT: fcmeq p1.d, p0/z, z0.d, z1.d
+; CHECK-NEXT: mov z2.d, p1/z, #-1 // =0xffffffffffffffff
+; CHECK-NEXT: ptrue p1.d
+; CHECK-NEXT: and z2.d, z2.d, #0x1
+; CHECK-N...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
You can test this locally with the following command:git diff -U0 --pickaxe-regex -S '([^a-zA-Z0-9#_-]undef[^a-zA-Z0-9_-]|UndefValue::get)' 'HEAD~1' HEAD llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp llvm/test/CodeGen/AArch64/arm64-build-vector.ll llvm/test/CodeGen/AArch64/concat-vector-add-combine.ll llvm/test/CodeGen/AArch64/sve-fixed-length-fp-vselect.ll llvm/test/CodeGen/AArch64/sve-fixed-length-int-vselect.ll llvm/test/CodeGen/AArch64/sve-fixed-length-masked-gather.ll llvm/test/CodeGen/AArch64/sve-streaming-mode-fixed-length-fp-vselect.ll llvm/test/CodeGen/AArch64/sve-streaming-mode-fixed-length-int-vselect.ll llvm/test/CodeGen/RISCV/rvv/fixed-vectors-vfw-web-simplification.ll llvm/test/CodeGen/RISCV/rvv/fixed-vectors-vw-web-simplification.ll llvm/test/CodeGen/Thumb2/mve-vld3.ll llvm/test/CodeGen/X86/merge-consecutive-loads-128.ll llvm/test/CodeGen/X86/mmx-build-vector.ll llvm/test/CodeGen/X86/vector-shuffle-combining.ll llvm/test/CodeGen/X86/zero_extend_vector_inreg_of_broadcast.llThe following files introduce new uses of undef:
Undef is now deprecated and should only be used in the rare cases where no replacement is possible. For example, a load of uninitialized memory yields In tests, avoid using For example, this is considered a bad practice: define void @fn() {
...
br i1 undef, ...
}Please use the following instead: define void @fn(i1 %cond) {
...
br i1 %cond, ...
}Please refer to the Undefined Behavior Manual for more information. |
| return InVec; | ||
| } else if (DAG.isGuaranteedNotToBePoison(InVec)) { | ||
| return InVec; | ||
| } | ||
| } |
There was a problem hiding this comment.
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);
}There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
(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
| @@ -4716,6 +4717,7 @@ define void @vec384_i64_widen_to_i128_factor2_broadcast_to_v3i128_factor3(ptr %i | |||
| ; AVX-NEXT: vmovdqa %xmm3, 32(%rcx) | |||
| ; AVX-NEXT: vmovdqa %xmm1, 48(%rcx) | |||
| ; AVX-NEXT: vmovdqa %xmm2, (%rcx) | |||
| ; AVX-NEXT: vzeroupper | |||
There was a problem hiding this comment.
These look like merge errors as I've recently updated x86 shuffles to stop emitting so many vzeroupper ops
There was a problem hiding this comment.
They go away again in next patch in the stack #143103
I tried a local rebase but with this patch alone we still get this regression.
bjope
force-pushed
the
users/bjope/insertundef_2
branch
from
33ebb39 to
bf22ac6
Compare
bjope
force-pushed
the
users/bjope/insertundef_2
branch
from
bf22ac6 to
58b1ef9
Compare
| ; X86-SSE-NEXT: movdq2q %xmm0, %mm0 | ||
| ; X86-SSE-NEXT: paddd %mm0, %mm0 | ||
| ; X86-SSE-NEXT: movq %mm0, (%eax) | ||
| ; X86-SSE-NEXT: retl |
There was a problem hiding this comment.
you've lost the X64 test coverage - shared prefixes?
There was a problem hiding this comment.
Hmm. Bug in update_llc_test_checks.py?
There was a problem hiding this comment.
Ok, now I understand. We do not get same output for all X64 variants, so I need to add more prefixes.
(Or maybe swap order and do #143103 first to reduce amount of diffs?)
|
I'm still looking for advice on how to move forward with this.
As I see it fixing #141034 is needed to move forward with the fixes for #138513 (as they rely on that insert of UNDEF isn't surprisingly turned into inserting POISON). I've been stalling on updating the patches for #138513 while waiting for this patch to land. |
bjope
force-pushed
the
users/bjope/insertundef_2
branch
from
c695230 to
6299fdc
Compare
| // result more poisonous). | ||
| if (N1.isUndef()) { | ||
| if (N1.getOpcode() == ISD::POISON) | ||
| return N0; |
There was a problem hiding this comment.
why don't we do this for the visitINSERT_VECTOR_ELT case?
There was a problem hiding this comment.
I think I tried that, but it never triggered. So I didn't want to add a combine that would be "dead code".
| if (DAG.isGuaranteedNotToBePoison(N1.getOperand(0), ~EltMask)) | ||
| return N1.getOperand(0); | ||
| } else if (DAG.isGuaranteedNotToBePoison(N1.getOperand(0))) | ||
| return N1.getOperand(0); |
There was a problem hiding this comment.
return DAG.getFreeze(N1.getOperand(0))?
There was a problem hiding this comment.
I know that I tried that, but that resulted in regressions that I couldn't solve easily. Right now I don't remember the exact scenario. Probably something with the scalable vectors, and a FREEZE that couldn't be eliminated and thereby blocking some combine.
| return N1; | ||
| } else if (isGuaranteedNotToBePoison(N1)) | ||
| return N1; | ||
| } |
There was a problem hiding this comment.
How many regressions do we get it we drop all this from getNode() and just leave the combine to do it?
We've often ended up doing more then basic canonicalisation in getNode() just to avoid one specific test regression.....
There was a problem hiding this comment.
We can remove the things releated to isGuaranteedNotToBePoison from INSERT_VECTOR_ELT and INSERT_SUBVECTOR in getNode without any diffs, if doing it after the changes I propose in "part 2" and "part 3". If doing it directly in this patch there is around 20 lit tests impacted.
| return N0; | ||
| // If inserting an UNDEF, just return the original vector (unless it makes the | ||
| // result more poisonous). | ||
| if (N1.isUndef()) { |
There was a problem hiding this comment.
I keep forgetting than SDValue::isUndef() is really SDValue::isUndefOrPoison() :/
| return N1; | ||
| } else if (isGuaranteedNotToBePoison(N1)) | ||
| return N1; | ||
| } |
There was a problem hiding this comment.
again - are we doing too much in getNode at this stage?
Always try to fold freeze(op(....)) -> op(freeze(),freeze(),freeze(),...). This patch proposes we drop the opt-in limit for opcodes that are allowed to push a freeze through the op to freeze all its operands, bringing us more in line with how InstCombine handles it. I'm struggling to find a strong reason for this limit apart from the DAG freeze handling being immature for so long - as we've improved coverage in canCreateUndefOrPoison/isGuaranteedNotToBeUndefOrPoison it looks like the regressions are not as severe. If there's no objections to this approach I will yak shave some of the remaining regressions. Hopefully this will help some of the regression issues in llvm#143102 etc.
Always try to fold freeze(op(....)) -> op(freeze(),freeze(),freeze(),...). This patch proposes we drop the opt-in limit for opcodes that are allowed to push a freeze through the op to freeze all its operands, through the tree towards the roots. I'm struggling to find a strong reason for this limit apart from the DAG freeze handling being immature for so long - as we've improved coverage in canCreateUndefOrPoison/isGuaranteedNotToBeUndefOrPoison it looks like the regressions are not as severe. Hopefully this will help some of the regression issues in #143102 etc.
…R (part 1) As reported in #141034 SelectionDAG::getNode had some unexpected behaviors when trying to create vectors with UNDEF elements. Since we treat both UNDEF and POISON as undefined (when using isUndef()) we can't just fold away INSERT_VECTOR_ELT/INSERT_SUBVECTOR based on isUndef(), as that could make the resulting vector more poisonous. Same kind of bug existed in DAGCombiner::visitINSERT_SUBVECTOR. Here are some examples: This fold was done even if vec[idx] was POISON: INSERT_VECTOR_ELT vec, UNDEF, idx -> vec This fold was done even if any of vec[idx..idx+size] was POISON: INSERT_SUBVECTOR vec, UNDEF, idx -> vec This fold was done even if the elements not extracted from vec could be POISON: sub = EXTRACT_SUBVECTOR vec, idx INSERT_SUBVECTOR UNDEF, sub, idx -> vec With this patch we avoid such folds unless we can prove that the result isn't more poisonous when eliminating the insert. This patch in itself result in some regressions. Goal is to try to deal with those regressions in follow up commits. Fixes #141034
bjope
force-pushed
the
users/bjope/insertundef_2
branch
from
6299fdc to
0f35a70
Compare
|
Yes, I was preparing a rebase on top of 145939 (thanks!). And I've updated this pull request now. As shown in #143103 we can do some enhancements in isGuaranteedNotToBeUndefOrPoison to avoid some diffs otherwise introduced by this patch. The opcodes I identified in that patch was INSERT_VECTOR_ELT/INSERT_SUBVECTOR, but I have another patch here #147019 that I think would make isGuaranteedNotToBeUndefOrPoison treat demanded elts in even more situations (as a more general improvement). I haven't gotten any feedback about #143105 which would avoid lots of the regressions around scalable vectors (I think you stumbled upon that problem in #146332). But that relies on that it would be OK to set the extended lanes to POISON in convertToScalableVector for AArch64 and RISC-V. |
|
So right now, if checking out #143105 (the branch users/bjope/insertundef_4), the only test cases that has a diff compared to the base commit from main are these five: At least a couple of those seem trivial. Maybe the last one would be the main concern if any? |
|
One thing that is hurting us is the freeze getting in between a load and ext prevent a extload forming: I'm not certain but it'd be great if we were allowed to fold these to: WDYT? |
|
Similarly - should we be allowed to remove freezes from constant pool loads entirely (maybe with some handling for undef elts in the Constant)? |
As reported in #141034 SelectionDAG::getNode had some unexpected
behaviors when trying to create vectors with UNDEF elements. Since
we treat both UNDEF and POISON as undefined (when using isUndef())
we can't just fold away INSERT_VECTOR_ELT/INSERT_SUBVECTOR based on
isUndef(), as that could make the resulting vector more poisonous.
Same kind of bug existed in DAGCombiner::visitINSERT_SUBVECTOR.
Here are some examples:
This fold was done even if vec[idx] was POISON:
INSERT_VECTOR_ELT vec, UNDEF, idx -> vec
This fold was done even if any of vec[idx..idx+size] was POISON:
INSERT_SUBVECTOR vec, UNDEF, idx -> vec
This fold was done even if the elements not extracted from vec could
be POISON:
sub = EXTRACT_SUBVECTOR vec, idx
INSERT_SUBVECTOR UNDEF, sub, idx -> vec
With this patch we avoid such folds unless we can prove that the
result isn't more poisonous when eliminating the insert.
This patch in itself result in some regressions. Goal is to try to
deal with those regressions in follow up commits.
Fixes #141034