[PATCH] D130919: [MRI] isConstantPhysReg should also check if the register is clobbered by a RegMask
Guozhi Wei via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Aug 1 13:32:22 PDT 2022
Carrot added inline comments.
================
Comment at: llvm/lib/CodeGen/RegisterCoalescer.cpp:4116
MRI = &fn.getRegInfo();
+ MRI->collectUsedPhysRegMask();
const TargetSubtargetInfo &STI = fn.getSubtarget();
----------------
arsenm wrote:
> Adding a specific collection step here feels wrong. Other contexts the mask is added as soon as it's created. Not sure why we have the virtregrewriter adding these now
I agree it's ugly. Do you have any good idea where I can collect the RegMasks?
================
Comment at: llvm/lib/Target/AMDGPU/SIPreAllocateWWMRegs.cpp:104
for (MCRegister PhysReg : RegClassInfo.getOrder(MRI->getRegClass(Reg))) {
- if (!MRI->isPhysRegUsed(PhysReg) &&
+ if (!MRI->isPhysRegUsed(PhysReg, true) &&
Matrix->checkInterference(LI, PhysReg) == LiveRegMatrix::IK_Free) {
----------------
arsenm wrote:
> Don't see why this would skip regmask tests
To be honest, the MRI change causes two gpu tests failed, because previously there was no RegMask information collected, the checking of RegMask in isPhysRegUsed was actually nop. Now with RegMask collected, isPhysRegUsed behaves differently. I am completely ignorant with AMD GPU, so I just kept its original behavior by skipping regmask test.
Do you think it's better to enable the regmask test and send out the modified tests for review?
================
Comment at: llvm/lib/Target/Mips/MipsRegisterInfo.cpp:322-323
+
+bool MipsRegisterInfo::isConstantPhysReg(MCRegister PhysReg) const {
+ return PhysReg == Mips::ZERO_64 || PhysReg == Mips::ZERO;
+}
----------------
arsenm wrote:
> This should be a separate mips change
I sent out D130932 for it.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130919/new/
https://reviews.llvm.org/D130919
More information about the llvm-commits
mailing list