Page MenuHomePhabricator

[Attributor] Pointer privatization attribute (argument promotion)
AcceptedPublic

Authored by jdoerfert on Oct 10 2019, 7:36 PM.

Details

Summary

A pointer is privatizeable if it can be replaced by a new, private one.
Privatizing pointer reduces the use count, interaction between unrelated
code parts. This is a first step towards replacing argument promotion.
While we can already handle recursion (unlike argument promotion!) we
are restricted to stack allocations for now because we do not analyze
the uses in the callee.

All argument promotion test now run the Attributor as well.

Diff Detail

Event Timeline

jdoerfert created this revision.Oct 10 2019, 7:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2019, 7:36 PM

I went through all the tests, added mem2reg/sroa where approriate and modified the source sometimes, mostly to avoid UB. I think the results of the Attributor look good, all problems should have been addressed already.

lebedev.ri added inline comments.Oct 16 2019, 11:59 AM
llvm/test/Transforms/ArgumentPromotion/control-flow2.ll
27–31 ↗(On Diff #224526)

This is why i'm really pushing on using --check-prefixes=ALL,ARGPROMOTION from the getgo :[

35 ↗(On Diff #224526)

I strongly believe you want to precommit test changes+regeneration first.

jdoerfert marked 2 inline comments as done.
jdoerfert added inline comments.
llvm/test/Transforms/ArgumentPromotion/control-flow2.ll
27–31 ↗(On Diff #224526)

Agreed.

35 ↗(On Diff #224526)

I will update the test lines after the D68766 update. Do you want me to split test changes, e.g., that remove UB, as well?

Update the tests

jdoerfert updated this revision to Diff 227335.Oct 31 2019, 1:41 PM

Update tests

I'll copy the tests into the new Attributor test folder. Any other comments? @uenoku @sstefan1

uenoku added a comment.EditedThu, Dec 12, 6:57 AM

Looks generally fine.
I couldn't imagine what is Pointer privatization at first hand. Could you add an example result of Pointer privatization? Like,

int f(int* ptr){
 ...
}
=>
int f(int p){
 int* ptr = &p;
 ...
}

And I guess the current implementation always does privatization if possible. I think the cost may increase in some cases, right? What do you think about?

llvm/include/llvm/Transforms/IPO/Attributor.h
2205

Could you add comments for the condition of whether the pointer can be replaced a private one?

2217

nit: choose

llvm/test/Transforms/ArgumentPromotion/chained.ll
18 ↗(On Diff #227335)

Please add FIXME here for AAValueSimplify.

Looks generally fine.
I couldn't imagine what is Pointer privatization at first hand. Could you add an example result of Pointer privatization? Like,

int f(int* ptr){
 ...
}
=>
int f(int p){
 int* ptr = &p;
 ...
}

I will add the example to the class comment in the header file.

For the record:
Privatization, at least the part implemented so far, is roughly argument promotion. Instead of passing a pointer, pass the values accessed through it by the callee.
The existing argument promotion does not do privatization but tries to replace the uses of the pointer with the values passed right away. Privatization is simpler in that
regard but later, partially because of this, also more powerful.

And I guess the current implementation always does privatization if possible. I think the cost may increase in some cases. What do you think about?

That is correct. So far, we build the Attribtor to be powerful (=applicable) not to be "smart" about costs. We'll have to write heurisitcs soon but before that I want to ask people to test the powerful version in order to get more coverage and sniff out bugs.

In fact, we might always do privatization once another piece of code I have only locally is available. With it, privatization might cause arbitrarily many arguments at the call site but we can always recover the original call site from it. More on that later though. For comparison: ArgumentPromotion restricts the size of the structs that are expanded arbitrarily to 3, which is beyond me.

uenoku accepted this revision.Fri, Dec 13, 3:46 AM

LGTM from my side but please make sure that it passes test-suite.

llvm/lib/Transforms/IPO/ArgumentPromotion.cpp
846

I think you can put this function to llvm namespace and split commit.

This revision is now accepted and ready to land.Fri, Dec 13, 3:46 AM

LGTM from my side but please make sure that it passes test-suite.

Will do.

I have to wait for D68765 first but not necessarily for the update scrip patches (they are blocked by the update test infrastructure patch).

jdoerfert marked 5 inline comments as done.

Addressed comments

jdoerfert added inline comments.Sat, Dec 14, 12:24 AM
llvm/include/llvm/Transforms/IPO/Attributor.h
2205

Done.

llvm/lib/Transforms/IPO/ArgumentPromotion.cpp
846

I would like to get rid of this, the Attributor should directly use a more specialized form of areFunctionArgsABICompatible. I'll add a TODO.