[MachineVerifier] Verify the RegUsageInfo collected for the current function.

[jonpa]

Summary

https://bugs.llvm.org/show_bug.cgi?id=36587 discovered a set of used (preserved) pregs in function that was corrupt which led to wrong code.

This patch adds a check in the MachineVerifier that makes sure that any registers defined by any instruction in the current function which are call-preserved have all their aliases marked as used in function in the set produced by RegUsageInfoCollector.

As it is now no verification will be done if CallPreservedMask is nullptr (this happens with a lot of AMDGPU tests).

This test fails now: LLVM :: CodeGen/AMDGPU/function-returns.ll

The test in 36587 fails like:

`# End machine code for function fn2.

• • Bad machine code: Defined register (alias) not in RegUsageInfo: R0H ***
• function: fn2
• basic block: %bb.1 (0x50f3540)
• instruction: renamable $r0d = LGHI 0

LLVM ERROR: Found 1 machine code errors.`

[jonpa]

Note: the AMDGPU test failure is not helped by https://reviews.llvm.org/D45157.

[jonpa]

I discovered that a subreg may be call-preserved even though the full register is not. Patch updated to handle this.

[qcolombet]

lib/CodeGen/MachineVerifier.cpp

360

I would have named it verifyPreservedMask or something.
I would expect a verifyUsedPhysReg to work on UsedPhysRegMask not the PreservedMask one.

[jonpa]

lib/CodeGen/MachineVerifier.cpp

360

To me verifyPreservedMask mostly associates to getCallPreservedMask(), on the other hand :-) How about verifyRegUsageInfoCollector() ?

BTW, is my naming for PreservedRegs good? It seems to be called just RegMask elsewhere. 'PreservedRegs' reflects the fact that if a bit is set, then that reg is preserved. But this is always true for a RegMask, then perhaps we should stick to that name? Or 'FunctionRegMask'?

[qcolombet]

lib/CodeGen/MachineVerifier.cpp

360

I'd say it doesn't hurt to be explicit :). (I.e., I think having preserved in the name is good.)

I like the verifyRegUsageInfoCollector name!

[jonpa]

Wait - don't we need to check if current function returns true with isSafeForNoCSROpt()?

[qcolombet]

Good catch!

Actually, following the other related patch, I think we should use determineCalleeSaved. Moreover, I don't remember when this function is supposed to give the right answer, but we probably need to gate the check behind NoVRegs or something like that.

[jonpa]

In D45156#1087897, @qcolombet wrote:

Good catch!

Actually, following the other related patch, I think we should use determineCalleeSaved. Moreover, I don't remember when this function is supposed to give the right answer, but we probably need to gate the check behind NoVRegs or something like that.

I think it's ok since PRUI is not set until the pass has been run.

I tried to simply reuse the new function from the related patch now, but ran into a problem: A reg with subregs had one subreg completely unclobbered, and the other one saved/restored. We could check subregs again with a new loop to see if they are all saved or unclobbered, or we could just add this extra check in computeCalleeSavedRegs like

// Add PReg to SavedRegs if all subregs are saved or unmodified.                                                                                                                                                             
bool AllSubRegsSaved = true;
for (MCSubRegIterator SR(PReg, TRI, false); SR.isValid(); ++SR)
  if (!SavedRegs.test(*SR) && MRI->isPhysRegModified(*SR)) {
    AllSubRegsSaved = false;
    break;
  }

I am however having second thoughts about verifying this in the MachineVerifier, given that the Collector pass is run after all else (at least after post-RA pseudo expansion and scheduling), so it seems that we are just verifying our own algorithm without having any real benefit of verifying other passes.

Do you agree we could drop this patch?

[qcolombet]

Do you agree we could drop this patch?

Agreed.

lib/CodeGen/MachineVerifier.cpp

 #include "llvm/CodeGen/MachineOperand.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/PseudoSourceValue.h"
+#include "llvm/CodeGen/RegisterUsageInfo.h"
 #include "llvm/CodeGen/SlotIndexes.h"
 #include "llvm/CodeGen/StackMaps.h"
 #include "llvm/CodeGen/TargetInstrInfo.h"

     const TargetInstrInfo *TII;
     const TargetRegisterInfo *TRI;
     const MachineRegisterInfo *MRI;
+    PhysicalRegisterUsageInfo *PRUI;
 
     unsigned foundErrors;
 

 
     void verifySlotIndexes() const;
     void verifyProperties(const MachineFunction &MF);
+    void verifyUsedPhysRegs(const MachineFunction &MF);
   };
 
   struct MachineVerifierPass : public MachineFunctionPass {

     report("Function has NoVRegs property but there are VReg operands", &MF);
 }
 
+static bool regIsInSet(const uint32_t *RegMask, unsigned PhysReg) {
+  return !MachineOperand::clobbersPhysReg(RegMask, PhysReg);
+}
+
+void MachineVerifier::verifyUsedPhysRegs(const MachineFunction &MF) {
+  // PhysicalRegisterUsageInfo becomes available with interprocedural
+  // register allocation.
+  if (!PRUI)
+    return;
+  const Function &F = MF.getFunction();
+  const auto *PreservedRegs = PRUI->getRegUsageInfo(&F);
+  if (!PreservedRegs)
+    return;
+  const uint32_t *CallPreservedMask =
+    TRI->getCallPreservedMask(MF, F.getCallingConv());
+  if (!CallPreservedMask)
+    return;
+
+  for (unsigned PReg = 1, PRegE = TRI->getNumRegs(); PReg < PRegE; ++PReg) {
+    // Any register defined by an instruction that is not preserved across a
+    // call according to the calling convention should result in all aliases
+    // excluded from the function mask of preserved registers.
+    if (MRI->def_empty(PReg) || regIsInSet(CallPreservedMask, PReg))
+      continue;
+    for (MCRegAliasIterator AI(PReg, TRI, true); AI.isValid(); ++AI)
+      if (regIsInSet(&(*PreservedRegs)[0], *AI)) {
+        std::string Msg = "Defined register (alias) not in RegUsageInfo: "
+          + std::string(TRI->getName(*AI));
+        const MachineOperand &DefMO = *MRI->def_begin(PReg);
+        report(Msg.c_str(), DefMO.getParent());
+      }
+  }
+}
+
 unsigned MachineVerifier::verify(MachineFunction &MF) {
   foundErrors = 0;
 

   LiveInts = nullptr;
   LiveStks = nullptr;
   Indexes = nullptr;
+  PRUI = nullptr;
   if (PASS) {
     LiveInts = PASS->getAnalysisIfAvailable<LiveIntervals>();
     // We don't want to verify LiveVariables if LiveIntervals is available.

       LiveVars = PASS->getAnalysisIfAvailable<LiveVariables>();
     LiveStks = PASS->getAnalysisIfAvailable<LiveStacks>();
     Indexes = PASS->getAnalysisIfAvailable<SlotIndexes>();
+    PRUI = PASS->getAnalysisIfAvailable<PhysicalRegisterUsageInfo>();
   }
 
   verifySlotIndexes();
 
   verifyProperties(MF);
 
+  verifyUsedPhysRegs(MF);
+
   visitMachineFunctionBefore();
   for (MachineFunction::const_iterator MFI = MF.begin(), MFE = MF.end();
        MFI!=MFE; ++MFI) {