[PATCH] D104342: [IR] convert warn-stack-size from module attr to fn attr

Fri Jun 18 15:01:58 PDT 2021

nickdesaulniers added a comment.

In D104342#2827847 <https://reviews.llvm.org/D104342#2827847>, @dblaikie wrote:

> Another thing you might want to check is linkonce_odr functions - I guess you'll get an arbitrary choice between two linkonce_odr functions under LTO where they have different warn-stack-size? Maybe there's a way/place to merge and always pick the lower or upper value if there's one you think would be more right?

I've added an `llvm-link` test for this. I'm not sure it adds any signal though here; I think the answer to such a question is "don't do that."

================
Comment at: llvm/test/Transforms/Inline/warn-stack-size.ll:1
+; RUN: opt -passes=inline -S %s | FileCheck %s
+
----------------
dblaikie wrote:
> Nice to see the test - though I probably wouldn't bother adding this test if this behavior already falls out of more general support in the inliner and the way it already handles attributes - the general behavior is likely already tested elsewhere? (though it'd be good to confirm that either in tests and/or the inliner code itself)
> 
> my original question was to confirm that the inliner already had accounted for this situation in a way that was desirable & it looks like/sounds like it is.
`AttributeFuncs::areInlineCompatible` seems to define the disallow-list for mismatched function attributes.

`AttributeFuncs::mergeAttributesForInlining()` seems to be the merging strategy for certain function attributes.

I agree that this test just confirms that the implicit default merge strategy is used.  I guess it would fail if someone unintentionally changed that, but I don't mind removing this test either.  WDYT?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104342