This is an archive of the discontinued LLVM Phabricator instance.

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

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.

2369

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

2770

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)
2770

Please don't use statepoint here.

test/Verifier/statepoint.ll
55

add: gc "statepoint-example"

69

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

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

2787

I think this is checked elsewhere.

2798

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

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

2806

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
2770

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

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.