[patch][PR10367] Fix the design of GlobalAlias

[Duncan P. N. Exon Smith]

I'm just responding to the first two patches for now.

On 2014-May-12, at 21:52, Rafael Espíndola <rafael.espindola at gmail.com> wrote:

> * llvm-1.patch: Split GlobalValue into GlobalValue and GlobalObject.
> That allows us statically accept a Function or a GlobalVariable, but
> not an alias. This is already a cleanup by itself IMHO,

I agree.

> but the main
> reason is that it gives a lot more confidence that the next patch is
> doing the right thing.
> 
> * clang-1.patch is just an update for the api change.
> 
> These two patches are already fairly mature.

I noticed some memory bloat in llvm-1.patch (details below).

clang-1.patch LGTM.

> diff --git a/include/llvm/IR/GlobalObject.h b/include/llvm/IR/GlobalObject.h
> new file mode 100644
> index 0000000..26a9ce8
> --- /dev/null
> +++ b/include/llvm/IR/GlobalObject.h
> @@ -0,0 +1,55 @@
> +//===-- llvm/GlobalObject.h - Class to represent a global object *- C++ -*-===//
> +//
> +//                     The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENSE.TXT for details.
> +//
> +//===----------------------------------------------------------------------===//
> +//
> +// This represents an independent object. That is, a function or a global
> +// variable, but not an alias.
> +//
> +//===----------------------------------------------------------------------===//
> +
> +#ifndef LLVM_IR_GLOBALOBJECT_H
> +#define LLVM_IR_GLOBALOBJECT_H
> +
> +#include "llvm/IR/Constant.h"
> +#include "llvm/IR/DerivedTypes.h"
> +#include "llvm/IR/GlobalValue.h"
> +
> +namespace llvm {
> +
> +class Module;
> +
> +class GlobalObject : public GlobalValue {
> +  GlobalObject(const GlobalObject &) LLVM_DELETED_FUNCTION;
> +
> +protected:
> +  GlobalObject(Type *ty, ValueTy vty, Use *Ops, unsigned NumOps,
> +               LinkageTypes linkage, const Twine &Name)

Style nit:  Ty, VTy, and Linkage.  I know you copied this from
`GlobalValue::GlobalValue`; maybe update that style too in a
separate commit?

> +      : GlobalValue(ty, vty, Ops, NumOps, linkage, Name), Alignment(0) {}
> +
> +  unsigned Alignment : 16; // Alignment of this symbol, must be power of two

`Alignment` will effectively use 64-bits of memory here (on x86-64).

> +  std::string Section;     // Section to emit this into, empty means default
> +public:
> +  unsigned getAlignment() const { return (1u << Alignment) >> 1; }
> +  void setAlignment(unsigned Align);
> +
> +  bool hasSection() const { return !getSection().empty(); }
> +  const std::string &getSection() const { return Section; }
> +  void setSection(StringRef S);
> +
> +  void copyAttributesFrom(const GlobalValue *Src) override;
> +
> +  // Methods for support type inquiry through isa, cast, and dyn_cast:
> +  static inline bool classof(const Value *V) {
> +    return V->getValueID() == Value::FunctionVal ||
> +           V->getValueID() == Value::GlobalVariableVal;
> +  }
> +};
> +
> +} // End llvm namespace
> +
> +#endif
> diff --git a/include/llvm/IR/GlobalValue.h b/include/llvm/IR/GlobalValue.h
> index 60685ca..ca5b35d 100644
> --- a/include/llvm/IR/GlobalValue.h
> +++ b/include/llvm/IR/GlobalValue.h
> @@ -62,7 +62,7 @@ protected:
>    GlobalValue(Type *ty, ValueTy vty, Use *Ops, unsigned NumOps,
>                LinkageTypes linkage, const Twine &Name)
>        : Constant(ty, vty, Ops, NumOps), Linkage(linkage),
> -        Visibility(DefaultVisibility), Alignment(0), UnnamedAddr(0),
> +        Visibility(DefaultVisibility), UnnamedAddr(0),
>          DllStorageClass(DefaultStorageClass), Parent(nullptr) {
>      setName(Name);
>    }
> @@ -71,18 +71,15 @@ protected:
>    // Linkage and Visibility from turning into negative values.
>    LinkageTypes Linkage : 5;   // The linkage of this global
>    unsigned Visibility : 2;    // The visibility style of this global
> -  unsigned Alignment : 16;    // Alignment of this symbol, must be power of two

It was nice that `Alignment` was packed in here before.

I wonder if it's worth the complexity of storing `Alignment` in
`GlobalValue`, even though it's only valid for `GlobalObject`?  Maybe
using the `Value::SubclassData` model?  I'm not sure how much this
bloat matters (1M globals => 8 MB).

>    unsigned UnnamedAddr : 1;   // This value's address is not significant
>    unsigned DllStorageClass : 2; // DLL storage class
>    Module *Parent;             // The containing module.
> -  std::string Section;        // Section to emit this into, empty mean default
>  public:
>    ~GlobalValue() {
>      removeDeadConstantUsers();   // remove any dead constants using this.
>    }
>  
>    unsigned getAlignment() const;
> -  void setAlignment(unsigned Align);
>  
>    bool hasUnnamedAddr() const { return UnnamedAddr; }
>    void setUnnamedAddr(bool Val) { UnnamedAddr = Val; }
> @@ -112,7 +109,6 @@ public:
>  
>    bool hasSection() const { return !getSection().empty(); }
>    const std::string &getSection() const;
> -  void setSection(StringRef S);
>  
>    /// Global values are always pointers.
>    inline PointerType *getType() const {