[MachinePipeliner] Use `RegisterClassInfo::getRegPressureSetLimit` #119827

wangpc-pp · 2024-12-13T07:08:09Z

RegisterClassInfo::getRegPressureSetLimit is a wrapper of
TargetRegisterInfo::getRegPressureSetLimit with some logics to
adjust the limit by removing reserved registers.

It seems that we shouldn't use TargetRegisterInfo::getRegPressureSetLimit
directly, just like the comment "This limit must be adjusted
dynamically for reserved registers" said.

Thus we should use RegisterClassInfo::getRegPressureSetLimit and
remove replicated code.

Separate from #118787

arsenm · 2024-12-13T07:24:35Z

llvm/lib/CodeGen/MachinePipeliner.cpp

-    // We assume fixed registers, such as stack pointer, are already in use.
-    // Therefore subtracting the weight of the fixed registers from the limit of
-    // each pressure set in advance.
-    SmallDenseSet<Register, 8> FixedRegs;


Not sure what a fixed register is, but it seems to not be the same as reserved. Seems like another hook with bad defaults

Yeah, it is a bad API. Fixed registers are not reserved register... Fixed register are registers defined in FixedRegisters RegisterCategory by default...
The logic here seems to want to remove reserved registers @kasuga-fj.

As I commented in #118787 (comment), I dared to replace it from RegisterClassInfo::getRegPressureSetLimit before (in #87312). This is because there is a duplication between the reserved registers handled inside RegisterClassInfo::getRegPressureSetLimit and the FixedRegs calculated here, so the Limit >= Weight assertion fails. If I remember correctly, in my case (AArch64), the $ffr triggers it. What I'd like to do here is to ignore registers that have "specific rolls" (e.g., stack pointer) and those where TargetRegisterInfo::isFixedRegister returns true looks appropriate in this case to me. Maybe I'm misunderstanding reserved and fixed registers, and there may be a better way to achieve what I want, but I'm not sure this change will work well.

IMO isFixedRegister should be deleted. The intent seems to be isReserved || isConstantPhysREg

Thanks for explanation! At least, after removing this part of code, the tests still pass? @kasuga-fj

I ran tests and none of them failed. But that just means there are no correctness issues. This code is part of a heuristic decision, so performance degradation could happen in some cases (and I think the heuristics will be less accurate with this change).

I think the heuristics will be less accurate with this change

I don't think so. These "fixed" registers are also "reserved" registers, and "reserved" registers contain runtime fixed registers like those are reserved via -ffixed-xx options.

These "fixed" registers are also "reserved" registers,

Previously (where RegisterClassInfo::getRegPressureSetLimit was used here), #83437 caused the Limit >= Weight assertion to fail. This patch marks the $ffr register as reserved. I just tried to comment out the code that marks $ffr as reserved and run MachinePipeliner, and found that FixedRegs contains $ffr. It looks to me like there are some gaps between "reserved" and "fixed". Or maybe I misunderstood something (I don't know much about the LLVM API for registers).

`RegisterClassInfo::getRegPressureSetLimit` is a wrapper of `TargetRegisterInfo::getRegPressureSetLimit` with some logics to adjust the limit by removing reserved registers. It seems that we shouldn't use `TargetRegisterInfo::getRegPressureSetLimit` directly, just like the comment "This limit must be adjusted dynamically for reserved registers" said. Thus we should use `RegisterClassInfo::getRegPressureSetLimit` and remove replicated code. Separate from llvm#118787

kasuga-fj · 2024-12-20T07:35:17Z

@wangpc-pp Could you please check my comment #119827 (comment)? I don't have a strong opinion to reject this change, but I still suspect that there are some differences between "fixed" and "reserved" registers. To make the code consistent, I think we need to change the following function.

llvm-project/llvm/lib/CodeGen/MachinePipeliner.cpp

Lines 1287 to 1289 in 93743ee

    
           bool isFixedRegister(Register Reg) const { 
        
             return Reg.isPhysical() && TRI->isFixedRegister(MF, Reg.asMCReg()); 
        
           }

wangpc-pp · 2024-12-20T08:15:57Z

isFixedRegister

Sorry, I was thinking that I may have solved your problem.
I think fixed registers are subset of reserved registers.
For AArch64, fixed registers are:

bool AArch64GenRegisterInfo::
isFixedRegister(const MachineFunction &MF, MCRegister PhysReg) const {
  return
      AArch64::CCRRegClass.contains(PhysReg) ||
      AArch64::FIXED_REGSRegClass.contains(PhysReg) ||
      false;
}

// Condition code regclass.
def CCR : RegisterClass<"AArch64", [i32], 32, (add NZCV)> {
  let CopyCost = -1;  // Don't allow copying of status registers.

  // CCR is not allocatable.
  let isAllocatable = 0;
}

def FIXED_REGS : RegisterClass<"AArch64", [i64], 64, (add FP, SP, VG, FFR)>;
def FixedRegisters : RegisterCategory<[CCR, FIXED_REGS]>;

Reserved registers are:

llvm-project/llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp

Lines 426 to 518 in 93743ee

    
           BitVector 
        
           AArch64RegisterInfo::getStrictlyReservedRegs(const MachineFunction &MF) const { 
        
             const AArch64FrameLowering *TFI = getFrameLowering(MF); 
        
             // FIXME: avoid re-calculating this every time. 
        
             BitVector Reserved(getNumRegs()); 
        
             markSuperRegs(Reserved, AArch64::WSP); 
        
             markSuperRegs(Reserved, AArch64::WZR); 
        
             if (TFI->hasFP(MF) || TT.isOSDarwin()) 
        
               markSuperRegs(Reserved, AArch64::W29); 
        
             if (MF.getSubtarget<AArch64Subtarget>().isWindowsArm64EC()) { 
        
               // x13, x14, x23, x24, x28, and v16-v31 are clobbered by asynchronous 
        
               // signals, so we can't ever use them. 
        
               markSuperRegs(Reserved, AArch64::W13); 
        
               markSuperRegs(Reserved, AArch64::W14); 
        
               markSuperRegs(Reserved, AArch64::W23); 
        
               markSuperRegs(Reserved, AArch64::W24); 
        
               markSuperRegs(Reserved, AArch64::W28); 
        
               for (unsigned i = AArch64::B16; i <= AArch64::B31; ++i) 
        
                 markSuperRegs(Reserved, i); 
        
             } 
        
             for (size_t i = 0; i < AArch64::GPR32commonRegClass.getNumRegs(); ++i) { 
        
               if (MF.getSubtarget<AArch64Subtarget>().isXRegisterReserved(i)) 
        
                 markSuperRegs(Reserved, AArch64::GPR32commonRegClass.getRegister(i)); 
        
             } 
        
             if (hasBasePointer(MF)) 
        
               markSuperRegs(Reserved, AArch64::W19); 
        
             // SLH uses register W16/X16 as the taint register. 
        
             if (MF.getFunction().hasFnAttribute(Attribute::SpeculativeLoadHardening)) 
        
               markSuperRegs(Reserved, AArch64::W16); 
        
             // FFR is modelled as global state that cannot be allocated. 
        
             if (MF.getSubtarget<AArch64Subtarget>().hasSVE()) 
        
               Reserved.set(AArch64::FFR); 
        
             // SME tiles are not allocatable. 
        
             if (MF.getSubtarget<AArch64Subtarget>().hasSME()) { 
        
               for (MCPhysReg SubReg : subregs_inclusive(AArch64::ZA)) 
        
                 Reserved.set(SubReg); 
        
             } 
        
             // VG cannot be allocated 
        
             Reserved.set(AArch64::VG); 
        
             if (MF.getSubtarget<AArch64Subtarget>().hasSME2()) { 
        
               for (MCSubRegIterator SubReg(AArch64::ZT0, this, /*self=*/true); 
        
                    SubReg.isValid(); ++SubReg) 
        
                 Reserved.set(*SubReg); 
        
             } 
        
             markSuperRegs(Reserved, AArch64::FPCR); 
        
             markSuperRegs(Reserved, AArch64::FPMR); 
        
             markSuperRegs(Reserved, AArch64::FPSR); 
        
             if (MF.getFunction().getCallingConv() == CallingConv::GRAAL) { 
        
               markSuperRegs(Reserved, AArch64::X27); 
        
               markSuperRegs(Reserved, AArch64::X28); 
        
               markSuperRegs(Reserved, AArch64::W27); 
        
               markSuperRegs(Reserved, AArch64::W28); 
        
             } 
        
             assert(checkAllSuperRegsMarked(Reserved)); 
        
             // Add _HI registers after checkAllSuperRegsMarked as this check otherwise 
        
             // becomes considerably more expensive. 
        
             Reserved.set(AArch64::WSP_HI); 
        
             Reserved.set(AArch64::WZR_HI); 
        
             static_assert(AArch64::W30_HI - AArch64::W0_HI == 30, 
        
                           "Unexpected order of registers"); 
        
             Reserved.set(AArch64::W0_HI, AArch64::W30_HI); 
        
             static_assert(AArch64::B31_HI - AArch64::B0_HI == 31, 
        
                           "Unexpected order of registers"); 
        
             Reserved.set(AArch64::B0_HI, AArch64::B31_HI); 
        
             static_assert(AArch64::H31_HI - AArch64::H0_HI == 31, 
        
                           "Unexpected order of registers"); 
        
             Reserved.set(AArch64::H0_HI, AArch64::H31_HI); 
        
             static_assert(AArch64::S31_HI - AArch64::S0_HI == 31, 
        
                           "Unexpected order of registers"); 
        
             Reserved.set(AArch64::S0_HI, AArch64::S31_HI); 
        
             static_assert(AArch64::D31_HI - AArch64::D0_HI == 31, 
        
                           "Unexpected order of registers"); 
        
             Reserved.set(AArch64::D0_HI, AArch64::D31_HI); 
        
             static_assert(AArch64::Q31_HI - AArch64::Q0_HI == 31, 
        
                           "Unexpected order of registers"); 
        
             Reserved.set(AArch64::Q0_HI, AArch64::Q31_HI); 
        
             return Reserved; 
        
           }

So, all fixed registers are also reserved registers.

I created #120694 to skip reserved registers, please have a look. :-)

kasuga-fj · 2024-12-20T08:40:24Z

I see. Thanks, I thought a function like isReserved would call isFixed internally, but it doesn't.

wangpc-pp requested review from kasuga-fj, topperc and arsenm December 13, 2024 07:08

arsenm reviewed Dec 13, 2024

View reviewed changes

arsenm approved these changes Dec 16, 2024

View reviewed changes

wangpc-pp force-pushed the main-machinepipeliner-reg-pressure-set branch from b0dd0ca to e0b49e0 Compare December 18, 2024 06:52

wangpc-pp merged commit 1235a93 into llvm:main Dec 18, 2024
5 of 7 checks passed

wangpc-pp deleted the main-machinepipeliner-reg-pressure-set branch December 18, 2024 07:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MachinePipeliner] Use `RegisterClassInfo::getRegPressureSetLimit` #119827

[MachinePipeliner] Use `RegisterClassInfo::getRegPressureSetLimit` #119827

wangpc-pp commented Dec 13, 2024

arsenm Dec 13, 2024

wangpc-pp Dec 13, 2024

kasuga-fj Dec 13, 2024

arsenm Dec 13, 2024

wangpc-pp Dec 13, 2024

kasuga-fj Dec 13, 2024

wangpc-pp Dec 13, 2024 •

edited

Loading

kasuga-fj Dec 13, 2024

kasuga-fj commented Dec 20, 2024

wangpc-pp commented Dec 20, 2024

kasuga-fj commented Dec 20, 2024

[MachinePipeliner] Use RegisterClassInfo::getRegPressureSetLimit #119827

[MachinePipeliner] Use RegisterClassInfo::getRegPressureSetLimit #119827

Conversation

wangpc-pp commented Dec 13, 2024

arsenm Dec 13, 2024

Choose a reason for hiding this comment

wangpc-pp Dec 13, 2024

Choose a reason for hiding this comment

kasuga-fj Dec 13, 2024

Choose a reason for hiding this comment

arsenm Dec 13, 2024

Choose a reason for hiding this comment

wangpc-pp Dec 13, 2024

Choose a reason for hiding this comment

kasuga-fj Dec 13, 2024

Choose a reason for hiding this comment

wangpc-pp Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

kasuga-fj Dec 13, 2024

Choose a reason for hiding this comment

kasuga-fj commented Dec 20, 2024

wangpc-pp commented Dec 20, 2024

kasuga-fj commented Dec 20, 2024

[MachinePipeliner] Use `RegisterClassInfo::getRegPressureSetLimit` #119827

[MachinePipeliner] Use `RegisterClassInfo::getRegPressureSetLimit` #119827

wangpc-pp Dec 13, 2024 •

edited

Loading