Page MenuHomePhabricator

[IROutliner] IR Outliner can mix up order of incoming values when compressing phi nodes if contain the same values
ClosedPublic

Authored by AndrewLitteken on Mar 9 2022, 10:53 AM.

Details

Summary

Github issue: https://github.com/llvm/llvm-project/issues/54211

Example: https://godbolt.org/z/KrKPojnqE

When matching PHINodes when margining functions the IROutliner only checks that an incoming value exists in phi node in overall function. It doesn't check the length, the order, or that the incoming block also matches. In the given example, we see that both phi nodes have the same incoming values, but from different blocks.

The fix is to to enforce stricter a match of the incoming value, and the incoming block as well when matching the created phi nodes.

Diff Detail

Event Timeline

AndrewLitteken created this revision.Mar 9 2022, 10:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2022, 10:53 AM
AndrewLitteken requested review of this revision.Mar 9 2022, 10:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2022, 10:53 AM
paquette added inline comments.Mar 9 2022, 11:43 AM
llvm/lib/Transforms/IPO/IROutliner.cpp
1599–1603
  • Nit: I don't think we need an initial value for SmallVector anymore
  • Could use a comment explaining what the variable is? (Probably the same as findCanonNumsForPHI, but future readers may not see that.)
1629–1664

Could use a comment too?

1633

Comment on what you're trying to do here?

1681

findCorrespondingBlockIn mentions the value could be nullptr. Assert?

llvm/test/Transforms/IROutliner/different-order-phi-merges.ll
5

Updating with nits

Updating with context

This revision is now accepted and ready to land.Mar 14 2022, 10:58 AM
This revision was landed with ongoing or failed builds.Mar 14 2022, 2:48 PM
This revision was automatically updated to reflect the committed changes.