DAG: Fold extract_vector_elt (scalar_to_vector), K to undef

[RKSimon]

Summary

This was unconditionally folding this to the source operand,
even if the access was out of bounds. Use undef instead of
the extract is not the first element.

This helps with some cases where 3-vectors are legalized
and avoids processing the 4th component.

Avoids regressions in a future commit.

[RKSimon]

Avoids regressions in a future commit.

Do you have the patch available to show the test case?

[arsenm]

In D51589#1228234, @RKSimon wrote:

Avoids regressions in a future commit.

Do you have the patch available to show the test case?

I think this was for D51736, but I somehow ended up avoiding needing this to avoid regressions.

[RKSimon]

In D51589#1228474, @arsenm wrote:

In D51589#1228234, @RKSimon wrote:

Avoids regressions in a future commit.

Do you have the patch available to show the test case?

I think this was for D51736, but I somehow ended up avoiding needing this to avoid regressions.

So would you prefer to abandon this or try to add a different test?

[arsenm]

I think this should be done, but I have a hard time coming up with AMDGPU test cases that actually use SCALAR_TO_VECTOR

[RKSimon]

The patch has rotted and needs rewriting, but putting in an assert causes hits in these tests:

Failing Tests (16):
    LLVM :: CodeGen/AMDGPU/copy-illegal-type.ll
    LLVM :: CodeGen/AMDGPU/cvt_f32_ubyte.ll
    LLVM :: CodeGen/AMDGPU/flat_atomics_i64.ll
    LLVM :: CodeGen/AMDGPU/global_atomics_i64.ll
    LLVM :: CodeGen/AMDGPU/load-constant-i1.ll
    LLVM :: CodeGen/AMDGPU/load-constant-i16.ll
    LLVM :: CodeGen/AMDGPU/load-global-i1.ll
    LLVM :: CodeGen/AMDGPU/load-global-i16.ll
    LLVM :: CodeGen/AMDGPU/load-local-i1.ll
    LLVM :: CodeGen/AMDGPU/load-local-i16.ll
    LLVM :: CodeGen/AMDGPU/max.i16.ll
    LLVM :: CodeGen/AMDGPU/min.ll
    LLVM :: CodeGen/AMDGPU/select.f16.ll
    LLVM :: CodeGen/AMDGPU/selectcc.ll
    LLVM :: CodeGen/AMDGPU/v_madak_f16.ll
    LLVM :: CodeGen/X86/vector-intrinsics.ll

If possible please can you use one of these to show a codegen change? I tried the x86 test but didn't find anything.

TBH, I think this patch can be accepted anyhow as we know the codepath is being tested, its just annoying that it doesn't show up in test codegen....

[RKSimon]

@arsenm Are you still looking at this? I haven't been able to make a test case yet but I'm expecting PR41097 to require this as well.

[arsenm]

In D51589#1432072, @RKSimon wrote:

@arsenm Are you still looking at this? I haven't been able to make a test case yet but I'm expecting PR41097 to require this as well.

No, I don’t have time for this now

[RKSimon]

Finally got a test change, and its an increase in instructions....

[arsenm]

I think this is a regression because at some point the high element unnecessarily goes from undef to 0:
Combining: t158: v2i16 = extract_subvector t188, Constant:i32<2>
... into: t212: v2i16 = BUILD_VECTOR t134, Constant:i16<0>

This was undef originally

[RKSimon]

rebase - still need to fix the extract subvector regression

[RKSimon]

rebase

[RKSimon]

@arsenm The subvector regression is still there but the actual instruction count has reduced

[arsenm]

LGTM

llvm/test/CodeGen/AMDGPU/max.i16.ll

163–164

I'm pretty sure these zeros are dead

173–175

I think this for some reason just got luckier on scheduling

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

SDValue DAGCombiner::visitEXTRACT_VECTOR_ELT(SDNode *N) {
   if (VecOp.getOpcode() == ISD::INSERT_VECTOR_ELT &&
       Index == VecOp.getOperand(2)) {
     SDValue Elt = VecOp.getOperand(1);
     return VecVT.isInteger() ? DAG.getAnyExtOrTrunc(Elt, DL, ScalarVT) : Elt;
   }
 
   // (vextract (scalar_to_vector val, 0) -> val
   if (VecOp.getOpcode() == ISD::SCALAR_TO_VECTOR) {
+    // Only 0'th element of SCALAR_TO_VECTOR is defined.
+    if (DAG.isKnownNeverZero(Index))
+      return DAG.getUNDEF(ScalarVT);
+
     // Check if the result type doesn't match the inserted element type. A
     // SCALAR_TO_VECTOR may truncate the inserted element and the
     // EXTRACT_VECTOR_ELT may widen the extracted vector.
     SDValue InOp = VecOp.getOperand(0);
     if (InOp.getValueType() != ScalarVT) {
       assert(InOp.getValueType().isInteger() && ScalarVT.isInteger());
       return DAG.getSExtOrTrunc(InOp, DL, ScalarVT);
     }

llvm/test/CodeGen/AMDGPU/cvt_f32_ubyte.ll

 ; SI-NEXT:    s_waitcnt vmcnt(1)
 ; SI-NEXT:    v_lshlrev_b32_e32 v3, 8, v7
 ; SI-NEXT:    s_waitcnt vmcnt(0)
 ; SI-NEXT:    v_cvt_f32_ubyte0_e32 v0, v0
 ; SI-NEXT:    buffer_store_dword v0, off, s[4:7], 0 offset:24
 ; SI-NEXT:    s_waitcnt expcnt(0)
 ; SI-NEXT:    v_lshlrev_b32_e32 v0, 16, v2
 ; SI-NEXT:    v_or_b32_e32 v0, v0, v1
-; SI-NEXT:    v_or_b32_e32 v4, v3, v6
-; SI-NEXT:    v_and_b32_e32 v5, 0xffff0000, v0
-; SI-NEXT:    v_or_b32_e32 v4, v4, v5
-; SI-NEXT:    v_cvt_f32_ubyte1_e32 v5, v4
+; SI-NEXT:    v_or_b32_e32 v3, v3, v6
+; SI-NEXT:    v_cvt_f32_ubyte1_e32 v5, v3
+; SI-NEXT:    v_cvt_f32_ubyte0_e32 v4, v3
 ; SI-NEXT:    v_cvt_f32_ubyte3_e32 v3, v0
 ; SI-NEXT:    v_cvt_f32_ubyte2_e32 v2, v0
 ; SI-NEXT:    v_cvt_f32_ubyte1_e32 v1, v0
 ; SI-NEXT:    v_cvt_f32_ubyte0_e32 v0, v0
-; SI-NEXT:    v_cvt_f32_ubyte0_e32 v4, v4
-; SI-NEXT:    buffer_store_dwordx4 v[0:3], off, s[4:7], 0
 ; SI-NEXT:    buffer_store_dwordx2 v[4:5], off, s[4:7], 0 offset:16
+; SI-NEXT:    buffer_store_dwordx4 v[0:3], off, s[4:7], 0
 ; SI-NEXT:    s_endpgm
 ;
 ; VI-LABEL: load_v7i8_to_v7f32:
 ; VI:       ; %bb.0:
 ; VI-NEXT:    s_load_dwordx2 s[4:5], s[0:1], 0x24
 ; VI-NEXT:    s_load_dwordx2 s[0:1], s[0:1], 0x2c
 ; VI-NEXT:    v_lshlrev_b32_e32 v0, 3, v0
 ; VI-NEXT:    s_mov_b32 s7, 0xf000

 ; VI-NEXT:    flat_load_ubyte v4, v[6:7]
 ; VI-NEXT:    flat_load_ubyte v0, v[0:1]
 ; VI-NEXT:    s_waitcnt vmcnt(6) lgkmcnt(6)
 ; VI-NEXT:    v_lshlrev_b32_e32 v1, 8, v9
 ; VI-NEXT:    s_waitcnt vmcnt(5) lgkmcnt(5)
 ; VI-NEXT:    v_lshlrev_b32_e32 v5, 8, v10
 ; VI-NEXT:    s_waitcnt vmcnt(2) lgkmcnt(2)
 ; VI-NEXT:    v_lshlrev_b32_e32 v3, 8, v3
-; VI-NEXT:    s_waitcnt vmcnt(1) lgkmcnt(1)
-; VI-NEXT:    v_or_b32_e32 v4, v3, v4
 ; VI-NEXT:    s_waitcnt vmcnt(0) lgkmcnt(0)
 ; VI-NEXT:    v_cvt_f32_ubyte0_e32 v6, v0
 ; VI-NEXT:    v_or_b32_e32 v0, v1, v8
 ; VI-NEXT:    v_or_b32_sdwa v1, v5, v2 dst_sel:WORD_1 dst_unused:UNUSED_PAD src0_sel:DWORD src1_sel:DWORD
+; VI-NEXT:    v_or_b32_e32 v2, v3, v4
 ; VI-NEXT:    v_or_b32_e32 v0, v1, v0
-; VI-NEXT:    v_and_b32_e32 v5, 0xffff0000, v0
-; VI-NEXT:    v_or_b32_e32 v4, v4, v5
-; VI-NEXT:    v_cvt_f32_ubyte1_e32 v5, v4
+; VI-NEXT:    v_cvt_f32_ubyte1_e32 v5, v2
+; VI-NEXT:    v_cvt_f32_ubyte0_e32 v4, v2
 ; VI-NEXT:    v_cvt_f32_ubyte3_e32 v3, v0
 ; VI-NEXT:    v_cvt_f32_ubyte2_e32 v2, v0
 ; VI-NEXT:    v_cvt_f32_ubyte1_e32 v1, v0
 ; VI-NEXT:    v_cvt_f32_ubyte0_e32 v0, v0
-; VI-NEXT:    v_cvt_f32_ubyte0_e32 v4, v4
-; VI-NEXT:    buffer_store_dwordx4 v[0:3], off, s[4:7], 0
 ; VI-NEXT:    buffer_store_dwordx3 v[4:6], off, s[4:7], 0 offset:16
+; VI-NEXT:    buffer_store_dwordx4 v[0:3], off, s[4:7], 0
 ; VI-NEXT:    s_endpgm
   %tid = call i32 @llvm.amdgcn.workitem.id.x()
   %gep = getelementptr <7 x i8>, <7 x i8> addrspace(1)* %in, i32 %tid
   %load = load <7 x i8>, <7 x i8> addrspace(1)* %gep, align 1
   %cvt = uitofp <7 x i8> %load to <7 x float>
   store <7 x float> %cvt, <7 x float> addrspace(1)* %out, align 16
   ret void
 }

llvm/test/CodeGen/AMDGPU/max.i16.ll

 ; VI-NEXT:    flat_store_dword v[6:7], v1
 ; VI-NEXT:    s_endpgm
 ;
 ; GFX9-LABEL: v_test_imax_sge_v3i16:
 ; GFX9:       ; %bb.0:
 ; GFX9-NEXT:    s_load_dwordx4 s[4:7], s[0:1], 0x24
 ; GFX9-NEXT:    s_load_dwordx2 s[0:1], s[0:1], 0x34
 ; GFX9-NEXT:    v_lshlrev_b32_e32 v4, 3, v0
+; GFX9-NEXT:    v_mov_b32_e32 v5, 0
+; GFX9-NEXT:    v_mov_b32_e32 v6, 0
 ; GFX9-NEXT:    s_waitcnt lgkmcnt(0)
 ; GFX9-NEXT:    v_mov_b32_e32 v1, s7
 ; GFX9-NEXT:    v_add_co_u32_e32 v0, vcc, s6, v4
 ; GFX9-NEXT:    v_addc_co_u32_e32 v1, vcc, 0, v1, vcc
 ; GFX9-NEXT:    v_mov_b32_e32 v3, s1
 ; GFX9-NEXT:    v_add_co_u32_e32 v2, vcc, s0, v4
 ; GFX9-NEXT:    v_addc_co_u32_e32 v3, vcc, 0, v3, vcc
-; GFX9-NEXT:    global_load_dword v6, v[0:1], off
-; GFX9-NEXT:    global_load_dword v7, v[2:3], off
-; GFX9-NEXT:    v_mov_b32_e32 v5, s5
-; GFX9-NEXT:    v_add_co_u32_e32 v4, vcc, s4, v4
-; GFX9-NEXT:    v_addc_co_u32_e32 v5, vcc, 0, v5, vcc
-; GFX9-NEXT:    s_waitcnt vmcnt(1)
-; GFX9-NEXT:    v_mov_b32_e32 v8, v6
-; GFX9-NEXT:    s_waitcnt vmcnt(0)
-; GFX9-NEXT:    v_mov_b32_e32 v9, v7
-; GFX9-NEXT:    global_load_short_d16 v8, v[0:1], off offset:4
-; GFX9-NEXT:    global_load_short_d16 v9, v[2:3], off offset:4
-; GFX9-NEXT:    v_pk_max_i16 v6, v6, v7
+; GFX9-NEXT:    global_load_short_d16 v5, v[2:3], off offset:4
+; GFX9-NEXT:    global_load_short_d16 v6, v[0:1], off offset:4
+; GFX9-NEXT:    global_load_dword v7, v[0:1], off
+; GFX9-NEXT:    global_load_dword v2, v[2:3], off
+; GFX9-NEXT:    v_mov_b32_e32 v1, s5
+; GFX9-NEXT:    v_add_co_u32_e32 v0, vcc, s4, v4
+; GFX9-NEXT:    v_addc_co_u32_e32 v1, vcc, 0, v1, vcc
+; GFX9-NEXT:    s_waitcnt vmcnt(2)
+; GFX9-NEXT:    v_pk_max_i16 v3, v6, v5
 ; GFX9-NEXT:    s_waitcnt vmcnt(0)
-; GFX9-NEXT:    v_pk_max_i16 v0, v8, v9
-; GFX9-NEXT:    global_store_dword v[4:5], v6, off
-; GFX9-NEXT:    global_store_short v[4:5], v0, off offset:4
+; GFX9-NEXT:    v_pk_max_i16 v2, v7, v2
+; GFX9-NEXT:    global_store_short v[0:1], v3, off offset:4
+; GFX9-NEXT:    global_store_dword v[0:1], v2, off
 ; GFX9-NEXT:    s_endpgm
   %tid = call i32 @llvm.amdgcn.workitem.id.x() nounwind readnone
   %gep0 = getelementptr <3 x i16>, <3 x i16> addrspace(1)* %aptr, i32 %tid
   %gep1 = getelementptr <3 x i16>, <3 x i16> addrspace(1)* %bptr, i32 %tid
   %outgep = getelementptr <3 x i16>, <3 x i16> addrspace(1)* %out, i32 %tid
   %a = load <3 x i16>, <3 x i16> addrspace(1)* %gep0, align 4
   %b = load <3 x i16>, <3 x i16> addrspace(1)* %gep1, align 4
   %cmp = icmp sge <3 x i16> %a, %b