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

[Carrot]

Summary

Current implementation of MachineRegisterInfo::isConstantPhysReg checks if the specified physical is modified in a function, but it only checks if it is used in a Def register operand, it may still be modified in a RegMask operand, like a function call. So it can give wrong result in this situation.

Other changes are used to prevent test regressions.

[arsenm]

llvm/lib/CodeGen/MachineRegisterInfo.cpp

583

This can just be a range loop. Also braces

llvm/lib/CodeGen/RegisterCoalescer.cpp

4116

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

llvm/lib/Target/AMDGPU/SIPreAllocateWWMRegs.cpp

104

Don't see why this would skip regmask tests

llvm/lib/Target/Mips/MipsRegisterInfo.cpp

322–323

This should be a separate mips change

[Carrot]

llvm/lib/CodeGen/RegisterCoalescer.cpp

4116

I agree it's ugly. Do you have any good idea where I can collect the RegMasks?

llvm/lib/Target/AMDGPU/SIPreAllocateWWMRegs.cpp

104

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?

llvm/lib/Target/Mips/MipsRegisterInfo.cpp

322–323

I sent out D130932 for it.

[arichardson]

It's been a while since I looked at constant registers but I made a few changes in CHERI LLVM to avoid unnecessary copies. I just had a quick look at some of the commits and https://github.com/CTSRD-CHERI/llvm-project/commit/8588d8b81458ed6d87b674893e7752e6a6915574 looks somewhat related.

[arichardson]

Uploaded the patch I committed to CHERI LLVM as https://reviews.llvm.org/D131958.

[arsenm]

Can this be closed with D131958?

[arichardson]

In D130919#3802733, @arsenm wrote:

Can this be closed with D131958?

I think so (about to commit that revision once I check tests still pass), but it's hard to say if it covers all cases this revision does since there are no tests.

[Carrot]

In D130919#3802764, @arichardson wrote:

In D130919#3802733, @arsenm wrote:

Can this be closed with D131958?

I think so (about to commit that revision once I check tests still pass), but it's hard to say if it covers all cases this revision does since there are no tests.

No, a phyiscal register is not a target constant register, but if it is not modified inside a specific function, it can still be treated as a constant register by MachineRegisterInfo::isConstantPhysReg inside the specific function. The modification should include both defined register operand and regmask.

I will update this patch. But it still depends on D132987 and D132561 to prevent regressions. These are real problems exposed by this patch.

[Carrot]

Update the patch to collect RegMask information whenever a new RegMask operand is created or its content is changed.

[arsenm]

llvm/lib/CodeGen/RegUsageInfoPropagate.cpp

147–149 ↗

(On Diff #462551)

Won't this be reflected in the regmask updates (BTW I think this pass needs to be changed to directly modify the regmasks in the instruction instead of the analysis)?

[Carrot]

llvm/lib/CodeGen/RegUsageInfoPropagate.cpp

147–149 ↗

(On Diff #462551)

Query register information through MRI is precise through the linked Register operands. But RegMask is different, all RegMask information is cumulated in MRI.UsedPhysRegMask. So usually we can only add new used physical registers into UsedPhysRegMask. This pass is used to reduce used physical registers, we need to reset UsedPhysRegMask and collect RegMask again.

[Carrot]

ping

[Carrot]

Rebase.

[Carrot]

Any other comments?

[foad]

Why are there so many changes in AMDGPU codegen, even in tests that do not contain function calls?

[Carrot]

In D130919#3891206, @foad wrote:

Why are there so many changes in AMDGPU codegen, even in tests that do not contain function calls?

In a previous comment I mentioned I'm completely ignorant with AMDGPU, the instructions looks very different when compared with traditional CPU. So it's better to have an AMDGPU expert to have a look at these changes.

I tried to debug the simplest one wwm-reserved.ll. The difference comes from pass si-pre-allocate-wwm-regs, the related code is

...
160B      dead $sgpr30_sgpr31 = SI_CALL %8:sreg_64, @called, <regmask $noreg $exec $exec_hi $exec_lo $flat_scr $flat_scr_hi $flat_scr_hi_ci $flat_scr_hi_vi $flat_scr_lo $flat_scr_lo_ci $flat_scr_lo_vi $flat_scr_ci $flat_scr_vi $fp_reg $lds_direct $mode $pc_reg $private_rsrc_reg $scc $sgpr_null $sgpr_null_hi $sp_reg $src_execz $src_pops_exiting_wave_id $src_private_base $src_private_limit $src_scc $src_shared_base $src_shared_limit $src_vccz $tba $tba_hi $tba_lo and 6831 more...>, implicit $sgpr0_sgpr1_sgpr2_sgpr3, implicit $vgpr0, implicit-def $vgpr0
168B      %10:vgpr_32 = COPY $vgpr0
...

When this pass processing the COPY instruction, function SIPreAllocateWWMRegs::processDef tries to find a physical register for virtual register %10. It tries every physical register in class vgpr_32 to see if it fits.

...
  for (MCRegister PhysReg : RegClassInfo.getOrder(MRI->getRegClass(Reg))) {
    if (!MRI->isPhysRegUsed(PhysReg) &&                                                                             // Different result
        Matrix->checkInterference(LI, PhysReg) == LiveRegMatrix::IK_Free) {
      Matrix->assign(LI, PhysReg);
      assert(PhysReg != 0); 
      RegsToRewrite.push_back(Reg);
      return true;
    }   
  }
...

When PhysReg is $vgpr1, MRI->isPhysRegUsed(PhysReg) computes different result because of this patch. Function MRI->isPhysRegUsed checks regmask before register operands. But regmask information was not collected until very late, so at this time MRI doesn't have any regmask information, and there is no explicit $vgpr1 in the function, so MRI->isPhysRegUsed returns true for $vgpr1, and it is assigned to %10.

With this patch, MRI regmask information is updated whenever there is a regmask change, and recollected in reg-usage-propagation pass. So when $vgpr1 is passed to MRI->isPhysRegUsed, false is returned because $vgpr1 is used in @called and propagated to MRI in current function, it means $vgpr1 is not appropriate for %10, and next physical register $vgpr2 is tried.

Although the original behavior is correct for this specific function, but it is very dangerous. If we have another function call to @called inside the live range of %10, wrong code can be generated.

[foad]

llvm/lib/CodeGen/MachineCSE.cpp

280 ↗

(On Diff #471353)

Can you please split this part out into a separate patch for review? It causes significant codegen changes for AMDGPU and we need to be sure that the changes are OK.

llvm/test/CodeGen/AMDGPU/GlobalISel/udiv.i64.ll

1 ↗

(On Diff #471353)

I don't understant why these diffs are included in the patch. Can you double check? I don't think your patch affects codegen for any of:

LLVM :: CodeGen/AMDGPU/GlobalISel/udiv.i64.ll
LLVM :: CodeGen/AMDGPU/GlobalISel/urem.i64.ll
LLVM :: CodeGen/AMDGPU/sgpr-control-flow.ll

[Carrot]

@foad, thanks a lot for reviewing AMDGPU changes!

llvm/lib/CodeGen/MachineCSE.cpp

280 ↗

(On Diff #471353)

It is split to D137222. I also did an analysis for it.

llvm/test/CodeGen/AMDGPU/GlobalISel/udiv.i64.ll

1 ↗

(On Diff #471353)

Will investigate it.

[Carrot]

llvm/test/CodeGen/AMDGPU/GlobalISel/udiv.i64.ll

1 ↗

(On Diff #471353)

They are also caused by the change in MachineCSE. They don't show up in D137222 because there are other changes in MachineCSE::isPRECandidate between the two versions of my two clients. They will disappear after rebaseing.

[Carrot]

Rebase this patch after committing the MachineCSE part.

[Carrot]

ping

[Carrot]

Any other comments?

[Carrot]

Add a test case to show that llvm can generate wrong instructions without this patch.

[Carrot]

Rebase the code.

The new failed test pr59305.ll is another case that wrong code was generated without this patch.

Several DIV instructions crossing function call to fesetround got CSEd, fesetround is used to change rounding mode, so these DIV instructions may generate different results, they can't be CSEd.

llvm/include/llvm/CodeGen/MachineRegisterInfo.h

public:
   bool isPhysRegUsed(MCRegister PhysReg, bool SkipRegMaskTest = false) const;
 
   /// addPhysRegsUsedFromRegMask - Mark any registers not in RegMask as used.
   /// This corresponds to the bit mask attached to register mask operands.
   void addPhysRegsUsedFromRegMask(const uint32_t *RegMask) {
     UsedPhysRegMask.setBitsNotInMask(RegMask);
   }
 
+  void collectUsedPhysRegMask();
+
   const BitVector &getUsedPhysRegsMask() const { return UsedPhysRegMask; }
 
   //===--------------------------------------------------------------------===//
   // Reserved Register Info
   //===--------------------------------------------------------------------===//
   //
   // The set of reserved registers must be invariant during register
   // allocation.  For example, the target cannot suddenly decide it needs a

llvm/lib/CodeGen/MachineRegisterInfo.cpp

bool MachineRegisterInfo::isConstantPhysReg(MCRegister PhysReg) const {
   const TargetRegisterInfo *TRI = getTargetRegisterInfo();
   if (TRI->isConstantPhysReg(PhysReg))
     return true;
 
   // Check if any overlapping register is modified, or allocatable so it may be
   // used later.
   for (MCRegAliasIterator AI(PhysReg, TRI, true);
        AI.isValid(); ++AI)
-    if (!def_empty(*AI) || isAllocatable(*AI))
+    if (!def_empty(*AI) || isAllocatable(*AI) || UsedPhysRegMask.test(PhysReg))
       return false;
+
   return true;
 }
 
 /// markUsesInDebugValueAsUndef - Mark every DBG_VALUE referencing the
 /// specified register as undefined which causes the DBG_VALUE to be
 /// deleted during LiveDebugVariables analysis.
 void MachineRegisterInfo::markUsesInDebugValueAsUndef(Register Reg) const {
   // Mark any DBG_VALUE* that uses Reg as undef (but don't delete it.)
for (const MachineOperand &MO : make_range(def_begin(*AI), def_end())) {
       if (!SkipNoReturnDef && isNoReturnDef(MO))
         continue;
       return true;
     }
   }
   return false;
 }
 
+void MachineRegisterInfo::collectUsedPhysRegMask() {
+  for (auto MBBI = MF->begin(), MBBE = MF->end(); MBBI != MBBE; ++MBBI)
+    for (MachineInstr &MI : MBBI->instrs())
+      for (MachineOperand &MO : MI.operands())
+        if (MO.isRegMask())
+          addPhysRegsUsedFromRegMask(MO.getRegMask());
+}
+
 bool MachineRegisterInfo::isPhysRegUsed(MCRegister PhysReg,
                                         bool SkipRegMaskTest) const {
   if (!SkipRegMaskTest && UsedPhysRegMask.test(PhysReg))
     return true;
   const TargetRegisterInfo *TRI = getTargetRegisterInfo();
   for (MCRegAliasIterator AliasReg(PhysReg, TRI, true); AliasReg.isValid();
        ++AliasReg) {
     if (!reg_nodbg_empty(*AliasReg))

llvm/lib/CodeGen/RegisterCoalescer.cpp

bool RegisterCoalescer::runOnMachineFunction(MachineFunction &fn) {
   if (fn.exposesReturnsTwice()) {
     LLVM_DEBUG(
         dbgs() << "* Skipped as it exposes funcions that returns twice.\n");
     return false;
   }
 
   MF = &fn;
   MRI = &fn.getRegInfo();
+  MRI->collectUsedPhysRegMask();
   const TargetSubtargetInfo &STI = fn.getSubtarget();
   TRI = STI.getRegisterInfo();
   TII = STI.getInstrInfo();
   LIS = &getAnalysis<LiveIntervals>();
   AA = &getAnalysis<AAResultsWrapperPass>().getAAResults();
   Loops = &getAnalysis<MachineLoopInfo>();
   if (EnableGlobalCopies == cl::BOU_UNSET)
     JoinGlobalCopies = STI.enableJoinGlobalCopies();

llvm/lib/CodeGen/TargetInstrInfo.cpp

if (Reg == 0)
       continue;
 
     // Check for a well-behaved physical register.
     if (Register::isPhysicalRegister(Reg)) {
       if (MO.isUse()) {
         // If the physreg has no defs anywhere, it's just an ambient register
         // and we can freely move its uses. Alternatively, if it's allocatable,
         // it could get allocated to something with a def during allocation.
-        if (!MRI.isConstantPhysReg(Reg))
+        if (!MRI.isConstantPhysReg(Reg) && !isIgnorableUse(MO))
           return false;
       } else {
         // A physreg def. We can't remat it.
         return false;
       }
       continue;
     }
 

llvm/lib/Target/AMDGPU/SIPreAllocateWWMRegs.cpp

if (!TRI->isVGPR(*MRI, Reg))
     return false;
 
   if (VRM->hasPhys(Reg))
     return false;
 
   LiveInterval &LI = LIS->getInterval(Reg);
 
   for (MCRegister PhysReg : RegClassInfo.getOrder(MRI->getRegClass(Reg))) {
-    if (!MRI->isPhysRegUsed(PhysReg) &&
+    if (!MRI->isPhysRegUsed(PhysReg, true) &&
         Matrix->checkInterference(LI, PhysReg) == LiveRegMatrix::IK_Free) {
       Matrix->assign(LI, PhysReg);
       assert(PhysReg != 0);
       RegsToRewrite.push_back(Reg);
       return true;
     }
   }
 

llvm/lib/Target/Mips/MipsRegisterInfo.h

void eliminateFrameIndex(MachineBasicBlock::iterator II,
                            RegScavenger *RS = nullptr) const override;
 
   // Stack realignment queries.
   bool canRealignStack(const MachineFunction &MF) const override;
 
   /// Debug information queries.
   Register getFrameRegister(const MachineFunction &MF) const override;
 
+  bool isConstantPhysReg(MCRegister PhysReg) const override;
+
   /// Return GPR register class.
   virtual const TargetRegisterClass *intRegClass(unsigned Size) const = 0;
 
 private:
   virtual void eliminateFI(MachineBasicBlock::iterator II, unsigned OpNo,
                            int FrameIndex, uint64_t StackSize,
                            int64_t SPOffset) const = 0;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_LIB_TARGET_MIPS_MIPSREGISTERINFO_H

llvm/lib/Target/Mips/MipsRegisterInfo.cpp

bool MipsRegisterInfo::canRealignStack(const MachineFunction &MF) const {
   // don't have variable sized objects.
   if (Subtarget.getFrameLowering()->hasReservedCallFrame(MF))
     return true;
 
   // We have to reserve the base pointer register in the presence of variable
   // sized objects.
   return MF.getRegInfo().canReserveReg(BP);
 }
+
+bool MipsRegisterInfo::isConstantPhysReg(MCRegister PhysReg) const {
+  return PhysReg == Mips::ZERO_64 || PhysReg == Mips::ZERO;
+}

llvm/lib/Target/X86/X86InstrInfo.h

#include "X86GenInstrInfo.inc"
 
   static bool hasLockPrefix(const MachineInstr &MI) {
     return MI.getDesc().TSFlags & X86II::LOCK;
   }
 
   Optional<ParamLoadedValue> describeLoadedValue(const MachineInstr &MI,
                                                  Register Reg) const override;
 
+  /// Given \p MO is a PhysReg use return if it can be ignored for the purpose
+  /// of instruction rematerialization or sinking.
+  bool isIgnorableUse(const MachineOperand &MO) const override {
+    // An RIP relative address is a constant.
+    return MO.getReg() == X86::RIP;
+  }
+
 protected:
   /// Commutes the operands in the given instruction by changing the operands
   /// order and/or changing the instruction's opcode and/or the immediate value
   /// operand.
   ///
   /// The arguments 'CommuteOpIdx1' and 'CommuteOpIdx2' specify the operands
   /// to be commuted.
   ///