Page MenuHomePhabricator

[InlineFunction] Update metadata on loads that are return values

Authored by anna on Mar 25 2020, 11:30 AM.



This patch builds upon D76140 by updating metadata on pointer typed loads in inlined functions, when the load is the return value, and the callsite contains return attributes which can be updated as metadata on the load.
Added test cases show this for nonnull, dereferenceable, dereferenceable_or_null

Diff Detail

Event Timeline

anna created this revision.Mar 25 2020, 11:30 AM
jdoerfert added inline comments.Mar 25 2020, 10:34 PM

Isn't this just VMap.lookup(V)?




Is there a test where the attribute is part of the callee and not the calls site?

anna marked 2 inline comments as done.Mar 26 2020, 6:15 AM
anna added inline comments.

you're right. We don't need a lambda for this.


Will add one.

anna updated this revision to Diff 252969.Mar 26 2020, 1:39 PM

addressed review comments

jdoerfert accepted this revision.Mar 28 2020, 12:14 AM

Minor comments, otherwise LGTM.


Nit: Cheap condition first: UpdateXXX && isa<...>


if (auto *CB = dyn_cast<CallBase>(...))
should be simpler.


Nit: I really hope that'll change, for non-null at least. Though, then we might also want to allow it in the metadata so all will be fine except the comment ;)

This revision is now accepted and ready to land.Mar 28 2020, 12:14 AM
anna marked an inline comment as done.Apr 2 2020, 11:21 AM

thanks for the review @jdoerfert . Will incorporate these changes and rebase on landed D76140.


We'll need to avoid updating dereferenceable, dereferenceable_or_null when these metadata is already present on the load. This will avoid updating the metadata with a different value.

In effect, we will have the same behaviour as Attribute::merge, where we avoid changing the attribute value when the call already has the same attribute with a value (see test6 in D76140). Will add analogous test case here.

This revision was automatically updated to reflect the committed changes.
nikic added a subscriber: nikic.Apr 6 2020, 12:55 AM

For the record, this causes a 0.6-0.8% compile-time regression on the tramp3d-v4 benchmark (there is also a 0.03% code-size improvement, so at least something is happening there codegen-wise). Can't say whether this is because adding the metadata itself has some cost, or because the presence of the metadata makes further optimization more expensive.

Hi, some of our internal tests seems to have started crashing due to this patch.

Here's a bug report with a reduced test:

Please take a look.

anna added a comment.Apr 17 2020, 3:02 PM

Hi, some of our internal tests seems to have started crashing due to this patch.

Here's a bug report with a reduced test:

Please take a look.

thanks for the test case, I've reverted the patch for now. Will take a look.