This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][GlobalISel] Rematerialze constants with different regbank
Needs ReviewPublic

Authored by mbrkusanin on May 17 2023, 8:28 AM.

Details

Reviewers
foad
arsenm
Summary

Constants which are mapped by default to SGPRs will require a copy to a
VPGR if they have a VGPR use. By rematerializing the instructions with the
appropriate bank we can avoid cross regbank copies.

Diff Detail

Event Timeline

mbrkusanin created this revision.May 17 2023, 8:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2023, 8:29 AM
mbrkusanin requested review of this revision.May 17 2023, 8:29 AM
mbrkusanin added inline comments.May 17 2023, 8:30 AM
llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp
250–261

Regbank's MIRBuilder does not have CSEInfo so this will crate new G_(F)CONSTANT for each use. I tried an idea to avoid that where InsertPoint for rematerializations would be changed to always be after old G_(F)CONSTANT, so we would only need to check instruction after the current/old one if it is a copy with the appropriate regbank (created earlier). This did make .mir test look shorter in some cases but it did not have any real improvements for final ISA, only shuffled instructions.

527

Maybe this should be a target hook? Lit tests from other targets are not affected by this change, but check for size<32 is because of lack of v_mov_b64.

  • Rebase + Ping
mbrkusanin edited the summary of this revision. (Show Details)
  • Rebase + Ping
  • Do we still want a solution that works as a new repair kind in regbankselect? Alternatively we could make a regbank combiner.
  • Do we still want a solution that works as a new repair kind in regbankselect? Alternatively we could make a regbank combiner.

I think it does still make sense for this to be a repair kind

arsenm added inline comments.Jun 30 2023, 2:56 PM
llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp
250–261

This is just a bug. I've been meaning to thread a MIRBuilder through all these APIs. MIRBuilders should be constructed once at pass entry to preserve CSEInfo

525–526

Do we have some sort of isLeaf or isConstantLike helper? This should also at minimum cover G_FRAME_INDEX and G_GLOBAL_VALUE. I think it makes sense in some cases with inputs too

527

I don't follow the v_mov_b64 part, the size shouldn't matter. We can rematerialize as 2 pieces easily (also, we do technically have the infrequently used V_MOV_B64_PSEUDO). In any case that's beyond the concern of this action