Page MenuHomePhabricator

[SeparateConstOffsetFromGEP] garbage-collect intermediate instructions
ClosedPublic

Authored by jingyue on Apr 18 2015, 11:29 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

jingyue updated this revision to Diff 23991.Apr 18 2015, 11:29 PM
jingyue retitled this revision from to [SeparateConstOffsetFromGEP] garbage-collect intermediate instructions.
jingyue updated this object.
jingyue edited the test plan for this revision. (Show Details)
jingyue added a reviewer: meheff.
jingyue added subscribers: hfinkel, HaoLiu, Unknown Object (MLST).
meheff added inline comments.Apr 20 2015, 11:18 AM
lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp
624 ↗(On Diff #23991)

Is this reuse going to cause problems with your garbage collection? When you go to recusively delete UserChain you'll encounter this reused (and now live) instruction and deletion will stop there. However, deeper in the UserChain[] it seems like you could still have dead instructions. The problem would be is that these dead instructions are no longer in the expression tree for UserChain.back()

test/Transforms/SeparateConstOffsetFromGEP/NVPTX/split-gep.ll
1 ↗(On Diff #23991)

Should any extra checks be added to verify we're removing dead code?

jingyue updated this revision to Diff 24074.Apr 20 2015, 4:16 PM

removeConstOffset stops reusing instructions in UserChain

This ensures RecursivelyDeleteTriviallyDeadInstructions(UserChain.back()) can
remove all dead instructions in UserChain.

Added a -gep-reassociate-verify-no-dead-code flag to detect dead code. This flag is supposed to be used in test only.

test/Transforms/SeparateConstOffsetFromGEP/NVPTX/split-gep.ll
1 ↗(On Diff #23991)

Good catch! Fixed by cloning BO even if it can be reused.

meheff edited edge metadata.Apr 21 2015, 12:40 PM

LGTM with the addition of the new flag to the unit tests.

test/Transforms/SeparateConstOffsetFromGEP/NVPTX/split-gep-and-gvn.ll
2 ↗(On Diff #24074)

Did you mean to add your new dead code checking flag here?

test/Transforms/SeparateConstOffsetFromGEP/NVPTX/split-gep.ll
1 ↗(On Diff #24074)

And here?

jingyue updated this revision to Diff 24158.Apr 21 2015, 12:46 PM
jingyue edited edge metadata.

Done. Thanks for the review!

This revision was automatically updated to reflect the committed changes.