[PATCH] D61634: [clang/llvm] Allow efficient implementation of libc's memory functions in C/C++

[Hal Finkel via Phabricator via llvm-commits]

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