This is an archive of the discontinued LLVM Phabricator instance.

[ArgPromotion] Make implementation offset based

Authored by nikic on Feb 1 2022, 2:17 AM.


Group Reviewers
Restricted Project
rG68c1eeb4bad1: [ArgPromotion] Make implementation offset based

This rewrites ArgPromotion to be based on offsets rather than GEP structure. We inspect all loads at constant offsets and remember which types are loaded at which offsets. Then we promote based on those types.

This generalizes ArgPromotion to work with bitcasted loads, and is compatible with opaque pointers.

This patch also fixes incorrect handling of alignment during argument promotion. Previously, the implementation only checked that the pointer is dereferenceable, but was happy to speculate overaligned loads. (I would have fixed this separately in advance, but I found this hard to do with the previous implementation approach).

Diff Detail

Event Timeline

nikic created this revision.Feb 1 2022, 2:17 AM
nikic requested review of this revision.Feb 1 2022, 2:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2022, 2:17 AM
nikic added inline comments.Feb 1 2022, 2:19 AM

Worth highlighting that ArgPromotion has two separate promotion mechanisms, one that works only for byval, and one that is generally applicable. This case fails byval promotion, and previously failed general promotion. With the new implementation, it still fails byval promotion, but can now be generally promoted. Same for the case below.

ormris removed a subscriber: ormris.Feb 1 2022, 9:47 AM
nikic updated this revision to Diff 405232.Feb 2 2022, 5:49 AM
nikic edited the summary of this revision. (Show Details)

Check for sized type when constructing GEP.

aeubanks added inline comments.

would this be nicer if DataLayout::getIndexTypeSizeInBits() also handled struct types?


this should be separated out into its own change


the NeededDerefBytes = ... is ok to be in this if block because this will only have an effect when we see this offset for the first time because we only support the same type, meaning Size will always be the same for a given offset

this is slightly confusing, perhaps putting it in a separate !GuaranteedToExecute && Pair.second block would be clearer


I don't think we have a test for an unrelated load in the entry block

nikic updated this revision to Diff 406739.Feb 8 2022, 2:09 AM
nikic marked 2 inline comments as done.

Address review comments.


I don't think that would fit the API, as getIndexTypeSizeInBits() works on the pointer type, not the pointed-to/indexed type.


Right, I've landed this separately.


Right, the correctness of this logic depends on only supporting a single type (or at least, a single size). I've extended the comment on the block to explicitly say that.

this is slightly confusing, perhaps putting it in a separate !GuaranteedToExecute && Pair.second block would be clearer

Not sure I got this, do you mean splitting this into two blocks, one that updates NeededDerefBytes and one that updates NeededAlign?


Nice catch! It looks like this ultimately makes no functional difference, because we still inspect all loads in the loop below. So this code could actually call HandleLoad() without checking the return value, from a correctness perspective. Checking the return value just aborts the analysis a bit earlier.

I don't think we have a test for an unrelated load in the entry block

We have a test for this in basictest.ll at least -- generally, any test with multiple promoted arguments, because the loads for the other arguments will be "unrelated" to the current argument.

aeubanks accepted this revision.Feb 8 2022, 9:44 AM
This revision is now accepted and ready to land.Feb 8 2022, 9:44 AM
This revision was landed with ongoing or failed builds.Feb 9 2022, 12:35 AM
This revision was automatically updated to reflect the committed changes.