Page MenuHomePhabricator

[InstCombine] Disable branch predicate canonicalization
AbandonedPublic

Authored by yrouban on Jul 24 2020, 12:17 AM.

Details

Summary

Introduce a flag to enable/disable the branch predicate canonicalization in InstCombine.
Fix all sensitive tests to pass with the flag set to /off.

This is the next step to move the branch predicate canonicalization transformation from InstCombine to SimplifyCFG. See the motivation in the description of the first patch D84491.

Diff Detail

Event Timeline

yrouban created this revision.Jul 24 2020, 12:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2020, 12:17 AM

Since out of all 3 patches this is the one with comments, i'll reply here.
I'm not a fan of this direction. You'll need to additionally cripple
canonicalizeICmpPredicate(), canFreelyInvertAllUsersOf(),
possibly their users. I'm not sure it conceptually makes sense
to do that in SimplifyCFG. But it for sure will restrict InstCombine.

Was the other proper solution of either updating PDT, or making it order-insensitive explored and turned out to be impossible?

Thank you for pointing this out. I will try changing canonicalizeICmpPredicate(), canFreelyInvertAllUsersOf().

Was the other proper solution of either updating PDT, or making it order-insensitive explored and turned out to be impossible?

Nobody proposed a good solution with a PDT calculation fix to make it insensitive to the branch order. PDT could not be updated properly either.

So in other words, instead of paying the price of recreating domtrees after instcombine,
we'll just shift the transforms to some other pass that already causes domtrees to be recreated, correct?

Thank you for pointing this out. I will try changing canonicalizeICmpPredicate(), canFreelyInvertAllUsersOf().

Was the other proper solution of either updating PDT, or making it order-insensitive explored and turned out to be impossible?

Nobody proposed a good solution with a PDT calculation fix to make it insensitive to the branch order. PDT could not be updated properly either.

<..> you have to chose some arbitrary way of selecting a fake-entry node from nodes in an scc. Right now this order is whatever children or inverse_children return, but could be based on the order of blocks in a function instead.

yrouban planned changes to this revision.Jul 24 2020, 5:15 AM

So in other words, instead of paying the price of recreating domtrees after instcombine,
we'll just shift the transforms to some other pass that already causes domtrees to be recreated, correct?

I have just proposed a solution alternative to D81089. The intention is clear: do CFG changes in SimplifyCFG instead of InstCombine. Strictly speaking PreserveCFG must not be reported if we change terminators in any way.

<..> you have to chose some arbitrary way of selecting a fake-entry node from nodes in an scc. Right now this order is whatever children or inverse_children return, but could be based on the order of blocks in a function instead.

Ok. Let us try now. But why did not they do it a few years ago then?

<..> you have to chose some arbitrary way of selecting a fake-entry node from nodes in an scc. Right now this order is whatever children or inverse_children return, but could be based on the order of blocks in a function instead.

Ok. Let us try now. But why did not they do it a few years ago then?

We just missed this IIRC :)

lebedev.ri requested changes to this revision.Aug 5 2020, 4:05 AM

Abandon now that D84763 has resolved the base problem?

yrouban abandoned this revision.Aug 13 2020, 8:52 AM