This is an archive of the discontinued LLVM Phabricator instance.

[InlineCost] Check for conflicting target attributes early
ClosedPublic

Authored by kazu on May 11 2023, 12:57 PM.

Details

Summary

When we inline a callee into a caller, the compiler needs to make sure
that the caller supports a superset of instruction sets that the
callee is allowed to use. Normally, we check for the compatibility of
target features via functionsHaveCompatibleAttributes, but that
happens after we decide to honor call site attribute
Attribute::AlwaysInline. If the caller contains a call marked with
Attribute::AlwaysInline, which can happen with
attribute((flatten)) placed on the caller, the caller could end up
with code that cannot be lowered to assembly code.

This patch fixes the problem by checking the target feature
compatibility before we honor Attribute::AlwaysInline.

Fixes https://github.com/llvm/llvm-project/issues/62664

Diff Detail

Event Timeline

kazu created this revision.May 11 2023, 12:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2023, 12:57 PM
kazu requested review of this revision.May 11 2023, 12:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2023, 12:57 PM
tejohnson added inline comments.May 11 2023, 1:15 PM
llvm/lib/Analysis/InlineCost.cpp
2917

Probably this should be a missed optimization remark?

2935

Can we move this above the always inline handling and put the new checking in here?

kazu added inline comments.May 11 2023, 1:41 PM
llvm/lib/Analysis/InlineCost.cpp
2917

Oops. I didn't meant to publish the fprintf, but yes, I'll turn it into a missed optimization remark.

2935

Could you elaborate this comment a little bit? I am not sure what you mean by "put the new checking in here".

This patch essentially peels the CalleeTTI.areInlineCompatible check off of functionsHaveCompatibleAttributes. If we are moving the rest of functionsHaveCompatibleAttributes above the always inline handling, then we might just call the original unmodified functionsHaveCompatibleAttributes above the always inline handling.

tejohnson added inline comments.May 11 2023, 1:49 PM
llvm/lib/Analysis/InlineCost.cpp
2935

Oh I see that it is being removed from functionsHaveCompatibleAttributes. It seems better to simply move the functionsHaveCompatibleAttributes checking above the always inline handling. I don't know the history here though - were you able to figure out why always inline functions skip this checking?

kazu added inline comments.May 11 2023, 3:42 PM
llvm/lib/Analysis/InlineCost.cpp
2935

If I simply moved functionsHaveCompatibleAttributes above the always inline handling (independently of this patch), then I would get:

Failed Tests (3):
  LLVM :: Transforms/Inline/attributes.ll
  LLVM :: Transforms/Inline/inline_noprofile.ll
  LLVM :: Transforms/SampleProfile/remarks.ll

So, people (or at least these tests) expect a very specific behavior.

If I just move the CalleeTTI.areInlineCompatible instead (as in this patch), then I don't get any test failure.

Since we do not merge target features upon inlining, we should reject an inlining opportunity if the caller is not allowed to use a superset of the instruction sets that the callee is allowed to use.

Here is a little bit of history:

In 2013, commit 2ad3698b70745826f73e7270fc173e056e879619 added the comment:

// Never inline functions with conflicting attributes (unless callee has                                                                                                   
// always-inline attribute).

along with checks for the compatibility of Attribute::SanitizeAddress, Attribute::SanitizeMemory, and Attribute::SanitizeThread. These are a precursor to what we call AttributeFuncs::areInlineCompatible today. Back then, we didn't have checks for target features yet.

In 2015, commit f99e1913ae449e7d7f95fc918f44a2a1116e03d4 added checks to see if the target-cpu and target-features attributes exactly match between the callee and the caller, respectively.

Later in 2015, commit e1002268798ecedadaa9cdf6cc52b26e446ef3a7 relaxed the rule on the target features. The caller just has to have a superset of instructions that the callee is allowed to use.

I haven't checked every single patch in this area, but the ones I looked at do not have anything to do with the intersection of Attribute::AlwaysInline and target features.

kazu updated this revision to Diff 521527.May 11 2023, 6:52 PM

Added a check for a remark.

kazu added a comment.May 11 2023, 6:54 PM

I've added a CHECK for a missed optimization remark. Please take a look. Thanks!

llvm/lib/Analysis/InlineCost.cpp
2917

It turns out that the failure reason is already emitted as a missed optimization remark up in the call hierarchy, so we don't need to emit one on our own here.

kazu added a comment.May 23 2023, 2:20 PM

Friendly ping. I now emit a remark:

return InlineResult::failure("conflicting target attributes");

so users should be able to tell where they are missing potential optimization opportunities due to mismatching target attributes.

tejohnson added inline comments.May 23 2023, 3:00 PM
llvm/lib/Analysis/InlineCost.cpp
2935

Thanks for the history. Can none of the other attributes now checked by functionsHaveCompatibleAttributes (after removing the target attribute checks) cause failures if we ignore then due to the always inline attribute? I suppose the original sanitizer attributes aren't really correctness as they just control what type of sanitization is applied. But I wonder about the others, e.g. TLI. Also, AttributeFuncs::areInlineCompatible now also checks things like the denorm mode - that also seems problematic to ignore.

kazu updated this revision to Diff 527106.May 31 2023, 10:16 AM

Add a FIXME comment.

kazu added a comment.May 31 2023, 10:18 AM

I've added a FIXME comment about other attributes that the always-inline attribute takes precedence over. Please take a look. Thanks!

This revision is now accepted and ready to land.Jun 2 2023, 10:56 AM
This revision was automatically updated to reflect the committed changes.