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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
