Page MenuHomePhabricator

[InstCombine] Handle gc.relocate(null) in one iteration

Authored by skatkov on Mar 4 2020, 2:32 AM.



InstCombine adds users of transformed instruction to working list to
process on the same iteration. However gc.relocate may have a hidden
user (next gc.relocate) which is connected through gc.statepoint intrinsic.

In this case if the next gc.relocation is already processed it will not be added
to worklist and will not be able to be processed on the same iteration.
Let's we have the following case:
A = gc.relocate(null)
B = statepoint(A)
C = gc.relocate(B, hidden(A))
If C is already considered then after replacement of A with null, statepoint B
instruction will be added to the queue but not C.
C can be processed only on the next iteration.

If the chain of relocation is pretty long the many iteration may be required.
So this change is mostly compiler time optimization.

Diff Detail

Event Timeline

skatkov created this revision.Mar 4 2020, 2:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2020, 2:32 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
skatkov marked an inline comment as done.Mar 4 2020, 2:36 AM
skatkov added inline comments.

safe -> save.

skatkov updated this revision to Diff 248132.Mar 4 2020, 2:49 AM

Can gc.relocate(null) be constant folded? I quickly glanced over code and it seems it's not. Could you double-check it?
After quick look through initial worklist construction in AddReachableCodeToWorklist from Transforms/InstCombine/InstructionCombining.cpp, it seems to me that if gc.relocate(null) was constant folded, this problem would not exist.
Am I missed something here?

reames added a comment.Mar 4 2020, 4:23 PM

I don't think this is quite the right factoring, for two reasons:

  1. This applies to the other gc_relocate transforms as well (e.g. undef, non-null, etc..)
  2. This applies in the other direction to a chain of dead gc.relocates. (Actually, it looks like we don't recurse the chain at all?)

I'm not sure on the right approach here. One idea would be to add a gc_statepoint case to the intrinsic switch and phrase many of these transforms as a change to the statepoint operand list. (i.e. we can remove null, undef, and duplicate entries and thus have to renumber gc.relocates.)

One way to think about it is that the "statepoint" is really the whole sequence. And as a result, we're not doing a transform on one instruction at a time, but the whole "unit" of instructions. Not sure I really like that though.

skatkov abandoned this revision.Mar 12 2020, 10:46 PM

Need to think about other approach