Page MenuHomePhabricator

Verification of invoke statepoints
ClosedPublic

Authored by igor-laevsky on Feb 2 2015, 4:10 PM.

Details

Summary

This changes implements invoke statepoint verification. It depends on the change in Statepoint.h (http://reviews.llvm.org/D7364) as it uses some of the helper functions form there.

Diff Detail

Repository
rL LLVM

Event Timeline

igor-laevsky retitled this revision from to Verification of invoke statepoints.
igor-laevsky updated this object.
igor-laevsky edited the test plan for this revision. (Show Details)
igor-laevsky added a reviewer: reames.
igor-laevsky set the repository for this revision to rL LLVM.
igor-laevsky added a subscriber: Unknown Object (MLST).
reames edited edge metadata.Feb 3 2015, 9:16 AM

Comments inline.

lib/IR/Verifier.cpp
1862 ↗(On Diff #19202)

Add a TODO here. We'd really should be using visitIntrinsicFunctionCall here. So should patchpoint. It just happens that's phrased in CallInst and not worth updating.

2366 ↗(On Diff #19202)

Given the structure of this clause, I'd prefer to see this as a test against the intrinsic id directly.

2767 ↗(On Diff #19202)

IsLandingpad doesn't actually check this is a landing pad? Possibly IsTiedToInvoke? (better: see comment below about organization)

Meta comments:

  • I really don't like this organization. I'd suggest:

// structural checks only...
if (isa<IntrinsicInst>(arg_0)) {

// call statepoint case

} else if (isa<ExtractElementInst>()) {

// invoke

} else { report error }
Given the statepoint..
rest of code

  • Your adding usage of the statepoint utility function in places which are deliberately not using them. (i.e. that want to report more fine grained errors)
2798 ↗(On Diff #19202)

Please don't use statepoint here.

test/Verifier/statepoint.ll
55 ↗(On Diff #19202)

add: gc "statepoint-example"

69 ↗(On Diff #19202)

Does this extra branch add anything to this test?

igor-laevsky edited edge metadata.
reames added inline comments.Feb 5 2015, 10:24 AM
lib/IR/Verifier.cpp
2772 ↗(On Diff #19372)

I'd suggest using dyn_cast here. It will simplify several parts of the following code.

2787 ↗(On Diff #19372)

I think this is checked elsewhere.

2798 ↗(On Diff #19372)

The cast needs to be check in a way you don't crash here. :)

auto Op0 = ...
isa<Instruction>(Op0) && ...

2806 ↗(On Diff #19372)

This can be more cleanly written as:
ImmutableCallSite StatepointCS(ops.statepoint());

LGTM, would you like me to commit on your behalf?

Yes, please. Please note that this patch depends on http://reviews.llvm.org/D7364

lib/IR/Verifier.cpp
2798 ↗(On Diff #19202)

In this case it's fine to use ops.statepoint because we already verified that gc relocate is correctly tied to the statepoint. I made this more clear in the updated change.

2787 ↗(On Diff #19372)

It is checked at the visitBasicBlock, but I didn't want to rely on the basic block visiting order.

This revision was automatically updated to reflect the committed changes.