This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU/GlobalISel: Add stub custom regbankselect pass
ClosedPublic

Authored by arsenm on Jan 18 2023, 7:21 AM.

Details

Summary

Uniformity analysis needs to be the fundamental basis for
regbank decisions. The considerations of the default pass
are secondary, but potentially useful for some edge cases (e.g.
selecting AGPRs when arbitrary loads and stores can directly use
them). This needs to be a separate pass since it requires new
analysis dependencies.

Boilerplate to subclass the existing pass which does nothing
different.

Diff Detail

Event Timeline

arsenm created this revision.Jan 18 2023, 7:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 18 2023, 7:21 AM
arsenm requested review of this revision.Jan 18 2023, 7:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 18 2023, 7:21 AM
Herald added a subscriber: wdng. · View Herald Transcript
rovka added a subscriber: rovka.Jan 19 2023, 1:36 AM

Why do we need a new pass? Could we achieve the same goals with a new RegBankSelect mode instead? (Sorry if this was discussed somewhere else and I missed it, in any case I think the commit message should either explain the rationale or point to wherever it's been discussed)

llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp
71–72

Where is PassID used?

llvm/lib/Target/AMDGPU/AMDGPURegBankSelect.cpp
15

Nit: This diverges from AMDGPUInstructionSelector and Legalizer, which both define the DEBUG_TYPE as "amdgpu-blah" directly.

llvm/lib/Target/AMDGPU/AMDGPURegBankSelect.h
11

Update this comment for RegBankSelect instead of InstructionSelector (ditto for the header guard)

I agree that the commit message should have a rationale for this change

Also, now that we need to pass amdgpu-regbankselect, what happens if someone just passes regbankselect? 💥 ?

llvm/lib/Target/AMDGPU/AMDGPURegBankSelect.h
11

It would be nice to explain why this custom pass is actually needed. If it's a stub then just say it's a stub but it's been added because it makes X or Y possible later for instance;
Otherwise it's difficult to understand why it's needed and someone might try to remove it later

Why do we need a new pass? Could we achieve the same goals with a new RegBankSelect mode instead? (Sorry if this was discussed somewhere else and I missed it, in any case I think the commit message should either explain the rationale or point to wherever it's been discussed)

I believe the mode is an orthogonal choice, which is part of why I am trying to subclass the existing pass. We could have the same use and def analysis like other targets with the different modes, but the uniformity driven decisions are more fundamental. Additionally this needs to be a separate pass because it introduces new required analysis passes

I agree that the commit message should have a rationale for this change

Also, now that we need to pass amdgpu-regbankselect, what happens if someone just passes regbankselect? 💥 ?

It will just be broken in the same way it’s broken today. Running arbitrary MIR passes isn’t supposed to be a production ready, end user exposed feature in the same way as an IR pass so I’m not too worried about diagnosing this

arsenm marked an inline comment as done.Jan 20 2023, 6:26 AM
arsenm added inline comments.
llvm/lib/Target/AMDGPU/AMDGPURegBankSelect.cpp
15

Those should probably use the generic name. Different debug types for the same pass are counterproductive

llvm/lib/Target/AMDGPU/AMDGPURegBankSelect.h
11

I'll just drop it because these style comments are useless

arsenm updated this revision to Diff 490836.Jan 20 2023, 7:16 AM

More comments, fix -stop-after tests

arsenm edited the summary of this revision. (Show Details)Jan 20 2023, 7:16 AM
This revision is now accepted and ready to land.Jan 24 2023, 5:23 AM
rovka accepted this revision as: rovka.Jan 24 2023, 6:12 AM

Thanks for the updates, LGTM with nits.

llvm/lib/Target/AMDGPU/AMDGPURegBankSelect.cpp
11

Nit: I like the first sentence, but the second one is a bit difficult to parse. Maybe you can rephrase it a bit?

15

Be that as it may, I'd prefer using the same string for -stop-before/after and -debug-only. As it is now we'd have to use "amdgpu-regbankselect" to run the pass and just "regbankselect" to get the debug logs.

arsenm closed this revision.Jan 30 2023, 12:18 PM
arsenm marked an inline comment as done.
llvm/lib/Target/AMDGPU/AMDGPURegBankSelect.cpp
15

If I change this to amdgpu-regbankselect, -debug-only=amdgpu-regbankselect will print a fraction of the debugging information. The real debug information is in the base classes and you can't stack -debug-onlys