[PATCH] D21115: Add a callee-saved register verifier to LLVM

Quentin Colombet via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 8 13:51:26 PDT 2016


qcolombet added inline comments.

================
Comment at: include/llvm/InitializePasses.h:342
@@ -341,2 +341,3 @@
 void initializePatchableFunctionPass(PassRegistry &);
+void initializeVerifyCalleeSavedRegsPass(PassRegistry &);
 }
----------------
Could you sort that call in alphabetical order?
I know this is not the case for the last few calls, but really, it should be the case for all of them.

================
Comment at: include/llvm/Target/TargetInstrInfo.h:764
@@ +763,3 @@
+  /// constant pool value if profitable or required.  Return true on success.
+  virtual bool copyImmToPhysReg(MachineBasicBlock &MBB,
+                                MachineBasicBlock::iterator MI, DebugLoc DL,
----------------
I would go with something more generic in the name, like insertLoadImmediate.
The fact that DestReg is a physical register or not shouldn’t be a criteria.

================
Comment at: include/llvm/Target/TargetInstrInfo.h:769
@@ +768,3 @@
+                                bool AllowConstantPool = true) const {
+    llvm_unreachable("Target didn't implement TargetInstrInfo::copyPhysReg!");
+  }
----------------
Shouldn’t we return false for the default implementation.

================
Comment at: include/llvm/Target/TargetInstrInfo.h:1186
@@ +1185,3 @@
+                                     unsigned Reg, Constant *Imm,
+                                     MachineBasicBlock &DestMBB) const {
+    report_fatal_error("Unimplemented");
----------------
I have mixed feeling about that method. We already have insert branch and I was wondering if it would make sense to have a insertCmp like method to achieve the same goal.
What other people think?

Assuming we go with that one, I would be more generic in the arguments. Instead of Imm what about a register. If that register happens to be holding an immediate, we should fold it in the optimizer. That being said, right now we do not need the register support so I can live with this specific version.

================
Comment at: include/llvm/Target/TargetInstrInfo.h:1187
@@ +1186,3 @@
+                                     MachineBasicBlock &DestMBB) const {
+    report_fatal_error("Unimplemented");
+  }
----------------
Return false instead of fatal error?
I guessing you want the error to tell the backend what they need to implement in order to use the pass, but I would rather use the documentation of the pass for that.

================
Comment at: include/llvm/Target/TargetInstrInfo.h:1191
@@ +1190,3 @@
+  /// Append a breakpoint instruction to the end of \p MBB.
+  virtual void EmitBreakpoint(MachineBasicBlock &MBB, DebugLoc DL) const {
+    report_fatal_error("Unimplemented");
----------------
Could you be more specific on what you expect the breakpoint instruction to do?

================
Comment at: include/llvm/Target/TargetInstrInfo.h:1192
@@ +1191,3 @@
+  virtual void EmitBreakpoint(MachineBasicBlock &MBB, DebugLoc DL) const {
+    report_fatal_error("Unimplemented");
+  }
----------------
Ditto.

================
Comment at: lib/CodeGen/VerifyCalleeSavedRegs.cpp:12
@@ +11,3 @@
+// registers they were supposed to preserve.  This is a best-effort pass, but it
+// does get many interesting cases.
+//
----------------
Use doxygen comment.

================
Comment at: lib/CodeGen/VerifyCalleeSavedRegs.cpp:50
@@ +49,3 @@
+                               MachineOperand &RegMask,
+                               SmallVectorImpl<unsigned> &Regs);
+
----------------
The name of the function is misleading because CSRs depends on ABI and this prototype does not allow to query that information (no function argument). What about gatherMaximalPreservedRegisters.

As a side note, the name of the argument changed from Regs in the declaration to CSRs in the definition.

================
Comment at: lib/CodeGen/VerifyCalleeSavedRegs.cpp:57
@@ +56,3 @@
+  /// terminator.
+  void updatePhysRegUses(MachineRegisterInfo &MRI,
+                         const TargetRegisterInfo &TRI,
----------------
Mark this last remark as a precondition using \pre.

================
Comment at: lib/CodeGen/VerifyCalleeSavedRegs.cpp:82
@@ +81,3 @@
+             1 &&
+         "Expected exactly one regmask operand!");
+  return *find_if(MI.operands(), IsRegMask);
----------------
The formatting looks strange to me, is this clang-formatted?

================
Comment at: lib/CodeGen/VerifyCalleeSavedRegs.cpp:93
@@ +92,3 @@
+
+    bool ShouldVerify = MRI.use_begin(Reg) == MRI.use_end() &&
+                        MRI.def_begin(Reg) == MRI.def_end();
----------------
Add a comment like “Check that this register is used or defined”.
BTW, can’t you use isPhysRegModified?

================
Comment at: lib/CodeGen/VerifyCalleeSavedRegs.cpp:95
@@ +94,3 @@
+                        MRI.def_begin(Reg) == MRI.def_end();
+    if (ShouldVerify)
+      for (MCSuperRegIterator SR(Reg, &TRI, false); SR.isValid(); ++SR) {
----------------
if (!ShouldVerify)
  return;

================
Comment at: lib/CodeGen/VerifyCalleeSavedRegs.cpp:96
@@ +95,3 @@
+    if (ShouldVerify)
+      for (MCSuperRegIterator SR(Reg, &TRI, false); SR.isValid(); ++SR) {
+        if (!RegMask.clobbersPhysReg(*SR) ||
----------------
Add some comments.
E.g.,
Check if any of its super register is used or defined.
If that is the case, this register is not maximal and we are done.

================
Comment at: lib/CodeGen/VerifyCalleeSavedRegs.cpp:100
@@ +99,3 @@
+            MRI.def_begin(*SR) != MRI.def_end()) {
+          ShouldVerify = false;
+          break;
----------------
return;

================
Comment at: lib/CodeGen/VerifyCalleeSavedRegs.cpp:106
@@ +105,3 @@
+    if (ShouldVerify)
+      for (MCSubRegIterator SR(Reg, &TRI, false); SR.isValid(); ++SR) {
+        if (MRI.use_begin(*SR) != MRI.use_end() ||
----------------
I don’t get why we need to do that a second time but with one less check.

================
Comment at: lib/CodeGen/VerifyCalleeSavedRegs.cpp:175
@@ +174,3 @@
+
+  // We don't handle invokes yet
+  if (AfterCallIt->isEHLabel())
----------------
Period.

================
Comment at: lib/CodeGen/VerifyCalleeSavedRegs.cpp:193
@@ +192,3 @@
+    if (RegSize > 8)
+      continue;
+
----------------
Document what it would take to implement wider types.
I believe this is just a matter of doing the right thing with the APInt object, right?

================
Comment at: lib/CodeGen/VerifyCalleeSavedRegs.cpp:263
@@ +262,3 @@
+    assert(BranchEmitted && "Expected InsertBranchIfInequal to be successful "
+                            "(since copyImmToPhysReg was fine)!");
+
----------------
Nowhere in the documentation we made this commitment.
I wouldn’t rely on it anyway :).


http://reviews.llvm.org/D21115





More information about the llvm-commits mailing list