[PATCH] D126771: [fastalloc] Support allocate specific register class in fastalloc.

[LuoYuanke via Phabricator via llvm-commits]

LuoYuanke added inline comments.


================
Comment at: llvm/lib/CodeGen/RegAllocFast.cpp:1146
     MachineOperand &MO = MI.getOperand(I);
     if (MO.isReg()) {
       Register Reg = MO.getReg();
----------------
xiangzhangllvm wrote:
> I just take a look on this pass.
> Seem this patch mainly to insert the condition where will allocated the virtual regs.
> 
> This check seems a little duplicated with MO.isReg().
> Because we need to first make sure the MO is reg before we try to allocated for it.
> (The only exception I find is handleDebugValue, it directly get reg for MI.getUsedDebugRegs() not MO.  In fact, it also can be unified with MO)
> 
> I think we just need to replace all the "MO.isReg()" with a function which do the check of ShouldAllocateRegister
> 
> Some like:
> 
> ```
> ShouldAllocateForMO(MO){
>   return MO.isReg() && ShouldAllocateRegister(MO.getReg());   
> }
> ```
> 
> and keep your change in handleDebugValue (or unify the code with MO)
Yes, this patch is to check the callback when allocate virtual registers, but I'm not sure it is a good idea to just add check the callback following MO.isReg(). First it may be physical register for a mahcine operand. It would crash if pass physical register to ShouldAllocateRegister(). Secondly unlikely RegAllocBase, the current infrastructure of RegAllocFast has much details in allocateInstruction().


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126771