This is an archive of the discontinued LLVM Phabricator instance.

[InlineFunction] Update metadata on loads that are return values
ClosedPublic

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

Details

Summary

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
llvm/lib/Transforms/Utils/InlineFunction.cpp
1168

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

1196

Num

llvm/test/Transforms/Inline/ret_load_metadata.ll
69

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.
llvm/lib/Transforms/Utils/InlineFunction.cpp
1168

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

llvm/test/Transforms/Inline/ret_load_metadata.ll
69

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.

llvm/lib/Transforms/Utils/InlineFunction.cpp
1189

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

1234–1253

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

1244

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.

llvm/lib/Transforms/Utils/InlineFunction.cpp
1247

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: https://bugs.llvm.org/show_bug.cgi?id=45590

Please take a look.

anna added a comment.Apr 17 2020, 3:02 PM
In D76792#1989623, @yamauchi wrote:

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

Here's a bug report with a reduced test: https://bugs.llvm.org/show_bug.cgi?id=45590

Please take a look.

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