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

[Sanjoy Das via llvm-commits]

sanjoy added inline comments.

================
Comment at: include/llvm/Target/TargetInstrInfo.h:1186
@@ +1185,3 @@
+                                     unsigned Reg, Constant *Imm,
+                                     MachineBasicBlock &DestMBB) const {
+    report_fatal_error("Unimplemented");
----------------
qcolombet wrote:
> 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.
I've extracted out a `InsertCompareImm` method for now.

================
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");
----------------
qcolombet wrote:
> Could you be more specific on what you expect the breakpoint instruction to do?
I've stated that it is an instruction that raises a `SIGTRAP`.  Is that sufficient?

================
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.
+//
----------------
qcolombet wrote:
> Use doxygen comment.
Doesn't look like other files use doxygen comments for the header?  Is that a new convention?

================
Comment at: lib/CodeGen/VerifyCalleeSavedRegs.cpp:82
@@ +81,3 @@
+             1 &&
+         "Expected exactly one regmask operand!");
+  return *find_if(MI.operands(), IsRegMask);
----------------
qcolombet wrote:
> The formatting looks strange to me, is this clang-formatted?
Yes it is clang-formatted, but I've changed to code to not make it look odd anymore.

================
Comment at: lib/CodeGen/VerifyCalleeSavedRegs.cpp:86
@@ +85,3 @@
+
+void VerifyCalleeSavedRegs::gatherMaximalUnusedCSRs(
+    const TargetRegisterInfo &TRI, const MachineRegisterInfo &MRI,
----------------
I've refactored this function to make the logic more obvious.

================
Comment at: lib/CodeGen/VerifyCalleeSavedRegs.cpp:193
@@ +192,3 @@
+    if (RegSize > 8)
+      continue;
+
----------------
qcolombet wrote:
> 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?
The real issue here is that the callbacks don't support wider types and e.g. emitting the comparison for AVX registers has to be implemented in TII before we can use wider registers here.


http://reviews.llvm.org/D21115