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

[Carl Ritson via Phabricator via llvm-commits]

critson 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;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84910