[PATCH] D71164: [InstCombine] Fix infinite loop due to bitcast <-> phi transforms

Nikita Popov via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Dec 9 12:16:38 PST 2019


nikic planned changes to this revision.
nikic marked an inline comment as done.
nikic added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp:2321
+    // Make sure the old phis are cleaned up.
+    Worklist.Add(OldPN);
   }
----------------
cwabbott wrote:
> spatel wrote:
> > Can we assert that OldPN has no users at this point?
> This isn't true until D71209 lands, and therefore the problem could still happen because `OldPN` won't actually get cleaned up. Even afterwards, there could be a PHI web hanging around. In my patch I assumed that some other dead-code pass would come along and delete it eventually, but I don't know if InstCombine is smart enough to do that before folding the load. Maybe you can just remove all the old phi nodes manually once my patch lands?
Yes, we can't assert that that there are no users here, as there may still be phi users.

I generally agree that it may be safer to explicitly remove the old phi nodes -- InstCombine can clean up dead phi cycles, but I'm not sure whether there are any limitations to that. Doing a final `replaceInstUsesWith` UndefValue on all the old phis would be the safest option.

I'll hold off on this until your patch lands, as doing an undef replacement wouldn't be safe without it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71164/new/

https://reviews.llvm.org/D71164





More information about the llvm-commits mailing list