[PATCH] D84910: [AMDGPU] Make GCNRegBankReassign assign based on subreg banks

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/GCNRegBankReassign.cpp:403
         // If a register spans all banks we cannot shift it to avoid conflict.
         if (TRI->getNumCoveredRegs(LM) >= NUM_VGPR_BANKS)
           continue;
----------------
critson wrote:
> rampitec wrote:
> > I do not like this test gone. We really need to bail here.
> So if I understand correctly, the test was saying "if a sub-register covers all banks then exclude it from the stall count"?
> My problem with the test was that it was being applied only when a reg+bank was specified for reassignment.
> This meant that the stall count computation method was different when testing a bank assignment, causing stall count mismatches and infinite loops in some cases.
> 
> I have reinstated the test, but for all stall computations.
> Based on the above understanding this should be correct as we never want to spend time evaluating unavoidable stalls.
Right, thank you! LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84910/new/

https://reviews.llvm.org/D84910