Page MenuHomePhabricator

Add a callee-saved register verifier to LLVM
AbandonedPublic

Authored by sanjoy on Jun 7 2016, 6:38 PM.

Details

Summary

This change adds a pass to LLVM to (optionally) generate code around
function calls to check, at runtime, that the function preserves the
registers it is expected to preserve. The motivation for this was
covered on the list a couple of weeks ago [0], but generally the
intention is to support CSR verification around calls to hand-written
assembly stubs that are common in managed runtimes.

I'm not familiar with much of the areas of LLVM I've touched in this
patch, so a careful review will be appreciated.

[0]: http://lists.llvm.org/pipermail/llvm-dev/2016-May/099693.html

Diff Detail

Event Timeline

sanjoy updated this revision to Diff 59985.Jun 7 2016, 6:38 PM
sanjoy retitled this revision from to Add a callee-saved register verifier to LLVM.
sanjoy updated this object.
sanjoy added reviewers: qcolombet, MatzeB, dblaikie, atrick.
sanjoy added a subscriber: llvm-commits.
mehdi_amini edited edge metadata.Jun 8 2016, 1:25 PM

(summary from our IRC discussion)

I understand that is easier to do pre-RA, but I'm sad it won't help to validate the inter-procedural register allocation we're working on these days, because this will clobbered every physical registers and limit the analysis.
If other backend-gurus have a solution that could be easily implemented post-RA, I'd love it :)

qcolombet added inline comments.Jun 8 2016, 1:51 PM
include/llvm/InitializePasses.h
342

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.

include/llvm/Target/TargetInstrInfo.h
764

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.

769

Shouldn’t we return false for the default implementation.

1186

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.

1187

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.

1191

Could you be more specific on what you expect the breakpoint instruction to do?

1192

Ditto.

lib/CodeGen/VerifyCalleeSavedRegs.cpp
12

Use doxygen comment.

50

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.

57

Mark this last remark as a precondition using \pre.

82

The formatting looks strange to me, is this clang-formatted?

93

Add a comment like “Check that this register is used or defined”.
BTW, can’t you use isPhysRegModified?

95

if (!ShouldVerify)

return;
96

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.

100

return;

106

I don’t get why we need to do that a second time but with one less check.

175

Period.

193

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?

263

Nowhere in the documentation we made this commitment.
I wouldn’t rely on it anyway :).

sanjoy updated this revision to Diff 60393.Jun 10 2016, 1:23 PM
sanjoy marked 14 inline comments as done.
sanjoy edited edge metadata.
  • Address review
sanjoy added inline comments.Jun 12 2016, 2:25 PM
include/llvm/Target/TargetInstrInfo.h
1186

I've extracted out a InsertCompareImm method for now.

1191

I've stated that it is an instruction that raises a SIGTRAP. Is that sufficient?

lib/CodeGen/VerifyCalleeSavedRegs.cpp
12

Doesn't look like other files use doxygen comments for the header? Is that a new convention?

82

Yes it is clang-formatted, but I've changed to code to not make it look odd anymore.

86

I've refactored this function to make the logic more obvious.

193

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.

MatzeB edited edge metadata.Jun 16 2016, 7:10 PM

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...

lib/CodeGen/VerifyCalleeSavedRegs.cpp
14

The convention is often broken, but not always. In any way it is helpful and also mentioned in the coding standards.

29

"verify-callee-saved-regs" to match the passname?

100–101

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.

129–130

Too much auto for my taste. MachineBasicBlock& / MachineInstr&/ MachineRegisterInfo& etc. are not much longer to write and friendlier to the reader IMO.

140–141

Why are MRI, TRI, TII, Ctx are passed around as arguments to the functions, while these two are class members?

221–224

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.

sanjoy abandoned this revision.Sep 30 2016, 11:14 AM

I'm not actively working on this now. Will reopen if that changes.