From c91a01c555144e871480eff722fc1f68b5fcb732 Mon Sep 17 00:00:00 2001 From: Matt Arsenault Date: Mon, 18 Aug 2025 16:17:07 +0900 Subject: [PATCH 1/3] AMDGPU: Fix using illegal extract_subvector indexes --- llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp | 28 ++++- llvm/test/CodeGen/AMDGPU/load-constant-i32.ll | 106 +++++++++--------- 2 files changed, 77 insertions(+), 57 deletions(-) diff --git a/llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp b/llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp index 64e68ab7d753c..9751909f7be70 100644 --- a/llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp +++ b/llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp @@ -1802,15 +1802,35 @@ std::pair AMDGPUTargetLowering::splitVector(const SDValue &N, const SDLoc &DL, const EVT &LoVT, const EVT &HiVT, SelectionDAG &DAG) const { + EVT VT = N.getValueType(); assert(LoVT.getVectorNumElements() + (HiVT.isVector() ? HiVT.getVectorNumElements() : 1) <= - N.getValueType().getVectorNumElements() && + VT.getVectorNumElements() && "More vector elements requested than available!"); SDValue Lo = DAG.getNode(ISD::EXTRACT_SUBVECTOR, DL, LoVT, N, DAG.getVectorIdxConstant(0, DL)); - SDValue Hi = DAG.getNode( - HiVT.isVector() ? ISD::EXTRACT_SUBVECTOR : ISD::EXTRACT_VECTOR_ELT, DL, - HiVT, N, DAG.getVectorIdxConstant(LoVT.getVectorNumElements(), DL)); + + unsigned LoNumElts = LoVT.getVectorNumElements(); + + if (HiVT.isVector()) { + unsigned HiNumElts = HiVT.getVectorNumElements(); + if ((VT.getVectorNumElements() % HiNumElts) == 0) { + // Avoid creating an extract_subvector with an index that isn't a multiple + // of the result type. + SDValue Hi = DAG.getNode(ISD::EXTRACT_SUBVECTOR, DL, HiVT, N, + DAG.getConstant(LoNumElts, DL, MVT::i32)); + return {Lo, Hi}; + } + + SmallVector Elts; + DAG.ExtractVectorElements(N, Elts, /*Start=*/LoNumElts, + /*Count=*/HiNumElts); + SDValue Hi = DAG.getBuildVector(HiVT, DL, Elts); + return {Lo, Hi}; + } + + SDValue Hi = DAG.getNode(ISD::EXTRACT_VECTOR_ELT, DL, HiVT, N, + DAG.getVectorIdxConstant(LoNumElts, DL)); return std::pair(Lo, Hi); } diff --git a/llvm/test/CodeGen/AMDGPU/load-constant-i32.ll b/llvm/test/CodeGen/AMDGPU/load-constant-i32.ll index 0a938b0d2297d..8862cbe6391ea 100644 --- a/llvm/test/CodeGen/AMDGPU/load-constant-i32.ll +++ b/llvm/test/CodeGen/AMDGPU/load-constant-i32.ll @@ -872,66 +872,66 @@ define amdgpu_kernel void @constant_load_v11i32(ptr addrspace(1) %out, ptr addrs ; GFX7-HSA-NEXT: s_mov_b32 flat_scratch_lo, s13 ; GFX7-HSA-NEXT: s_lshr_b32 flat_scratch_hi, s12, 8 ; GFX7-HSA-NEXT: s_waitcnt lgkmcnt(0) -; GFX7-HSA-NEXT: s_load_dwordx4 s[12:15], s[10:11], 0x8 ; GFX7-HSA-NEXT: s_load_dwordx8 s[0:7], s[10:11], 0x0 +; GFX7-HSA-NEXT: s_load_dwordx4 s[12:15], s[10:11], 0x8 ; GFX7-HSA-NEXT: s_add_u32 s10, s8, 16 ; GFX7-HSA-NEXT: s_addc_u32 s11, s9, 0 -; GFX7-HSA-NEXT: v_mov_b32_e32 v7, s10 -; GFX7-HSA-NEXT: v_mov_b32_e32 v8, s11 +; GFX7-HSA-NEXT: v_mov_b32_e32 v5, s10 +; GFX7-HSA-NEXT: v_mov_b32_e32 v6, s11 ; GFX7-HSA-NEXT: s_waitcnt lgkmcnt(0) ; GFX7-HSA-NEXT: v_mov_b32_e32 v0, s4 ; GFX7-HSA-NEXT: v_mov_b32_e32 v1, s5 ; GFX7-HSA-NEXT: v_mov_b32_e32 v2, s6 ; GFX7-HSA-NEXT: v_mov_b32_e32 v3, s7 -; GFX7-HSA-NEXT: flat_store_dwordx4 v[7:8], v[0:3] -; GFX7-HSA-NEXT: v_mov_b32_e32 v7, s8 -; GFX7-HSA-NEXT: v_mov_b32_e32 v0, s0 -; GFX7-HSA-NEXT: v_mov_b32_e32 v1, s1 -; GFX7-HSA-NEXT: v_mov_b32_e32 v2, s2 -; GFX7-HSA-NEXT: v_mov_b32_e32 v3, s3 -; GFX7-HSA-NEXT: v_mov_b32_e32 v8, s9 +; GFX7-HSA-NEXT: v_mov_b32_e32 v4, s0 +; GFX7-HSA-NEXT: flat_store_dwordx4 v[5:6], v[0:3] ; GFX7-HSA-NEXT: s_add_u32 s0, s8, 32 -; GFX7-HSA-NEXT: flat_store_dwordx4 v[7:8], v[0:3] +; GFX7-HSA-NEXT: v_mov_b32_e32 v0, s8 +; GFX7-HSA-NEXT: v_mov_b32_e32 v5, s1 +; GFX7-HSA-NEXT: v_mov_b32_e32 v6, s2 +; GFX7-HSA-NEXT: v_mov_b32_e32 v7, s3 +; GFX7-HSA-NEXT: v_mov_b32_e32 v1, s9 ; GFX7-HSA-NEXT: s_addc_u32 s1, s9, 0 -; GFX7-HSA-NEXT: v_mov_b32_e32 v0, s0 -; GFX7-HSA-NEXT: v_mov_b32_e32 v4, s12 -; GFX7-HSA-NEXT: v_mov_b32_e32 v5, s13 -; GFX7-HSA-NEXT: v_mov_b32_e32 v6, s14 -; GFX7-HSA-NEXT: v_mov_b32_e32 v1, s1 -; GFX7-HSA-NEXT: flat_store_dwordx3 v[0:1], v[4:6] +; GFX7-HSA-NEXT: flat_store_dwordx4 v[0:1], v[4:7] +; GFX7-HSA-NEXT: v_mov_b32_e32 v0, s12 +; GFX7-HSA-NEXT: v_mov_b32_e32 v4, s1 +; GFX7-HSA-NEXT: v_mov_b32_e32 v1, s13 +; GFX7-HSA-NEXT: v_mov_b32_e32 v2, s14 +; GFX7-HSA-NEXT: v_mov_b32_e32 v3, s0 +; GFX7-HSA-NEXT: flat_store_dwordx3 v[3:4], v[0:2] ; GFX7-HSA-NEXT: s_endpgm ; ; GFX8-NOHSA-LABEL: constant_load_v11i32: ; GFX8-NOHSA: ; %bb.0: ; %entry ; GFX8-NOHSA-NEXT: s_load_dwordx4 s[0:3], s[4:5], 0x24 ; GFX8-NOHSA-NEXT: s_waitcnt lgkmcnt(0) -; GFX8-NOHSA-NEXT: s_load_dwordx4 s[12:15], s[2:3], 0x20 ; GFX8-NOHSA-NEXT: s_load_dwordx8 s[4:11], s[2:3], 0x0 +; GFX8-NOHSA-NEXT: s_load_dwordx4 s[12:15], s[2:3], 0x20 ; GFX8-NOHSA-NEXT: s_add_u32 s2, s0, 16 ; GFX8-NOHSA-NEXT: s_addc_u32 s3, s1, 0 -; GFX8-NOHSA-NEXT: v_mov_b32_e32 v8, s3 -; GFX8-NOHSA-NEXT: v_mov_b32_e32 v7, s2 +; GFX8-NOHSA-NEXT: v_mov_b32_e32 v6, s3 +; GFX8-NOHSA-NEXT: v_mov_b32_e32 v5, s2 ; GFX8-NOHSA-NEXT: s_waitcnt lgkmcnt(0) ; GFX8-NOHSA-NEXT: v_mov_b32_e32 v0, s8 ; GFX8-NOHSA-NEXT: v_mov_b32_e32 v1, s9 ; GFX8-NOHSA-NEXT: v_mov_b32_e32 v2, s10 ; GFX8-NOHSA-NEXT: v_mov_b32_e32 v3, s11 -; GFX8-NOHSA-NEXT: flat_store_dwordx4 v[7:8], v[0:3] -; GFX8-NOHSA-NEXT: v_mov_b32_e32 v8, s1 -; GFX8-NOHSA-NEXT: v_mov_b32_e32 v0, s4 -; GFX8-NOHSA-NEXT: v_mov_b32_e32 v1, s5 -; GFX8-NOHSA-NEXT: v_mov_b32_e32 v2, s6 -; GFX8-NOHSA-NEXT: v_mov_b32_e32 v3, s7 -; GFX8-NOHSA-NEXT: v_mov_b32_e32 v7, s0 -; GFX8-NOHSA-NEXT: s_add_u32 s0, s0, 32 -; GFX8-NOHSA-NEXT: flat_store_dwordx4 v[7:8], v[0:3] -; GFX8-NOHSA-NEXT: s_addc_u32 s1, s1, 0 +; GFX8-NOHSA-NEXT: flat_store_dwordx4 v[5:6], v[0:3] +; GFX8-NOHSA-NEXT: v_mov_b32_e32 v4, s4 ; GFX8-NOHSA-NEXT: v_mov_b32_e32 v0, s0 -; GFX8-NOHSA-NEXT: v_mov_b32_e32 v4, s12 -; GFX8-NOHSA-NEXT: v_mov_b32_e32 v5, s13 -; GFX8-NOHSA-NEXT: v_mov_b32_e32 v6, s14 ; GFX8-NOHSA-NEXT: v_mov_b32_e32 v1, s1 -; GFX8-NOHSA-NEXT: flat_store_dwordx3 v[0:1], v[4:6] +; GFX8-NOHSA-NEXT: s_add_u32 s0, s0, 32 +; GFX8-NOHSA-NEXT: v_mov_b32_e32 v5, s5 +; GFX8-NOHSA-NEXT: v_mov_b32_e32 v6, s6 +; GFX8-NOHSA-NEXT: v_mov_b32_e32 v7, s7 +; GFX8-NOHSA-NEXT: s_addc_u32 s1, s1, 0 +; GFX8-NOHSA-NEXT: flat_store_dwordx4 v[0:1], v[4:7] +; GFX8-NOHSA-NEXT: v_mov_b32_e32 v0, s12 +; GFX8-NOHSA-NEXT: v_mov_b32_e32 v4, s1 +; GFX8-NOHSA-NEXT: v_mov_b32_e32 v1, s13 +; GFX8-NOHSA-NEXT: v_mov_b32_e32 v2, s14 +; GFX8-NOHSA-NEXT: v_mov_b32_e32 v3, s0 +; GFX8-NOHSA-NEXT: flat_store_dwordx3 v[3:4], v[0:2] ; GFX8-NOHSA-NEXT: s_endpgm ; ; EG-LABEL: constant_load_v11i32: @@ -969,25 +969,25 @@ define amdgpu_kernel void @constant_load_v11i32(ptr addrspace(1) %out, ptr addrs ; GFX9-HSA-LABEL: constant_load_v11i32: ; GFX9-HSA: ; %bb.0: ; %entry ; GFX9-HSA-NEXT: s_load_dwordx4 s[8:11], s[8:9], 0x0 -; GFX9-HSA-NEXT: v_mov_b32_e32 v7, 0 +; GFX9-HSA-NEXT: v_mov_b32_e32 v8, 0 ; GFX9-HSA-NEXT: s_waitcnt lgkmcnt(0) -; GFX9-HSA-NEXT: s_load_dwordx4 s[12:15], s[10:11], 0x20 ; GFX9-HSA-NEXT: s_load_dwordx8 s[0:7], s[10:11], 0x0 +; GFX9-HSA-NEXT: s_load_dwordx4 s[12:15], s[10:11], 0x20 ; GFX9-HSA-NEXT: s_waitcnt lgkmcnt(0) -; GFX9-HSA-NEXT: v_mov_b32_e32 v4, s12 ; GFX9-HSA-NEXT: v_mov_b32_e32 v0, s4 ; GFX9-HSA-NEXT: v_mov_b32_e32 v1, s5 ; GFX9-HSA-NEXT: v_mov_b32_e32 v2, s6 ; GFX9-HSA-NEXT: v_mov_b32_e32 v3, s7 -; GFX9-HSA-NEXT: global_store_dwordx4 v7, v[0:3], s[8:9] offset:16 -; GFX9-HSA-NEXT: v_mov_b32_e32 v5, s13 -; GFX9-HSA-NEXT: v_mov_b32_e32 v0, s0 -; GFX9-HSA-NEXT: v_mov_b32_e32 v1, s1 -; GFX9-HSA-NEXT: v_mov_b32_e32 v2, s2 -; GFX9-HSA-NEXT: v_mov_b32_e32 v3, s3 -; GFX9-HSA-NEXT: v_mov_b32_e32 v6, s14 -; GFX9-HSA-NEXT: global_store_dwordx4 v7, v[0:3], s[8:9] -; GFX9-HSA-NEXT: global_store_dwordx3 v7, v[4:6], s[8:9] offset:32 +; GFX9-HSA-NEXT: v_mov_b32_e32 v4, s0 +; GFX9-HSA-NEXT: v_mov_b32_e32 v5, s1 +; GFX9-HSA-NEXT: v_mov_b32_e32 v6, s2 +; GFX9-HSA-NEXT: global_store_dwordx4 v8, v[0:3], s[8:9] offset:16 +; GFX9-HSA-NEXT: v_mov_b32_e32 v7, s3 +; GFX9-HSA-NEXT: v_mov_b32_e32 v0, s12 +; GFX9-HSA-NEXT: v_mov_b32_e32 v1, s13 +; GFX9-HSA-NEXT: v_mov_b32_e32 v2, s14 +; GFX9-HSA-NEXT: global_store_dwordx4 v8, v[4:7], s[8:9] +; GFX9-HSA-NEXT: global_store_dwordx3 v8, v[0:2], s[8:9] offset:32 ; GFX9-HSA-NEXT: s_endpgm ; ; GFX12-LABEL: constant_load_v11i32: @@ -995,19 +995,19 @@ define amdgpu_kernel void @constant_load_v11i32(ptr addrspace(1) %out, ptr addrs ; GFX12-NEXT: s_load_b128 s[8:11], s[4:5], 0x24 ; GFX12-NEXT: s_wait_kmcnt 0x0 ; GFX12-NEXT: s_clause 0x1 -; GFX12-NEXT: s_load_b96 s[12:14], s[10:11], 0x20 ; GFX12-NEXT: s_load_b256 s[0:7], s[10:11], 0x0 +; GFX12-NEXT: s_load_b96 s[12:14], s[10:11], 0x20 ; GFX12-NEXT: s_wait_kmcnt 0x0 -; GFX12-NEXT: v_dual_mov_b32 v11, 0 :: v_dual_mov_b32 v8, s12 +; GFX12-NEXT: v_dual_mov_b32 v11, 0 :: v_dual_mov_b32 v0, s4 +; GFX12-NEXT: v_dual_mov_b32 v1, s5 :: v_dual_mov_b32 v2, s6 +; GFX12-NEXT: v_dual_mov_b32 v3, s7 :: v_dual_mov_b32 v4, s0 +; GFX12-NEXT: v_dual_mov_b32 v5, s1 :: v_dual_mov_b32 v6, s2 +; GFX12-NEXT: v_dual_mov_b32 v7, s3 :: v_dual_mov_b32 v8, s12 ; GFX12-NEXT: v_dual_mov_b32 v9, s13 :: v_dual_mov_b32 v10, s14 -; GFX12-NEXT: v_dual_mov_b32 v0, s4 :: v_dual_mov_b32 v1, s5 -; GFX12-NEXT: v_dual_mov_b32 v2, s6 :: v_dual_mov_b32 v3, s7 -; GFX12-NEXT: v_dual_mov_b32 v4, s0 :: v_dual_mov_b32 v5, s1 -; GFX12-NEXT: v_dual_mov_b32 v6, s2 :: v_dual_mov_b32 v7, s3 ; GFX12-NEXT: s_clause 0x2 -; GFX12-NEXT: global_store_b96 v11, v[8:10], s[8:9] offset:32 ; GFX12-NEXT: global_store_b128 v11, v[0:3], s[8:9] offset:16 ; GFX12-NEXT: global_store_b128 v11, v[4:7], s[8:9] +; GFX12-NEXT: global_store_b96 v11, v[8:10], s[8:9] offset:32 ; GFX12-NEXT: s_endpgm entry: %ld = load <11 x i32>, ptr addrspace(4) %in From ce7657bcf790793b2c01e4c295b19bf9c54de7b9 Mon Sep 17 00:00:00 2001 From: Matt Arsenault Date: Mon, 18 Aug 2025 22:22:39 +0900 Subject: [PATCH 2/3] initializer list throughout splitVector --- llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp b/llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp index 9751909f7be70..83bc8e5eaef8e 100644 --- a/llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp +++ b/llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp @@ -1831,7 +1831,7 @@ AMDGPUTargetLowering::splitVector(const SDValue &N, const SDLoc &DL, SDValue Hi = DAG.getNode(ISD::EXTRACT_VECTOR_ELT, DL, HiVT, N, DAG.getVectorIdxConstant(LoNumElts, DL)); - return std::pair(Lo, Hi); + return {Lo, Hi}; } SDValue AMDGPUTargetLowering::SplitVectorLoad(const SDValue Op, From 3fd9c57c7993fb1ad2651a6fe8681cda289e3e27 Mon Sep 17 00:00:00 2001 From: Matt Arsenault Date: Mon, 18 Aug 2025 13:22:26 +0900 Subject: [PATCH 3/3] DAG: Add assert to getNode for EXTRACT_SUBVECTOR indexes Verify it's a multiple of the result vector element count instead of asserting this in random combines. The testcase in #153808 fails in the wrong point. Add an assert to getNode so the invalid extract asserts at construction instead of use. --- llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp | 2 -- llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp | 2 ++ 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp index 43d4138df8b49..9aae043f822a3 100644 --- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp +++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp @@ -26091,8 +26091,6 @@ SDValue DAGCombiner::visitEXTRACT_SUBVECTOR(SDNode *N) { EVT ConcatSrcVT = V.getOperand(0).getValueType(); assert(ConcatSrcVT.getVectorElementType() == NVT.getVectorElementType() && "Concat and extract subvector do not change element type"); - assert((ExtIdx % ExtNumElts) == 0 && - "Extract index is not a multiple of the input vector length."); unsigned ConcatSrcNumElts = ConcatSrcVT.getVectorMinNumElements(); unsigned ConcatOpIdx = ExtIdx / ConcatSrcNumElts; diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp index 84282d8a1c37b..fadf2c7a4b9bc 100644 --- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp +++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp @@ -7956,6 +7956,8 @@ SDValue SelectionDAG::getNode(unsigned Opcode, const SDLoc &DL, EVT VT, assert(N2C->getAPIntValue().getBitWidth() == TLI->getVectorIdxWidth(getDataLayout()) && "Constant index for EXTRACT_SUBVECTOR has an invalid size"); + assert(N2C->getZExtValue() % VT.getVectorMinNumElements() == 0 && + "Extract index is not a multiple of the output vector length"); // Trivial extraction. if (VT == N1VT)