This is an archive of the discontinued LLVM Phabricator instance.

[RS4GC] Prune inputs of BDV if they are BDV themselves
ClosedPublic

Authored by dmakogon on Apr 15 2022, 1:42 AM.

Details

Summary

There's a stage in RS4GC when we remove all nodes from the state list if all of their inputs are base pointers.
Consider the following PHI:

loop:
  %ph = phi i8* [ %ph, %loop ], [ %b, %block ]

Say %b is a base value. Then we should remove the %ph from the states list, because it's equal to %b.
This patch just skips all the input values if they are equal to the value currently being processed.

If in the example above we had a user of %ph for which we were asked to find a base value,
we'd clone that user and replaced %ph uses with the %b base value.
With the patch %ph is treated as a base pointer, so no cloning happens.

Diff Detail

Event Timeline

dmakogon created this revision.Apr 15 2022, 1:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2022, 1:42 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
dmakogon requested review of this revision.Apr 15 2022, 1:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2022, 1:42 AM
mkazantsev added inline comments.Apr 15 2022, 2:39 AM
llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
939

What if V->stripPointerCasts() == BDV, will this still return false?

mkazantsev added a comment.EditedApr 15 2022, 2:44 AM

Will this patch handle the following example:

loop: 
  %outer_phi = phi (%start, %inner_phi)
  br inner_loop

inner_loop:
  %inner_phi = phi(%start, %outer_phi)
  %inner.cond = <smth>
  br %inner.cond, inner_loop, outer_backedge

outer_backedge:
  outer_cond = ...
  br outer_cond, loop, exit

exit:
...

Here both inner_phi and outer_phi are in fact equal to one another (and both equal to start), but still different values. Will it work? If no, is there a plan to make it work?

mkazantsev added inline comments.Apr 15 2022, 2:49 AM
llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
939

One possible way to get that is

%ph = phi i8* [ %ph2, %loop ], [ %b, %block ]
%ph1 = bitcast %ph to i32*
%ph2 = bitcast %ph1 to i8*
reames accepted this revision.Apr 16 2022, 8:30 AM

LGTM

llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
937–942

Please rephrase this comment to emphasize this is handling the phi case (as no other instruction can have itself as operand in reachable code).

This revision is now accepted and ready to land.Apr 16 2022, 8:30 AM

Will this patch handle the following example:

loop: 
  %outer_phi = phi (%start, %inner_phi)
  br inner_loop

inner_loop:
  %inner_phi = phi(%start, %outer_phi)
  %inner.cond = <smth>
  br %inner.cond, inner_loop, outer_backedge

outer_backedge:
  outer_cond = ...
  br outer_cond, loop, exit

exit:
...

Here both inner_phi and outer_phi are in fact equal to one another (and both equal to start), but still different values. Will it work? If no, is there a plan to make it work?

No, these PHIs won't be removed at this stage of RS4GC.
However there won't be any cloning here. During states meet phase we will figure out that they both have the %start base. Merging two equal bases is not a conflict state.

dmakogon updated this revision to Diff 423655.Apr 19 2022, 9:30 AM

Added more test cases with derived pointers.
Fixed comment message.

dmakogon marked 3 inline comments as done.Apr 19 2022, 9:31 AM
mkazantsev accepted this revision.Apr 20 2022, 4:15 AM
This revision was landed with ongoing or failed builds.Apr 26 2022, 2:05 AM
This revision was automatically updated to reflect the committed changes.