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

Fri Jun 18 16:52:08 PDT 2021

dblaikie added a comment.

In D104342#2828193 <https://reviews.llvm.org/D104342#2828193>, @nickdesaulniers wrote:

> 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."

I don't think it's as easy as "don't do that". Unless someone passes exactly the same flags to every compilation (which they won't, that's why this is being implemented as a function attribute) then it'll be really easy for an inline function in a header (say, `std::vector<int>::size` - something easy for two unrelated translation units to use) in two different translation units each with a different warn-stack-size flag and so to get somewhat arbitrary behavior about how that function is warned on.

For instance: maybe the function doesn't get a warning because a copy with a higher warn-stack-size value is picked, until the translation unit using that higher value is refactored and starts using std::list instead of std::vector... and now some other TU's std::vector is picked, with a lower warn-stack-size value and breaks the build (assuming -Werror)...

================
Comment at: llvm/test/Transforms/Inline/warn-stack-size.ll:1
+; RUN: opt -passes=inline -S %s | FileCheck %s
+
----------------
nickdesaulniers wrote:
> 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?
generally I wouldn't add a test like this, or the LTO one - I'd just confirm that their features are separately tested and the behavior that's desired for how I want to use the feature (eg: I wouldn't test that the add instruction lowers to some machine code in my optimization - I'd confirm the add instruction has the desired semantics for whatever transformation I want to perform)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104342