Page MenuHomePhabricator

Correctly modify the CFG in IfConverter, and then remove the CorrectExtraCFGEdges function.
ClosedPublic

Authored by jyknight on Wed, May 6, 4:19 PM.

Details

Summary

The latter was a workaround for "Various pieces of code" leaving bogus
extra CFG edges in place. Where by "various" it meant only
IfConverter::MergeBlocks, which failed to clear all of the successors
of dead blocks it emptied out. This wouldn't matter a whole lot,
except that the dead blocks remained listed as predecessors of
still-useful blocks, inhibiting optimizations.

This fix slightly changed two thumb tests, because the correct CFG
successors allowed for the "diamond" if-conversion pattern to be
detected, when it could only use "simple" before.

Additionally, the removal of a now-redundant call to analyzeBranch
(with AllowModify=true) in BranchFolder::OptimizeFunction caused a
later check for an empty block in BranchFolder::OptimizeBlock to
fail. Correct this by moving the call to analyzeBranch in
OptimizeBlock higher.

Diff Detail

Event Timeline

jyknight created this revision.Wed, May 6, 4:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, May 6, 4:19 PM
efriedma added inline comments.Wed, May 6, 5:00 PM
llvm/lib/CodeGen/IfConversion.cpp
2292

I think this contradicts the documentation for this function? "This will leave FromBB as an empty block, so remove all of its successor edges except for the fall-through edge." Maybe this is better, but please make it consistent.

jyknight updated this revision to Diff 262627.Thu, May 7, 5:55 AM
jyknight marked 2 inline comments as done.

update function comment.

llvm/lib/CodeGen/IfConversion.cpp
2292

Fixed the doc, thanks. Keeping the existing fallthrough successor didn't really make any sense as this function moves the block to the end of the function.

efriedma accepted this revision.Thu, May 7, 11:33 AM

LGTM . (You probably want to try running ninja check with an EXPENSIVE_CHECKS build before merging.)

This revision is now accepted and ready to land.Thu, May 7, 11:33 AM
This revision was automatically updated to reflect the committed changes.