[PATCH] D14477: Remove unnecessary call to getAllocatableRegClass

[Matt Arsenault via llvm-commits]

arsenm added a comment.

In http://reviews.llvm.org/D14477#285174, @qcolombet wrote:

> > I'm not sure why you would ever define an instruction that produces an unallocatable register class.
>
>
> That’s not exactly the semantic here. The register class is not unallocatable per say, only a subset is. By calling getAllocatableClass, we get the subset of the class that is allocatable.
>  This may happen if say, your instruction can encode GPR + SP but you can only allocate GPR.
>
> In other words, what you’re saying in the next sentence is perfectly legal:
>
> >   it seems like it should be a verifier error to define such an instruction.
>
>
> I have to admit I have some troubles figuring why this is a problem in your case. May be worth spending more time to understand what is going on (i.e., probably spending more time to explain to me :)).


The parts of the problem are:

- All instructions produce either a VGPR or SGPR, they can never produce one or the other
- Most instructions are VALU instructions, and most operands can be either a VGPR or SGPR. This is modeled with the VS_32/VS_64 register classes, which I want to make unallocatable because currently they influence pressure heuristics even though there should never be a virtual register with these classes.
- From LLVM's perspective VGPRs and SGPRs are equivalent in every way except for the number of them.
- getAllocatableRegClass on the VS_32 superset will just pick one or the other. Based on the ordering scheme, register classes with more registers are ordered first, so this will always select the VGPR allocatable subset.
- When the input operand is an SGPR, we will always get an SGPR->VGPR copy. This introduces a large number of copies since the most common operands are the combined VS class.
- All of these extra copies increase the amount of work the later custom operand legalization and folding passes need to do. This is backwards from how we expect this to happen. We have a lot of logic for when to insert these copies as required, and we don't want the generic code deciding to do this for us, and just want to pass through the original result register's type.
- What happens now is since VS_32/VS_64 happen to be allocatable, that is selected and then the destination constraint is applied and it ends up with what we want.



> That being said, I believe the reason why you don’t have any test failing because of that change, is simply because when we create virtual register we restrict them on allocatable class. I honestly don’t see how we could be an example where this is needed.


Yes, we only want it restricted on the result registers and not the inputs.

> The bottom line is okay, to remove this additional check, but two things:

> 

> - I’d like to understand why this is a problem in your case. (Could you post the MI representation exposing the problem.)


When pretending isAllocatable for VS_32:

  	%vreg11<def> = V_OR_B32_e32 %vreg9<kill>, %vreg10<kill>, %EXEC<imp-use>; VGPR_32:%vreg11,%vreg10 SReg_32:%vreg9

Note %vreg9 is SReg_32. That operand is a VS_32 operand.

With http://reviews.llvm.org/D14478 applied without this patch:

  	%vreg12<def> = COPY %vreg9; VGPR_32:%vreg12 SReg_32:%vreg9
  	%vreg11<def> = V_OR_B32_e32 %vreg12<kill>, %vreg10<kill>, %EXEC<imp-use>; VGPR_32:%vreg11,%vreg12,%vreg10

The copy is inserted and all of the V_OR_B32 operands are VGPRs. The most common case the existing lit tests is materializing immediates, which are originally placed in SGPRs as a canonical form and may be changed later.

> - Please add a test case for your problem. (Could be done as a follow-up commit if you want to track the changes separately.)


This is pretty well covered by existing tests and fixes test regressions when http://reviews.llvm.org/D14478 is applied. I'm not sure how to come up with a test for the weird pressure tracking changes this is supposed to help with.

> Thanks,

> -Quentin



http://reviews.llvm.org/D14477