[PATCH] D15525: [GCC] Attribute ifunc support in llvm

[Dmitry Polukhin via llvm-commits]

On Mon, Jan 11, 2016 at 8:53 PM, David Blaikie via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> On Fri, Dec 18, 2015 at 6:39 AM, Joerg Sonnenberger via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> On Fri, Dec 18, 2015 at 09:17:17AM +0000, Dmitry Polukhin via
>> llvm-commits wrote:
>> > Just for the sake of completeness, there is a way to avoid IR extension
>> > - it is possible to emit global asm statement like
>> >    `__asm__ (".type resolver_alias_name, @gnu_indirect_function")`
>> > and avoid IR extension but I didn't seriously consider this path.
>>
>> Why not? Let me put it another way: what is the advantage of making the
>> intermediate layers aware of ifuncs? Can they provide better
>> optimisations? Can they diagnose any potential bugs?
>
>
> One possible optimization:
>
> say you have a target specific function that calls a generic function that
> in turn calls an ifunc function
> with ifunc as a first-class construct you can potentially inline both
> calls into the target specific outer function - inline the generic one, and
> in doing so, observe that the resolver function would produce a constant
> result (would have to teach the inliner about a cpuid intrinsic of some
> sort) & thus lower the call-to-ifunc with a direct call, then inline that
> call, etc.
>

Also I'm not very confident that global asm state will be enough to keep
alias in-place. It looks like it might be optimized away in some cases
because LLVM parses global asm statements very late and doesn't know their
content during optimizations. Final goal is to support attribute target
with possible inlining etc. proposed approach with explicit ifunc support
in IR is much better in long term.

On Tue, Jan 12, 2016 at 12:00 AM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

> > Index: lib/Transforms/IPO/GlobalDCE.cpp
> > ===================================================================
> > --- lib/Transforms/IPO/GlobalDCE.cpp
> > +++ lib/Transforms/IPO/GlobalDCE.cpp
> > @@ -205,9 +205,9 @@
> >      // referenced by the initializer to the alive set.
> >      if (GV->hasInitializer())
> >        MarkUsedGlobalsAsNeeded(GV->getInitializer());
> > -  } else if (GlobalAlias *GA = dyn_cast<GlobalAlias>(G)) {
> > -    // The target of a global alias is needed.
> > -    MarkUsedGlobalsAsNeeded(GA->getAliasee());
> > +  } else if (GlobalIndirectSymbol *GIS =
> dyn_cast<GlobalIndirectSymbol>(G)) {
> > +    // The target of a global alias or ifunc is needed.
> > +    MarkUsedGlobalsAsNeeded(GIS->getIndirectSymbol());
>
> It seems like this change could be done more incrementally.  For
> example, if you add `GlobalIndirectSymbol` in an initial commit, you can
> then convert all the appropriate things from `GlobalAlias` to
> `GlobalIndirectSymbol` one at a time, before finally landing the new
> `GlobalIFunc` commit which adds the new IR type.
>
> (I'm just picking on the first such change in the patch.)
>

I think it is good to have the whole path in single place to have complete
picture of proposed change. But I'm ready to split it in as many parts as
needed to speed up code review. My only worry that I will have to touch
some files more than once with this approach and it won't actually speed up
or simplify things. Please let me know if you really want me to split this
patch in multiple patches.

>    } else {
> >      // Otherwise this must be a function object.  We have to scan the
> body of
> >      // the function looking for constants and global values which are
> used as
> > Index: lib/IR/AsmWriter.cpp
> > ===================================================================
> > --- lib/IR/AsmWriter.cpp
> > +++ lib/IR/AsmWriter.cpp
> > @@ -249,11 +254,15 @@
> >      predictValueUseListOrder(&F, nullptr, OM, Stack);
> >    for (const GlobalAlias &A : M->aliases())
> >      predictValueUseListOrder(&A, nullptr, OM, Stack);
> > +  for (const GlobalIFunc &I : M->ifuncs())
> > +    predictValueUseListOrder(&I, nullptr, OM, Stack);
> >    for (const GlobalVariable &G : M->globals())
> >      if (G.hasInitializer())
> >        predictValueUseListOrder(G.getInitializer(), nullptr, OM, Stack);
> >    for (const GlobalAlias &A : M->aliases())
> >      predictValueUseListOrder(A.getAliasee(), nullptr, OM, Stack);
> > +  for (const GlobalIFunc &I : M->ifuncs())
> > +    predictValueUseListOrder(I.getResolver(), nullptr, OM, Stack);
>
> Can you add a lit test (if there isn't one already) that calls
> verify-uselistorder in a way that would confirm this "predict" call was
> added in the right place?
>
> I think it should be sufficient to a GIF that's referenced by each of
> GlobalVariable, GlobalAlias, GlobalIFunc, and Function; and a Function
> that's referenced in the same four ways.
>

I added such test, please check that it is what you were asking for.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160112/ec9190ac/attachment.html>