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

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

Sorry I've been a bit slow to respond here...

In D61634#1503089 <https://reviews.llvm.org/D61634#1503089>, @hfinkel wrote:

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


FWIW, I definitely agree here. This really is the end state we're going to find ourselves in and we should probably go directly there.

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

+1

> 
> 
>> - 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?).

Yep, this makes sense.

The new PM makes this quite easy. The analysis itself gets to implement the invalidation hook, and say "nope, I'm not invalidated". In fact, in the new PM, `TargetLibraryInfo` already works this way. We build an instance per function and say it is never invalidated.

However, they are just trivial wrappers around shared implementations, so it will still require some non-trivial changes. Will need to remove the module-based access and move clients over to provide a function when they query it, etc.

IIRC, `TargetTransformInfo` already basically works exactly this way in both old and new PMs and we should be able to look at exactly the techniques it uses in both pass managers to build an effective way to manage these per-function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61634