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

[Matthias Braun via llvm-commits]

MatzeB added a comment.

I fear that this pass will not be used much and is in danger of bitrotting. I hope you have some internal bots or something that use the code from time to time...


================
Comment at: lib/CodeGen/VerifyCalleeSavedRegs.cpp:13
@@ +12,3 @@
+// does get many interesting cases.
+//
+//===----------------------------------------------------------------------===//
----------------
The convention is often broken, but not always. In any way it is helpful and also mentioned in the coding standards.

================
Comment at: lib/CodeGen/VerifyCalleeSavedRegs.cpp:28
@@ +27,3 @@
+
+#define DEBUG_TYPE "verify-calling-convs"
+
----------------
"verify-callee-saved-regs" to match the passname?

================
Comment at: lib/CodeGen/VerifyCalleeSavedRegs.cpp:99-100
@@ +98,4 @@
+         ++SR)
+      if (MRI.isPhysRegUsed(Reg))
+        return false;
+    for (MCSubRegIterator SR(Reg, &TRI); SR.isValid(); ++SR)
----------------
This seems like an unnecessary pessimization of the verifier to me. The thing triggering isPhysRegUsed() may be elsewhere in the function. Why not check the implicit-use/-def operands of the call for physregs that are used for parameter/result passing?

I would also exclude reserved registers just to be sure, even if they are not reported as preserved in any of the existing backends.

================
Comment at: lib/CodeGen/VerifyCalleeSavedRegs.cpp:128-129
@@ +127,4 @@
+
+  for (auto &BB : MF)
+    for (auto &MI : BB) {
+      if (!MI.isCall())
----------------
Too much `auto` for my taste. `MachineBasicBlock&` / `MachineInstr&/ MachineRegisterInfo&` etc. are not much longer to write and friendlier to the reader IMO.

================
Comment at: lib/CodeGen/VerifyCalleeSavedRegs.cpp:139-140
@@ +138,4 @@
+
+  FrameSetupOp = TII.getCallFrameSetupOpcode();
+  FrameDestroyOp = TII.getCallFrameDestroyOpcode();
+
----------------
Why are MRI, TRI, TII, Ctx are passed around as arguments to the functions, while these two are class members?

================
Comment at: lib/CodeGen/VerifyCalleeSavedRegs.cpp:220-223
@@ +219,6 @@
+  // out of the return value, if any.
+  auto AfterCopiesIt = AfterCallIt;
+  while (AfterCopiesIt != CallMBB->end() &&
+         AfterCopiesIt->getOpcode() == TargetOpcode::COPY)
+    AfterCopiesIt++;
+
----------------
This could include additional COPYs unrelated to the call. I would at least tighten this a bit to only walk over COPYs with physregs as source.


http://reviews.llvm.org/D21115