[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