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

[Duncan P. N. Exon Smith via llvm-commits]

I have a bunch of nitpicks inline below; once you resolve those, LGTM!

> On 2016-Apr-05, at 15:59, Eric Christopher <echristo at gmail.com> wrote:
> 
> echristo accepted this revision.
> echristo added a comment.
> This revision is now accepted and ready to land.
> 
> LGTM. Since Duncan has been looking at these as well you might want to wait for an ACK from him as well.
> 
> Thanks a ton for doing all of this. :)
> 
> -eric
> 
> 
> http://reviews.llvm.org/D15525

> Index: lib/IR/Module.cpp
> ===================================================================
> --- 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.)

> +GlobalIFunc *Module::getNamedIFunc(StringRef Name) const {
> +  return dyn_cast_or_null<GlobalIFunc>(getNamedValue(Name));
> +}
> +
>  /// getNamedMetadata - Return the first NamedMDNode in the module with the
>  /// specified name. This method returns null if a NamedMDNode with the
>  /// specified name is not found.
> Index: lib/IR/Globals.cpp
> ===================================================================
> --- 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.

>    return cast<GlobalObject>(this)->getComdat();
>  }
>  
> Index: lib/Bitcode/Writer/BitcodeWriter.cpp
> ===================================================================
> --- 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);

> +    Vals.clear();
> +  }
> +
>    // Emit the module's source file name.
>    {
>      StringEncoding Bits = getStringEncoding(M->getSourceFileName().data(),
> Index: lib/Bitcode/Reader/BitcodeReader.cpp
> ===================================================================
> --- 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 ;).

>        bool NewRecord = BitCode != bitc::MODULE_CODE_ALIAS_OLD;
>        if (Record.size() < (3 + (unsigned)NewRecord))
>          return error("Invalid record");
> @@ -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...)

>        // Old bitcode files didn't have visibility field.
>        // Local linkage must have default visibility.
>        if (OpNum != Record.size()) {
> Index: include/llvm/IR/GlobalIFunc.h
> ===================================================================
> --- /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.

> +class GlobalIFunc : public GlobalIndirectSymbol,

final?

> +                    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?

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

final?

> +
> +  /// These methods retrieve and set ifunc resolver function.
> +  void setResolver(Constant *Resolver) {
> +    setIndirectSymbol(Resolver);
> +  }
> +  const Constant *getResolver() const {
> +    return getIndirectSymbol();
> +  }
> +  Constant *getResolver() {
> +    return getIndirectSymbol();
> +  }
> +
> +  // Methods for support type inquiry through isa, cast, and dyn_cast:
> +  static inline bool classof(const Value *V) {
> +    return V->getValueID() == Value::GlobalIFuncVal;
> +  }
> +};
> +
> +} // End llvm namespace
> +
> +#endif