hfinkel added a subscriber: chandlerc. hfinkel added a comment. In D61634#1502201 <https://reviews.llvm.org/D61634#1502201>, @tejohnson wrote:
> In D61634#1502138 <https://reviews.llvm.org/D61634#1502138>, @hfinkel wrote: > > > In D61634#1502043 <https://reviews.llvm.org/D61634#1502043>, @efriedma > > wrote: > > > > > > I have a related patch that turns -fno-builtin* options into module > > > > flags > > > > > > Do you have any opinion on representing -fno-builtin using a module flag > > > vs. a function attribute in IR? It seems generally more flexible and > > > easier to reason about a function attribute from my perspective. But I > > > might be missing something about the semantics of -fno-builtin that would > > > make that representation awkward. Or I guess it might just be more work > > > to implement, given we have some IPO passes that use TargetLibraryInfo? > > > > > > I think that a function attribute would be better. We generally use these > > flags only in the context of certain translation units, and when we use > > LTO, it would be sad if we had to take the most-conservative settings > > across the entire application. When we insert new function call to a > > standard library, we always do it in the context of some function. We'd > > probably need to block inlining in some cases, but that's better than a > > global conservative setting. > > > Using function level attributes instead of module flags does provide finer > grained control and avoids the conservativeness when merging IR for LTO. The > downsides I see, mostly just in terms of the engineering effort to get this > to work, are: > > - need to prevent inlining with different attributes I think that this should be relatively straightforward. You just need to update `AttributeFuncs::areInlineCompatible` and/or `AttributeFuncs::mergeAttributesForInlining` by adding a new MergeRule in include/llvm/IR/Attributes.td and writing a function like adjustCallerStackProbeSize. > - currently the TargetLibraryInfo is constructed on a per-module basis. > Presumably it would instead need to be created per Function - this one in > particular seems like it would require fairly extensive changes. Interesting point. The largest issue I see is that we need TLI available from loop passes, etc., and we can't automatically recompute a function-level analysis there. We need to make sure that it's always available and not invalidated. TLI is one of those analysis passes, being derived only from things which don't change (i.e., the target triple), or things that change very rarely (e.g., function attributes), that we probably don't want to require all passes to explicitly say that they preserve it (not that the mechanical change to all existing passes is hard, but it's easy to forget), so I think we'd like something like the CFG-only concept in the current passes, but stronger and perhaps turned on by default, for this kind of "attributes-only" pass. (@chandlerc , thoughts on this?). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61634/new/ https://reviews.llvm.org/D61634 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
