This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU/GlobalISel: Widen s1 SGPR constants during regbankselect
ClosedPublic

Authored by arsenm on Jan 6 2023, 5:31 AM.

Details

Reviewers
foad
Pierre-vh
mbrkusanin
Petar.Avramovic
Group Reviewers
Restricted Project
Summary

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.

Diff Detail

Event Timeline

arsenm created this revision.Jan 6 2023, 5:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2023, 5:31 AM
arsenm requested review of this revision.Jan 6 2023, 5:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2023, 5:31 AM
Herald added a subscriber: wdng. · View Herald Transcript
Pierre-vh added inline comments.Jan 6 2023, 5:55 AM
llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp
2143

Small nit: Can we put this on a separate line to reduce nesting? e.g.
const auto Val = MI.getOperand(1).getCImm()->getZExtValue()

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
Can you paste the pre-change ISA in a comment so I can see the before/after?

arsenm added inline comments.Jan 6 2023, 6:48 AM
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

arsenm marked an inline comment as done.Jan 6 2023, 5:21 PM
arsenm added inline comments.
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

arsenm updated this revision to Diff 487021.Jan 6 2023, 5:51 PM

Separateconstant var

Pierre-vh accepted this revision.Jan 9 2023, 12:14 AM

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?

This revision is now accepted and ready to land.Jan 9 2023, 12:14 AM