To unambiguously interpret these as 32-bit SGPRs, we need to widen
these to s32. This was selecting to a copy from a 64-bit SGPR to a
32-bit SGPR for wave64.
Details
- Reviewers
foad Pierre-vh mbrkusanin Petar.Avramovic - Group Reviewers
Restricted Project
Diff Detail
Event Timeline
llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp | ||
---|---|---|
2143 | Small nit: Can we put this on a separate line to reduce nesting? e.g. | |
llvm/test/CodeGen/AMDGPU/GlobalISel/bool-legalization.ll | ||
71 | If I understand correctly, it was interpreted as 1 on all lanes before (so -1 as a 32 bit value) but now we're widening the constant so it's just 1, and the end result is the same? | |
llvm/test/CodeGen/AMDGPU/GlobalISel/br-constant-invalid-sgpr-copy.ll | ||
6 | It would have been nice to have the test pre-committed to see exactly what has been fixed, but it's a small test/diff so I'm not going to ask to split it |
llvm/test/CodeGen/AMDGPU/GlobalISel/bool-legalization.ll | ||
---|---|---|
71 | The next instruction is an s_and_b32 1 to clear the high bits. We're missing simplify known bits anyway |
llvm/test/CodeGen/AMDGPU/GlobalISel/br-constant-invalid-sgpr-copy.ll | ||
---|---|---|
6 | It failed to compile before (for wave64, it happens to work on wave32). The selector is too permissive, which I'm planning on fixing separately |
LGTM but I didn't fully check the .i64 tests. Otherwise the change looks good to me and makes sense.
Did you check that this was working as intended on some conformance test/benchmark?
Small nit: Can we put this on a separate line to reduce nesting? e.g.
const auto Val = MI.getOperand(1).getCImm()->getZExtValue()