[AMDGPU] Fix V_INDIRECT_REG_READ_GPR_IDX expansion with immediate index

[kurapov-peter]

The definition for V_INDIRECT_REG_READ_GPR_IDX_B32_V*'s SSrc_b32 operand allows immediates, but the expansion logic handles only register cases now. This can result in expansion failures when e.g. llvm.amdgcn.wave.reduce.umin.i32 is folded into a constant and then used as an insertelement idx.

[llvmbot]

@llvm/pr-subscribers-backend-amdgpu

Author: Petr Kurapov (kurapov-peter)

Changes

The definition for V_INDIRECT_REG_READ_GPR_IDX_B32_V*'s SSrc_b32 operand allows immediates, but the expansion logic handles only register cases now. This can result in expansion failures when e.g. llvm.amdgcn.wave.reduce.umin.i32 is folded into a constant and then used as an insertelement idx.

---

Full diff: https://github.com/llvm/llvm-project/pull/179699.diff

1 Files Affected:

• (modified) llvm/lib/Target/AMDGPU/SIInstrInfo.cpp (+1-2)

diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
index 9e3b683d10c45..581391d3d83db 100644
--- a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
@@ -2410,11 +2410,10 @@ bool SIInstrInfo::expandPostRAPseudo(MachineInstr &MI) const {
     Register Dst = MI.getOperand(0).getReg();
     Register VecReg = MI.getOperand(1).getReg();
     bool IsUndef = MI.getOperand(1).isUndef();
-    Register Idx = MI.getOperand(2).getReg();
     Register SubReg = MI.getOperand(3).getImm();
 
     MachineInstr *SetOn = BuildMI(MBB, MI, DL, get(AMDGPU::S_SET_GPR_IDX_ON))
-                              .addReg(Idx)
+                              .add(MI.getOperand(2))
                               .addImm(AMDGPU::VGPRIndexMode::SRC0_ENABLE);
     SetOn->getOperand(3).setIsUndef();
 

[github-actions]

🐧 Linux x64 Test Results

• 189668 tests passed
• 5049 tests skipped

✅ The build succeeded and all tests passed.

[arsenm]

Needs tests

[kurapov-peter]

Needs tests

Is there something specific you have in mind? The change is trivial, so I decided it doesn't deserve a test.

[arsenm]

Is there something specific you have in mind?

An IR and/or MIR test that would have asserted before

so I decided it doesn't deserve a test.

No, every functional change needs a test.

[arsenm]

Do you actually need -O1 or -global-isel to reproduce? If not remove both

[kurapov-peter]

yeah, I do need both

[arsenm]

If it's so fragile it depends on -O1 -global-isel, might be worth adding a mir test

[arsenm]

Suggested change

	; RUN: llc -mtriple=amdgcn -mcpu=gfx90a -O1 -global-isel=true < %s | FileCheck %s
	; RUN: llc -mtriple=amdgcn -mcpu=gfx90a -O1 -global-isel < %s | FileCheck %s