Page MenuHomePhabricator

[UnifyFunctionExitNodes] Fix Modified status for unreachable blocks
ClosedPublic

Authored by dstenb on Aug 12 2020, 1:47 AM.

Details

Summary

If a function had at most one return block, the pass would return false
regardless if an unified unreachable block was created.

This patch fixes that by refactoring runOnFunction into two separate
helper functions for handling the unreachable blocks respectively the
return blocks, as suggested by @bjope in a review comment.

This was caught using the check introduced by D80916.

Diff Detail

Event Timeline

dstenb created this revision.Aug 12 2020, 1:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2020, 1:47 AM
dstenb requested review of this revision.Aug 12 2020, 1:47 AM

Gentle ping. I was not sure which reviews to add to this one, since the code has mostly remained unchanged since its original introduction in 2004, except for it being affected by wider changes.

bjope added a subscriber: bjope.Sep 1 2020, 6:30 AM
bjope added inline comments.
llvm/lib/Transforms/Utils/UnifyFunctionExitNodes.cpp
42–43

Slightly unrelated, but this looks wrong.

I assume it is describing what this function is returning (not what the singular exit node for the function being compiled is returning). But either way it is incorrect.

As this is an ordinary runOnFunction method I don't think it need to say anything special about what it returns (or it should say whatever other runOnFunction methods are saying).

43

One idea (a bit larger refactoring) is to split this function by adding two new helpers, so the code in runOnFunction would look like:

{
  bool Changed = false;
  Changed |= unifyUnreachableBlocks();
  Changed |= unifyReturnBlocks();
  return Changed;
}

That would be nice since there doesn't seem to really be any dependency between the handling of unreachable and return blocks. And you wouldn't need to care about Changed status from one analysis inside the other.

dstenb updated this revision to Diff 289699.Sep 3 2020, 6:23 AM
dstenb edited the summary of this revision. (Show Details)

Address review comments.

dstenb added a reviewer: bjope.
dstenb marked 2 inline comments as done.
dstenb added inline comments.
llvm/lib/Transforms/Utils/UnifyFunctionExitNodes.cpp
43

That seems worthwhile! I did that in the latest diff.

bjope added inline comments.Sep 4 2020, 9:42 AM
llvm/lib/Transforms/Utils/UnifyFunctionExitNodes.cpp
104

Assuming intention was to do return Changed; here.

llvm/test/Transforms/UnifyFunctionExitNodes/unreachable-blocks-status.ll
4

I guess one would have neeed to enable EXPENSIVE_CHECKS for this test case to fail in the past (but only since the check was addded to EXPENSIVE_CHECKS). Could it be worth the trouble to use -debug-pass=Details and check that the pass returns that it has modified both @foo and @bar? In some sense getting rid of the dependency to EXPENSIVE_CHECKS.

On the other hand, I'm not sure if -debug-pass=Details works with new-pm. But mergereturn has not been ported to the new-pm either, so I'm not sure about the future plans for the pass. Maybe we shouldn't spend too much time here. So I'm happy either way.

serge-sans-paille accepted this revision.Sep 9 2020, 1:52 AM

LGTM

llvm/test/Transforms/UnifyFunctionExitNodes/unreachable-blocks-status.ll
4

I think the test is fine as is, if it was triggering the error before that patch under EXPENSIVE_CHECKS

This revision is now accepted and ready to land.Sep 9 2020, 1:52 AM
dstenb marked 2 inline comments as done.Sep 9 2020, 4:31 AM
dstenb added inline comments.
llvm/lib/Transforms/Utils/UnifyFunctionExitNodes.cpp
104

Yes, thanks! I'll fix it when landing.

This revision was automatically updated to reflect the committed changes.
dstenb marked an inline comment as done.