This is an archive of the discontinued LLVM Phabricator instance.

[Attributor] Function signature rewrite infrastructure
ClosedPublic

Authored by jdoerfert on Oct 10 2019, 12:02 AM.

Details

Summary

As part of the Attributor manifest we want to change the signature of
functions. This patch introduces a fairly generic interface to do so.
As a first, very simple, use case, we remove unused arguments. A second
use case, pointer privatization, will be committed with this patch as
well.

A lot of the code and ideas are taken from argument promotion.

Event Timeline

jdoerfert created this revision.Oct 10 2019, 12:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2019, 12:02 AM
jdoerfert updated this revision to Diff 224516.Oct 10 2019, 6:07 PM

Lessons learned from running *all* argument promotion tests, patches will follow

Reuse some argument promotion tests with dead arguments

jdoerfert updated this revision to Diff 227330.Oct 31 2019, 1:28 PM

Update tests

jdoerfert updated this revision to Diff 227332.Oct 31 2019, 1:33 PM

Remove spurios changes to tests

uenoku added a comment.EditedNov 7 2019, 11:08 PM

Sorry for the delay. Basically it looks good to me regarding the logic.
I think you need to separate diff for argument promotion test changes or wait for reaching on an agreement in D69748.

llvm/lib/Transforms/IPO/Attributor.cpp
4643–4662

Replaced/Replacement is a bit confusing. Could you change to Old/New or more appropriate words?

jdoerfert marked an inline comment as done.Nov 8 2019, 12:14 PM

Sorry for the delay. Basically it looks good to me regarding the logic.

Let me know if there is anything else. I would update this for the final version based on the smaller changes you requested.

I think you need to separate diff for argument promotion test changes or wait for reaching on an agreement in D69748.

I'll copy the tests over into a new Attributor test folder.

llvm/lib/Transforms/IPO/Attributor.cpp
4643–4662

Will do.

jdoerfert marked an inline comment as done.

Rebase, addressed comment

uenoku added inline comments.Dec 21 2019, 8:28 AM
llvm/test/Transforms/Attributor/ArgumentPromotion/reserve-tbaa.ll
1 ↗(On Diff #233920)

Could you explain what occurs in this test?

jdoerfert marked an inline comment as done.Dec 21 2019, 12:08 PM
jdoerfert added inline comments.
llvm/test/Transforms/Attributor/ArgumentPromotion/reserve-tbaa.ll
1 ↗(On Diff #233920)

So the intent was to make sure we keep tbaa metadata properly attached to the new loads and stores.
I changed it because it was not using one of the arguments (transitively). As a result, we would
deleted the argument with this patch but that is not what we want to test. So I added a transitive user to ensure tbaa metadata are still tested (once we actually promote the arguments).

uenoku accepted this revision.Dec 22 2019, 6:09 AM

LGTM

llvm/lib/Transforms/IPO/Attributor.cpp
4741

nit: Rewrite

llvm/test/Transforms/Attributor/ArgumentPromotion/reserve-tbaa.ll
1 ↗(On Diff #233920)

It makes sense.

This revision is now accepted and ready to land.Dec 22 2019, 6:09 AM
This revision was automatically updated to reflect the committed changes.