Page MenuHomePhabricator

[FuncSpec] Specialising on addresses of const global values
ClosedPublic

Authored by SjoerdMeijer on Tue, Sep 14, 11:32 AM.

Details

Summary

This introduces an option to allow specialising on the address of global values. This option is off by default because it is likely not that profitable to do so and needs more investigation. Before, we were specialising on addresses and thus this changes the default behaviour.

Diff Detail

Event Timeline

SjoerdMeijer created this revision.Tue, Sep 14, 11:32 AM
SjoerdMeijer requested review of this revision.Tue, Sep 14, 11:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Sep 14, 11:32 AM
nikic added a subscriber: nikic.Tue, Sep 14, 11:53 AM

Shouldn't this be specializing on the address of the global rather than its initializer?

Thanks for commenting!
I think we want to specialise on the constant value that is passed to the value. If we specialise on the address, then I think we could get wrong results as that global may be modified. Tomorrow morning I will write some more tests to convince myself this is true. In the mean time I have regenerated the test that shows the before/after a little bit better.

This looks good to me. Although we could specialize on the address, the benefit seems little. We couldn't remove the loads. We could only eliminate one argument. Although it is possible that the user computes the address as integer, in which case specialize on address could be beneficial, it seems really rare.

I have precommitted the test in rGa4e437e3c959.

Specialising on the address confused me, i.e. I had not yet consciously thought about allowing/disallowing it. But now having looked a bit more into this, I don't see the problem, and I don't see why we would disallow it with this patch. So think I will abandon this patch. Any thoughts?

I have precommitted the test in rGa4e437e3c959.

Specialising on the address confused me, i.e. I had not yet consciously thought about allowing/disallowing it. But now having looked a bit more into this, I don't see the problem, and I don't see why we would disallow it with this patch. So think I will abandon this patch. Any thoughts?

I prefer to disallowing specializing on the address of variable globals. Since it looks like not beneficial in most cases. And if we could be sure that it wouldn't be beneficial, I think we could disallow such cases.

nikic added a comment.Thu, Sep 16, 1:56 AM

I have precommitted the test in rGa4e437e3c959.

Specialising on the address confused me, i.e. I had not yet consciously thought about allowing/disallowing it. But now having looked a bit more into this, I don't see the problem, and I don't see why we would disallow it with this patch. So think I will abandon this patch. Any thoughts?

I prefer to disallowing specializing on the address of variable globals. Since it looks like not beneficial in most cases. And if we could be sure that it wouldn't be beneficial, I think we could disallow such cases.

Shouldn't that be a cost model decision though? I agree that it likely won't be beneficial, but it it's not like it can't be (say you have an icmp eq %arg, @g in the specialized function).

I have precommitted the test in rGa4e437e3c959.

Specialising on the address confused me, i.e. I had not yet consciously thought about allowing/disallowing it. But now having looked a bit more into this, I don't see the problem, and I don't see why we would disallow it with this patch. So think I will abandon this patch. Any thoughts?

I prefer to disallowing specializing on the address of variable globals. Since it looks like not beneficial in most cases. And if we could be sure that it wouldn't be beneficial, I think we could disallow such cases.

Shouldn't that be a cost model decision though? I agree that it likely won't be beneficial, but it it's not like it can't be (say you have an icmp eq %arg, @g in the specialized function).

Oh yeah, right, it would be absolutely better to filter the not beneficial cases in the cost model. I agree that it is a better direction.

Ok, good points. I will introduce an option to enable/disable specialising on addresses that is off by default, to make explicit and 'document' that this is possible (and give the opportunity to turn this on).

Added an option that is off by default to specialise on addresses. Investigating a cost model for this is for another day, I don't want to do this at this point.

This revision is now accepted and ready to land.Thu, Sep 16, 8:46 PM
SjoerdMeijer retitled this revision from [FuncSpec] Fix isConstant check for globals to [FuncSpec] Specialising on addresses of const global values.Fri, Sep 17, 12:17 AM
SjoerdMeijer edited the summary of this revision. (Show Details)

Thanks for reviewing!

This revision was landed with ongoing or failed builds.Fri, Sep 17, 12:19 AM
This revision was automatically updated to reflect the committed changes.