This is an archive of the discontinued LLVM Phabricator instance.

Differential D51589

DAG: Fold extract_vector_elt (scalar_to_vector), K to undef
ClosedPublic

Authored by RKSimon on Sep 3 2018, 5:43 AM.

Details

Reviewers
arsenm
Commits
rG5f5f47856489: [DAG] Fold extract_vector_elt (scalar_to_vector), K to undef (K != 0)
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.
Diff Detail
Repository 
rL LLVM 
Event Timeline
arsenm created this revision.Sep 3 2018, 5:43 AM
Herald added a subscriber: wdng.  ·  View Herald TranscriptSep 3 2018, 5:43 AM
RKSimon added a comment.Sep 8 2018, 7:17 AM
Avoids regressions in a future commit.



Do you have the patch available to show the test case?
arsenm added a comment.Sep 9 2018, 6:54 PM


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 added a comment.Sep 12 2018, 5:10 AM


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 added a comment.Feb 21 2019, 5:46 PM
I think this should be done, but I have a hard time coming up with AMDGPU test cases that actually use SCALAR_TO_VECTOR
Herald added a subscriber: jdoerfert.  ·  View Herald TranscriptFeb 21 2019, 5:46 PM
RKSimon added a comment.Feb 22 2019, 4:06 AM
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 added a comment.Mar 16 2019, 10:29 AM
@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 added a comment.Mar 16 2019, 4:20 PM


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 mentioned this in rL356340: [AMDGPU] Regenerate some f16/i16 tests..Mar 17 2019, 1:36 PM
RKSimon mentioned this in rG10ba65cc48f9: [AMDGPU] Regenerate some f16/i16 tests..
RKSimon commandeered this revision.Mar 17 2019, 1:54 PM
RKSimon updated this revision to Diff 191043.
RKSimon edited reviewers, added: arsenm; removed: RKSimon.
Finally got a test change, and its an increase in instructions....
Herald added a project: Restricted Project.  ·  View Herald TranscriptMar 17 2019, 1:55 PM
Herald added subscribers: nhaehnle, jvesely.  ·  View Herald Transcript
arsenm added a comment.Mar 18 2019, 7:53 AM
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 updated this revision to Diff 221327.Sep 23 2019, 7:33 AM
rebase - still need to fix the extract subvector regression
RKSimon updated this revision to Diff 239057.Jan 20 2020, 3:28 AM
rebase
Herald added subscribers: kerbowa, hiraditya.  ·  View Herald TranscriptJan 20 2020, 3:28 AM
RKSimon added a comment.Jan 20 2020, 3:37 AM
@arsenm The subvector regression is still there but the actual instruction count has reduced
arsenm accepted this revision.Jan 20 2020, 2:04 PM
LGTM
llvm/test/CodeGen/AMDGPU/max.i16.ll


163–164 ↗(On Diff #239057)
I'm pretty sure these zeros are dead


173–175 ↗(On Diff #239057)
I think this for some reason just got luckier on scheduling
This revision is now accepted and ready to land.Jan 20 2020, 2:04 PM
Closed by commit rG5f5f47856489: [DAG] Fold extract_vector_elt (scalar_to_vector), K to undef (K != 0) (authored by RKSimon).  ·  Explain WhyJan 21 2020, 3:05 AM
This revision was automatically updated to reflect the committed changes.
Revision Contents
PathSize
lib/
CodeGen/
SelectionDAG/
DAGCombiner.cpp
DAGCombiner.cpp	(revision 356330)

4 lines
test/
CodeGen/
AMDGPU/
max.i16.ll
max.i16.ll	(revision 356340)

21 lines
Diff 191043
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 16,011 Lines • ▼ Show 20 LinesSDValue DAGCombiner::visitEXTRACT_VECTOR_ELT(SDNode *N) {

  if (VecOp.getOpcode() == ISD::INSERT_VECTOR_ELT &&
  if (VecOp.getOpcode() == ISD::INSERT_VECTOR_ELT &&

      Index == VecOp.getOperand(2)) {
      Index == VecOp.getOperand(2)) {

    SDValue Elt = VecOp.getOperand(1);
    SDValue Elt = VecOp.getOperand(1);

    return VecVT.isInteger() ? DAG.getAnyExtOrTrunc(Elt, DL, ScalarVT) : Elt;
    return VecVT.isInteger() ? DAG.getAnyExtOrTrunc(Elt, DL, ScalarVT) : Elt;

  }
  }




  // (vextract (scalar_to_vector val, 0) -> val
  // (vextract (scalar_to_vector val, 0) -> val

  if (VecOp.getOpcode() == ISD::SCALAR_TO_VECTOR) {
  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
    // Check if the result type doesn't match the inserted element type. A

    // SCALAR_TO_VECTOR may truncate the inserted element and the
    // SCALAR_TO_VECTOR may truncate the inserted element and the

    // EXTRACT_VECTOR_ELT may widen the extracted vector.
    // EXTRACT_VECTOR_ELT may widen the extracted vector.

    SDValue InOp = VecOp.getOperand(0);
    SDValue InOp = VecOp.getOperand(0);

    if (InOp.getValueType() != ScalarVT) {
    if (InOp.getValueType() != ScalarVT) {

      assert(InOp.getValueType().isInteger() && ScalarVT.isInteger());
      assert(InOp.getValueType().isInteger() && ScalarVT.isInteger());

      return DAG.getSExtOrTrunc(InOp, DL, ScalarVT);
      return DAG.getSExtOrTrunc(InOp, DL, ScalarVT);

    }
    }

▲ Show 20 Lines • Show All 3,726 Lines • Show Last 20 Lines
test/CodeGen/AMDGPU/max.i16.ll
Show First 20 Lines • Show All 154 Lines • ▼ Show 20 Lines
; VI-NEXT:    flat_store_short v[2:3], v4
; VI-NEXT:    flat_store_short v[2:3], v4

; VI-NEXT:    s_endpgm
; VI-NEXT:    s_endpgm

;
;

; GFX9-LABEL: v_test_imax_sge_v3i16:
; GFX9-LABEL: v_test_imax_sge_v3i16:

; GFX9:       ; %bb.0:
; GFX9:       ; %bb.0:

; GFX9-NEXT:    s_load_dwordx4 s[4:7], s[0:1], 0x24
; 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:    s_load_dwordx2 s[0:1], s[0:1], 0x34

; GFX9-NEXT:    v_lshlrev_b32_e32 v4, 3, v0
; GFX9-NEXT:    v_lshlrev_b32_e32 v4, 3, v0

; GFX9-NEXT:    v_mov_b32_e32 v6, 0

; GFX9-NEXT:    v_mov_b32_e32 v7, 0

; GFX9-NEXT:    s_waitcnt lgkmcnt(0)
; GFX9-NEXT:    s_waitcnt lgkmcnt(0)

; GFX9-NEXT:    v_mov_b32_e32 v1, s7
; GFX9-NEXT:    v_mov_b32_e32 v1, s7

; GFX9-NEXT:    v_add_co_u32_e32 v0, vcc, s6, v4
; 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_addc_co_u32_e32 v1, vcc, 0, v1, vcc

; GFX9-NEXT:    v_mov_b32_e32 v3, s1
; GFX9-NEXT:    v_mov_b32_e32 v3, s1

; GFX9-NEXT:    v_add_co_u32_e32 v2, vcc, s0, v4
; 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:    v_addc_co_u32_e32 v3, vcc, 0, v3, vcc

; GFX9-NEXT:    global_load_dword v6, v[2:3], off
; GFX9-NEXT:    global_load_short_d16 v7, v[0:1], off offset:4

; GFX9-NEXT:    global_load_dword v7, v[0:1], off
; GFX9-NEXT:    global_load_dword v8, v[2:3], off

; GFX9-NEXT:    global_load_dword v0, v[0:1], off

; GFX9-NEXT:    global_load_short_d16 v6, v[2:3], off offset:4

; GFX9-NEXT:    v_mov_b32_e32 v5, s5
; GFX9-NEXT:    v_mov_b32_e32 v5, s5

; GFX9-NEXT:    v_add_co_u32_e32 v4, vcc, s4, v4
; 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:    v_addc_co_u32_e32 v5, vcc, 0, v5, vcc

; GFX9-NEXT:    s_waitcnt vmcnt(1)

; GFX9-NEXT:    v_pk_max_i16 v0, v0, v8

; GFX9-NEXT:    s_waitcnt vmcnt(0)
; GFX9-NEXT:    s_waitcnt vmcnt(0)

; GFX9-NEXT:    v_mov_b32_e32 v8, v7
; GFX9-NEXT:    v_pk_max_i16 v1, v7, v6

; GFX9-NEXT:    v_pk_max_i16 v7, v7, v6
; GFX9-NEXT:    global_store_dword v[4:5], v0, off

; GFX9-NEXT:    global_load_short_d16 v6, v[2:3], off offset:4
; GFX9-NEXT:    global_store_short v[4:5], v1, off offset:4

; GFX9-NEXT:    global_load_short_d16 v8, v[0:1], off offset:4

; GFX9-NEXT:    s_waitcnt vmcnt(0)

; GFX9-NEXT:    v_pk_max_i16 v0, v8, v6

; GFX9-NEXT:    global_store_dword v[4:5], v7, off

; GFX9-NEXT:    global_store_short v[4:5], v0, off offset:4

; GFX9-NEXT:    s_endpgm
; GFX9-NEXT:    s_endpgm

  %tid = call i32 @llvm.amdgcn.workitem.id.x() nounwind readnone
  %tid = call i32 @llvm.amdgcn.workitem.id.x() nounwind readnone

  %gep0 = getelementptr <3 x i16>, <3 x i16> addrspace(1)* %aptr, i32 %tid
  %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
  %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
  %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
  %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
  %b = load <3 x i16>, <3 x i16> addrspace(1)* %gep1, align 4

  %cmp = icmp sge <3 x i16> %a, %b
  %cmp = icmp sge <3 x i16> %a, %b

▲ Show 20 Lines • Show All 298 Lines • Show Last 20 Lines