Page MenuHomePhabricator

[CGP] Prevent optimizePhiType from iterating forever
ClosedPublic

Authored by dmgreen on Jun 26 2020, 12:11 PM.

Details

Summary

The recently added optimizePhiType algorithm had no checks to make sure it didn't continually iterate backward and forth between float and int types. This means that given an input like store(phi(bitcast(load))), we could convert that back and forth to store(bitcast(phi(load))). This particular case would usually have been simplified to a different load type (folding the bitcast into the load) before CGP, but other cases can occur. The one that came up was phi(bitcast(phi)), where the two phi's of different types were bitcast between. That was not helped by a dead bitcast being kept around which could make conversion look profitable.

This adds an extra check of the bitcast Uses or Defs, to make sure that at least one is "grounded" and will not end up being converted back. It also makes sure that dead bitcasts are removed, and there is a minor change to include newly created Phi nodes in the Visited set so that they do not need to be revisited.

Diff Detail

Event Timeline

dmgreen created this revision.Jun 26 2020, 12:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2020, 12:11 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
tpopp resigned from this revision.Jul 1 2020, 2:54 AM

Thank you for handling this case and the patience in waiting for the reproducer when I originally rolled this back. I'm resigning as a reviewer just to make it clear that I'm not qualified to review this change.

^ Thanks for the help anyway!

dmgreen updated this revision to Diff 291247.Sep 11 2020, 9:40 AM

I've tried to rewrite this to something less convoluted. Hopefully it's a little more straightforward than the nested code from before.

efriedma accepted this revision.Sep 11 2020, 4:16 PM

LGTM

llvm/lib/CodeGen/CodeGenPrepare.cpp
5900

Please use braces on both sides of the if/else.

This revision is now accepted and ready to land.Sep 11 2020, 4:16 PM
This revision was automatically updated to reflect the committed changes.