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

[Dmitry Polukhin via llvm-commits]

Eric and Duncan than you a lot for the review!

On Wed, Apr 6, 2016 at 5:53 AM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

> > --- lib/IR/Module.cpp
> > +++ lib/IR/Module.cpp
> > @@ -250,6 +252,13 @@
> >    return dyn_cast_or_null<GlobalAlias>(getNamedValue(Name));
> >  }
> >
> > +// getNamedIFunc - Look up the specified global in the module symbol
> table.
> > +// If it does not exist, return null.
> > +//
>
> Please put this comment in the header instead of the source file, and
> don't duplicate the identifier ("getNamedIFunc").
>
> (If you're up to it, ideally you'd make the same transformation for the
> existing function comments in this file as a prep commit.)
>

The header already has similar comment so it is kind of duplication
therefore I just remove it. I'll update existing functions in separate
patch later, need careful comparison of the comments in .cpp and .h file.

> --- lib/IR/Globals.cpp
> > +++ lib/IR/Globals.cpp
> > @@ -145,6 +145,8 @@
> >        return const_cast<GlobalObject *>(GO)->getComdat();
> >      return nullptr;
> >    }
> > +  if (isa<GlobalIFunc>(this))
> > +    return nullptr;
>
> Should this try to look through the GlobalIFunc and return the same
> comdat?  (I honestly don't know; I don't know much about comdats.)
>
>   - If so, please add that (maybe change the above code to use
>     GlobalIndirectSymbol?).
>
>   - If not, please add a comment explaining why it's appropriate for
>     GlobalAlias and not GlobalIFunc.
>

Alias is just another name for the same thing. As for ifunc and its
resolver they are completely separate things. Added comment.

> --- lib/Bitcode/Writer/BitcodeWriter.cpp
> > +++ lib/Bitcode/Writer/BitcodeWriter.cpp
> > @@ -809,6 +809,19 @@
> >      Vals.clear();
> >    }
> >
> > +  // Emit the ifunc information.
> > +  for (const GlobalIFunc &I : M->ifuncs()) {
> > +    // IFUNC: [ifunc type, address space, resolver val#, linkage,
> visibility]
> > +    Vals.push_back(VE.getTypeID(I.getValueType()));
> > +    Vals.push_back(I.getType()->getAddressSpace());
> > +    Vals.push_back(VE.getValueID(I.getResolver()));
> > +    Vals.push_back(getEncodedLinkage(I));
> > +    Vals.push_back(getEncodedVisibility(I));
> > +    unsigned AbbrevToUse = 0;
> > +    Stream.EmitRecord(bitc::MODULE_CODE_IFUNC, Vals, AbbrevToUse);
>
> This code is equivalent to the last two lines and a little cleaner:
>

>     Stream.EmitRecord(bitc::MODULE_CODE_IFUNC, Vals);
>

Done.

> --- lib/Bitcode/Reader/BitcodeReader.cpp
> > +++ lib/Bitcode/Reader/BitcodeReader.cpp
> > @@ -3658,8 +3658,10 @@
> >      }
> >      // ALIAS: [alias type, addrspace, aliasee val#, linkage]
> >      // ALIAS: [alias type, addrspace, aliasee val#, linkage,
> visibility, dllstorageclass]
> > +    // IFUNC: [alias type, addrspace, aliasee val#, linkage,
> visibility, dllstorageclass]
> >      case bitc::MODULE_CODE_ALIAS:
> > -    case bitc::MODULE_CODE_ALIAS_OLD: {
> > +    case bitc::MODULE_CODE_ALIAS_OLD:
> > +    case bitc::MODULE_CODE_IFUNC: {
>
> Diff would be one line shorter if you put MODULE_CODE_IFUNC *before*
> MODULE_CODE_ALIAS ;).
>

Done.

> @@ -3687,7 +3689,8 @@
> >          NewGA = GlobalAlias::create(
> >            Ty, AddrSpace, getDecodedLinkage(Linkage), "", TheModule);
> >        else
> > -        llvm_unreachable("Not an alias!");
> > +        NewGA = GlobalIFunc::create(
> > +          Ty, AddrSpace, getDecodedLinkage(Linkage), "", nullptr,
> TheModule);
>
> Have you run this through clang-format?  I think clang-format would
> indent this an extra two spaces.  (Not that you have to abide by
> clang-format, just made me wonder...)
>

I re-run clang format on this particular code fragment and now it formatted
it completely different. Done.

> --- /dev/null
> > +++ include/llvm/IR/GlobalIFunc.h
> > @@ -0,0 +1,74 @@
> > +//===-------- llvm/GlobalIFunc.h - GlobalIFunc class ------------*- C++
> -*-===//
> > +//
> > +//                     The LLVM Compiler Infrastructure
> > +//
> > +// This file is distributed under the University of Illinois Open Source
> > +// License. See LICENSE.TXT for details.
> > +//
> >
> +//===----------------------------------------------------------------------===//
> > +///
> > +/// \brief
> > +/// This file contains the declaration of the GlobalIFunc class, which
> > +/// represents a single indirect function in the IR. Indirect function
> uses
> > +/// ELF symbol type extension to mark that the address of a declaration
> should
> > +/// be resolved at runtime by calling a resolver function.
> > +///
> >
> +//===----------------------------------------------------------------------===//
> > +
> > +#ifndef LLVM_IR_GLOBALIFUNC_H
> > +#define LLVM_IR_GLOBALIFUNC_H
> > +
> > +#include "llvm/ADT/Twine.h"
> > +#include "llvm/ADT/ilist_node.h"
> > +#include "llvm/IR/GlobalIndirectSymbol.h"
> > +
> > +namespace llvm {
> > +
> > +class Module;
> > +template <typename ValueSubClass> class SymbolTableListTraits;
> > +
>
> A comment for this class would be nice.
>

Done.


>
> > +class GlobalIFunc : public GlobalIndirectSymbol,
>
> final?
>

Done.


>
> > +                    public ilist_node<GlobalIFunc> {
> > +  friend class SymbolTableListTraits<GlobalIFunc>;
> > +  void operator=(const GlobalIFunc &) = delete;
> > +  GlobalIFunc(const GlobalIFunc &) = delete;
> > +
> > +  void setParent(Module *parent);
> > +
> > +  GlobalIFunc(Type *Ty, unsigned AddressSpace, LinkageTypes Linkage,
> > +              const Twine &Name, Constant *Resolver, Module *Parent);
> > +
> > +public:
> > +  /// If a parent module is specified, the ifunc is automatically
> inserted into
> > +  /// the end of the specified module's ifunc list.
> > +  static GlobalIFunc *create(Type *Ty, unsigned AddressSpace,
> > +                             LinkageTypes Linkage, const Twine &Name,
> > +                             Constant *Resolver, Module *Parent);
> > +
> > +  /// This method unlinks 'this' from the containing module, but does
> not
> > +  /// delete it.
> > +  void removeFromParent() override;
>
> final?
>

Done.

> +
> > +  /// This method unlinks 'this' from the containing module and deletes
> it.
> > +  void eraseFromParent() override;
>
> final?
>

Done.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160406/83191895/attachment.html>