Page MenuHomePhabricator

[GC][NFC] Make getGCStrategy by name available in IR
AcceptedPublic

Authored by mkazantsev on Apr 15 2021, 6:52 AM.

Details

Summary

We might want to use info from GC strategy in middle end analysis.
The motivation for this is provided in D99135: we may want to ask
a GC if it's going to work with a given pointer (currently this code
makes naive check by the method name).

Diff Detail

Event Timeline

mkazantsev created this revision.Apr 15 2021, 6:52 AM
mkazantsev requested review of this revision.Apr 15 2021, 6:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2021, 6:52 AM
mkazantsev retitled this revision from [GC][NFC] Make getGCStrategy by name available in Analysis to [GC][NFC] Make getGCStrategy by name available in IR.
reames requested changes to this revision.Apr 16 2021, 11:22 AM

(Not reviewing until issue raised on previous patch addressed, please request review when that one is resolved.)

This revision now requires changes to proceed.Apr 16 2021, 11:22 AM
mkazantsev abandoned this revision.Apr 19 2021, 8:32 AM
mkazantsev reclaimed this revision.Apr 30 2021, 2:02 AM
This revision now requires changes to proceed.Apr 30 2021, 2:02 AM
mkazantsev planned changes to this revision.Apr 30 2021, 2:02 AM

I didn't receive any complains about linkage problems in previous patch, so re-requesting review.

I also tried dynamic linkage build on this one and it worked for me.

I don't think the interface on this is quite right. You have the new utility exposing a new instance of the GCStrategy object. I would expect each GCStrategy to be a singleton. (i.e. having two "statepoint-example" GCStrategy instances would seem surprising.)

I think you need to move the ownership of GCStrategy objects from GCMetadata to LLVMContext, and then cache the lookup. You want to leave GCStrategyList in place, but probably make it non-owning.

What's the problem of having multiple instances of the same GC strategy? Currently, each strategy has only const methods, so it doesn't matter which instance we are using. Even if we make them mutable, isn't it possible that we are willing to get some default strategy and then configure it, e.g., depending on a platform? In that case, having different instances is even useful.

What's the problem of having multiple instances of the same GC strategy? Currently, each strategy has only const methods, so it doesn't matter which instance we are using. Even if we make them mutable, isn't it possible that we are willing to get some default strategy and then configure it, e.g., depending on a platform? In that case, having different instances is even useful.

These are currently singleton objects; I'd like to preserve that. It would be reasonable for a custom strategy to e.g. cache computation in mutable state, and having two or more copies breaks that reasonable implementation in hard to predict ways.

If you think this is too large an ask implementation wise, say so. I'm not strongly tied to it, it's simply a mild preference.

mkazantsev added a comment.EditedMay 18 2021, 10:05 PM

Why are they singleton objects now? In current code, they are instatiated as many times as getGCStrategy is called, and it's called once for every function in module from GCFunctionInfo &GCModuleInfo::getFunctionInfo(const Function &F) (if I'm reading this correctly). Am I missing something here?

Why are they singleton objects now? In current code, they are instatiated as many times as getGCStrategy is called, and it's called once for every function in module from GCFunctionInfo &GCModuleInfo::getFunctionInfo(const Function &F) (if I'm reading this correctly). Am I missing something here?

You are missing something.

GCModuleInfo::getGCStrategy maintains a map from name to GCStrategy object. If you look up the same name repeatedly (e.g. for every function), you get the same GCStrategy object. There is currently one object per GC name.

mkazantsev added a comment.EditedWed, Jun 9, 1:32 AM

So it's not a singleton, it's something that is cached inside of GCModuleInfo. If you have 2 different instances of GCModuleInfo, you will have two different GCStrategies with same name. And this patch preserves this behavior.

You can as well say that SCEV constant 1 of type i32 is a singleton, but it's not: it is something that is cached within a particular instance of ScalarEvolution.

Even if this thing was a singleton, I don't believe it's important for any practical purpose.

reames accepted this revision.Fri, Jun 11, 12:53 PM

Max, I'd still prefer the singleton, but frankly, I don't care enough to keep discussing this. Land what we've got; we can refactor at some point in the future.

This revision is now accepted and ready to land.Fri, Jun 11, 12:53 PM