Page MenuHomePhabricator

[CallSite removal] Migrate the statepoint GC infrastructure to use the `CallBase` class rather than `CallSite` wrappers.
ClosedPublic

Authored by chandlerc on Dec 28 2018, 1:28 AM.

Details

Summary

This is the largest remaining direct user of CallSite in the IR
headers.

I pushed this change down through most of the statepoint infrastructure,
completely removing the use of CallSite where I could reasonably do so.
I ended up making a couple of cut-points: generic call handling
(instcombine, TLI, SDAG). As soon as it hit truly generic handling with
users outside the immediate code, I simply transitioned into or out of
a CallSite to make this a reasonable sized chunk.

The next big chunk will be cleaning out the Analysis APIs, however it
may be unreasonable to make easy cut points there and so I may jointly
migrate all of Analysis and Transforms due to their tightly coupled
nature.

The last big chunk will be migrating codegen and SDAG.

Amidst these, of course, will be migrating implementation-only code in
various libraries when convenient. However, that is likely to be less
disruptive.

Diff Detail

Repository
rL LLVM

Event Timeline

chandlerc created this revision.Dec 28 2018, 1:28 AM
chandlerc updated this revision to Diff 180433.Jan 6 2019, 9:10 PM

Rebase and ping

reames accepted this revision.Jan 16 2019, 3:53 PM

LGTM w/one requested change before submit. If you strongly disagree, feel free to land and discuss afterwards. I don't want this blocked on me finding time to re-review.

Minor (can be easily cleaned up later): Using Call seems a bit confusing as a variable name. I'd have preferred CB for consistency with other naming.

I see a bunch of follow on simplifications, but I can do a wave of small commits once this lands and bakes for a bit.

llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
2379 ↗(On Diff #180433)

This looks like a spurious unrelated change. I also don't see why passing by pointer is better than reference here. Can you remove this from the diff please?

This revision is now accepted and ready to land.Jan 16 2019, 3:53 PM
chandlerc marked an inline comment as done.Sun, Feb 10, 11:41 PM

LGTM w/one requested change before submit. If you strongly disagree, feel free to land and discuss afterwards. I don't want this blocked on me finding time to re-review.

Thanks, made the change and landing.

Minor (can be easily cleaned up later): Using Call seems a bit confusing as a variable name. I'd have preferred CB for consistency with other naming.

I prefer words over initialisms where there are reasonable words to use. That said, if you want to use CB here, :: shrug ::...

I see a bunch of follow on simplifications, but I can do a wave of small commits once this lands and bakes for a bit.

Yeah, happy for you to do subsequent simplifications.

llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
2379 ↗(On Diff #180433)

Yeah, no idea how this got into the diff in the first place. Removed.

Herald added a project: Restricted Project. · View Herald TranscriptSun, Feb 10, 11:41 PM
This revision was automatically updated to reflect the committed changes.