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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson 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");
----------------
kazu wrote:
> 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.
> 
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150396