Page MenuHomePhabricator

Handle successor's PHI node correctly when flattening CFG merges two if-regions
ClosedPublic

Authored by jaebaek on Sep 25 2019, 9:14 AM.

Details

Summary

FlattenCFG merges two 'if' basicblocks by inserting one basicblock
to another basicblock. The inserted basicblock can have a successor
that contains a PHI node whoes incoming basicblock is the inserted
basicblock. Since the existing code does not handle it, it becomes
a badref.

if (cond1)

statement

if (cond2)

statement

successor - contains PHI node whose predecessor is cond2

-->
if (cond1 || cond2)

statement

(BB for cond2 was deleted)
successor - contains PHI node whose predecessor is cond2 --> bad ref!

Diff Detail

Repository
rL LLVM

Event Timeline

jaebaek created this revision.Sep 25 2019, 9:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2019, 9:14 AM
kuhar added inline comments.Sep 25 2019, 10:50 AM
llvm/lib/Transforms/Utils/FlattenCFG.cpp
457 ↗(On Diff #221786)

nit: for (unsigned int i = 0; i < PBI->getNumSuccessors(); ++i)
->
for (BasicBlock *Succ : successors(PBI))

459 ↗(On Diff #221786)

I think this should iterate over all the phi nodes in the successor basic block?
You can get that with: BB->phis()

460 ↗(On Diff #221786)

LLVM prefers quick exists; I think it would be better to use:

if (!SomePhi)
  continue;
461 ↗(On Diff #221786)

nit: for (unsigned int j = 0; j < SomePHI->getNumIncomingValues(); ++j)
-->
for (unsigned i = 0, e = SomePHI->getNumIncomingValues(); i != e; ++i)

jaebaek marked 4 inline comments as done.Sep 25 2019, 2:00 PM
jaebaek updated this revision to Diff 221832.Sep 25 2019, 2:02 PM

Updated based on reviews. PTAL

kuhar accepted this revision.Sep 25 2019, 2:17 PM

Seems fine to me. Thanks for the fixes.
Please wait some time and see if others are OK with this change too.

This revision is now accepted and ready to land.Sep 25 2019, 2:17 PM
This revision was automatically updated to reflect the committed changes.