[PATCH] D130919: [MRI] isConstantPhysReg should also check if the register is clobbered by a RegMask

[Guozhi Wei via Phabricator via llvm-commits]

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