This is an archive of the discontinued LLVM Phabricator instance.

[RewriteStatepointsForGC] Fix a relocation bug w.r.t values defined by invoke instructions
ClosedPublic

Authored by chenli on Feb 26 2015, 4:47 PM.

Details

Summary

RewriteStatepointsForGC pass emits an alloca for each GC pointer which will be relocated. It then inserts stores after def and all relocations, and inserts loads before each use as well. In the end, mem2reg is used to update IR with relocations in SSA form.

However, there is a problem with inserting stores for values defined by invoke instructions. The code didn't expect a def was a terminator instruction, and inserting instructions after these terminators resulted in malformed IR.

This patch fixes this problem by handling invoke instructions as a special case. If the def is an invoke instruction, the store will be inserted at the beginning of the normal destination block. Since return value from invoke instruction does not dominate the unwind destination block, no action is needed there.

Diff Detail

Event Timeline

chenli updated this revision to Diff 20805.Feb 26 2015, 4:47 PM
chenli retitled this revision from to [RewriteStatepointsForGC] Fix a relocation bug w.r.t values defined by invoke instructions.
chenli updated this object.
chenli edited the test plan for this revision. (Show Details)
chenli added a reviewer: reames.
chenli added a subscriber: Unknown Object (MLST).
sanjoy added a subscriber: sanjoy.Feb 26 2015, 5:36 PM

Comments inline.

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
1569

Minor nit: currently the abort string will be "The only ... a value is < several spaces > InvokeInst which ...". I think you can just split the string literal here:

assert(!cast<Instruction>(def)->isTerminator() &&
       "The only TerminatorInst that can produce a value is "
       "InvokeInst which is handled above.");
1571

You have an isa<Instruction>(def) followed by two cast<Instruction>(def)s. It might just be better to dyn_cast to an Instruction in the condition like you've done for the InvokeInst.

chenli updated this revision to Diff 21049.Mar 2 2015, 3:13 PM

Update patch w.r.t Sanjoy's comments.

reames added inline comments.Mar 2 2015, 3:31 PM
test/Transforms/RewriteStatepointsForGC/relocate_invoke_result.ll
2

Please use FileCheck to check the output of this test.

chenli updated this revision to Diff 21057.Mar 2 2015, 3:55 PM

Update test case with FileCheck w.r.t Philip's comments.

reames edited edge metadata.Mar 2 2015, 5:16 PM

LGTM. Do you have commit access or should I submit for you?

Hi Philip,
I don't have commit access. Please help me submit it. Thanks!

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
1571

Yes, I will clean up this part.

This revision was automatically updated to reflect the committed changes.