This is an archive of the discontinued LLVM Phabricator instance.

[rs4gc] don't duplicate existing values which are provably base pointers
ClosedPublic

Authored by reames on Mar 6 2021, 11:31 AM.

Details

Summary

RS4GC needs to rewrite the IR to ensure that every relocated pointer has an associated base pointer. The existing code isn't particularly smart about avoiding duplication of existing IR when it turns out the original pointer we were asked to materialize a base pointer for is itself a base pointer.

This patch adds a stage to the algorithm which prunes nodes proven (with a simple forward dataflow fixed point) to be base pointers from the list of nodes considered for duplication. This does require changing some of the later invariants slightly, that's probably the riskiest part of the change.

Note: The version posted is slightly WIP. I need to confirm I can back out the recent deopt value change, and add more targeted testing of cornercases in the new inference. I'm posting it now to get feedback in the context of the ongoing discussion on D97837. Planning to rebase with mentioned changes Monday.

Diff Detail

Event Timeline

reames created this revision.Mar 6 2021, 11:31 AM
reames requested review of this revision.Mar 6 2021, 11:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2021, 11:31 AM
skatkov added inline comments.Mar 9 2021, 1:18 AM
llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
947

First BaseValue seems to be redundant.

skatkov added inline comments.Mar 9 2021, 1:29 AM
llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
909

why not
do {} while(!ToRemove.empty());
?

925

not a fan of &= operation. It will require to execute the function call even if CanPrune is already false.
Please replace with &&.

dantrushin added inline comments.Mar 9 2021, 2:55 AM
llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
925

I disagree. &= is much more concise. Unless its overhead is visible, I'd trade it for readability.
And I'm not sure RHS of &= will be evaluated when LHS is false.

6.5.16.2 Compound assignment
...
Semantics
3
A compound assignment of the form E1 op = E2 differs from the simple assignment
expression E1 = E1 op (E2) only in that the lvalue E1 is evaluated only once.

And for binary operators is can be written as

CanPrune = canPrune(0) && canPrune(1)

to get rid of that compound assignment at all :)

reames updated this revision to Diff 329366.Mar 9 2021, 9:26 AM

address review comments

(note: testing improvements still pending)

reames updated this revision to Diff 329518.Mar 9 2021, 8:13 PM

rebase over tests to exercise new functionality, and fix a bug found by inspection related to caching.

skatkov added inline comments.Mar 9 2021, 8:15 PM
llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
913

nit: not sure but consider moving lambda out of a loop.
The loop becomes small and easy to read.

reames updated this revision to Diff 329522.Mar 9 2021, 8:47 PM

A bit of code cleanup

The code looks ok to me but I wanted to do a bit more extensive local testing and it takes some time, so if you are ok, please wait with landing for a couple of days.

skatkov accepted this revision.Mar 14 2021, 9:10 PM

The code looks ok to me but I wanted to do a bit more extensive local testing and it takes some time, so if you are ok, please wait with landing for a couple of days.

No issues found in downstream testing so far - great job!
Thanks!

This revision is now accepted and ready to land.Mar 14 2021, 9:10 PM
This revision was landed with ongoing or failed builds.Mar 16 2021, 12:51 PM
This revision was automatically updated to reflect the committed changes.