[PATCH] D150396: [InlineCost] Check for conflicting target attributes early

[Kazu Hirata via Phabricator via llvm-commits]

kazu added inline comments.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:2935
   // always-inline attribute).
-  Function *Caller = Call.getCaller();
-  if (!functionsHaveCompatibleAttributes(Caller, Callee, CalleeTTI, GetTLI))
+  if (!functionsHaveCompatibleAttributes(Caller, Callee, GetTLI))
     return InlineResult::failure("conflicting attributes");
----------------
tejohnson wrote:
> kazu wrote:
> > tejohnson wrote:
> > > Can we move this above the always inline handling and put the new checking in here?
> > 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.
> 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?
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.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150396/new/

https://reviews.llvm.org/D150396