This is an archive of the discontinued LLVM Phabricator instance.

[SimplifyCFG] Don't merge invoke if this makes immarg non-constant (PR61265)
ClosedPublic

Authored by nikic on Mar 23 2023, 7:26 AM.

Details

Summary

Don't merge invokes if this replaces constant operands with phis in a place where this is not legal.

@mkazantsev Could you please check the change to merge-deopt-bundle-constants.ll? Was this test added to check that differing deopt bundles aren't merged but then never fixed, or was it added to check that they are merged? If the latter, then I'll adjust canReplaceOperandWithVariable() to special-case deopt.

Fixes https://github.com/llvm/llvm-project/issues/61265.

Diff Detail

Event Timeline

nikic created this revision.Mar 23 2023, 7:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2023, 7:26 AM
nikic requested review of this revision.Mar 23 2023, 7:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2023, 7:26 AM
mkazantsev accepted this revision.EditedMar 23 2023, 9:33 PM

@nikic if I remember this correctly, the story here is following:

Because semantics of deopt bundle is defined by frontend/runtime, it may have some specific requirement, e.g. something is ought to be a constant known in compile time. If there is a transform that replaces constants with non-constants, you have two options:

  • Disallow this (I guess this is what your patch is doing);
  • Allow it, but undo this transform later (this is what we did a year ago in downstream code when this test was introduced) when the fact of constantness starts to matter;

Personally I don't have a strong preference which approach we take, prohibition of such transforms seems even less headache to me.

This revision is now accepted and ready to land.Mar 23 2023, 9:33 PM
nikic added a comment.Mar 24 2023, 6:27 AM

@mkazantsev Thanks for the context! In that case I'll go with this patch, as it seems like we are already disallowing this everywhere apart from this one fold, so it makes more sense to be consistent.