This is an archive of the discontinued LLVM Phabricator instance.

[NewGVN] Mark function as changed if we erase instructions.
ClosedPublic

Authored by fhahn on Aug 23 2018, 8:55 AM.

Details

Summary

Currently eliminateInstructions only returns true if any instruction got
replaced. In the test case for this patch, we eliminate the trivially
dead calls, for which eliminateInstructions not do a replacement and the
function is not marked as changed, which is why the inliner crashes
while traversing the call graph.

Alternatively we could also change eliminateInstructions to return true
in case we mark instructions for deletion, but that's slightly more code
and doing it at the place where the replacement happens seems safer.

Fixes PR37517.

Diff Detail

Repository
rL LLVM

Event Timeline

fhahn created this revision.Aug 23 2018, 8:55 AM
bjope accepted this revision.Sep 6 2018, 1:38 AM
bjope added a subscriber: bjope.

An alternative could perhaps be to do something like this

Changed |= !InstructionsToErase.empty();

before the loop (assuming that if we got something in InstructionsToErase then we also have done some modifications).
But I think your solution (setting Changed whenever we do a change) is easier to understand.

Thanks for the patch. LGTM!

This revision is now accepted and ready to land.Sep 6 2018, 1:38 AM
fhahn added a comment.Sep 6 2018, 7:26 AM

An alternative could perhaps be to do something like this

Changed |= !InstructionsToErase.empty();

before the loop (assuming that if we got something in InstructionsToErase then we also have done some modifications).
But I think your solution (setting Changed whenever we do a change) is easier to understand.

Thanks for the patch. LGTM!

Thanks! I just had another look and I think even though we have 2 ifs in the loop over InstructionsToErase, we should always do modifications if it is not empty. In particular, if an instruction to erase does not have a parent any more, that would mean the parent BB has been deleted earlier, so we already did modifications. I think I'll go with your suggestion!

fhahn updated this revision to Diff 164217.Sep 6 2018, 7:47 AM
bjope accepted this revision.Sep 6 2018, 8:33 AM

Still LGTM!

This revision was automatically updated to reflect the committed changes.