[PATCH] D108872: [IR] Refactor GlobalIFunc to inherit from GlobalObject, Remove GlobalIndirectSymbol

[Itay Bookstein via Phabricator via llvm-commits]

ibookstein added a subscriber: respindola.
ibookstein added a comment.

Regarding leaving GlobalIndirectSymbol as a utility class, it seems to me that its utility is in direct proportion to the number of places I ended up converting to use GlobalValue rather than monomorphizing (to the degree that you agree with my tactical decisions in these places, of course :) ).
The major ones were the BitcodeReader, LLParser, IRLinker, SplitModule, ValueMapper.

Regarding the direction gaining consensus in general, I'm wondering myself whether the benefits of this direction outweigh the costs, I mainly wanted a working patch so that the discussion will no longer be theoretical. 
Who would we want to weigh in / involve? I think the opinion of someone who understands the reasoning and design behind the GlobalValue <-> GlobalObject <-> GlobalAlias split will be valuable.
Git names @respindola as the author of that split.



================
Comment at: llvm/unittests/IR/ConstantsTest.cpp:388-389
 
-  auto *Global = new GlobalVariable(*M, PtrTy, false,
+  auto *Global = new GlobalVariable(*M, IntTy, false,
                                     GlobalValue::ExternalLinkage, nullptr);
   auto *Alias = GlobalAlias::create(IntTy, 0, GlobalValue::ExternalLinkage,
----------------
dexonsmith wrote:
> It's not obvious how this change is related to the patch... did it slip in accidentally?
It's not accidental, but it could definitely be committed separately. It's a bugfix for a type mismatch error that was exposed by the change to the IR class hierarchy, since only afterwards does the code-path of `GlobalAlias::create` pass through the type check assertion in `GlobalAlias::setAliasee`:

```
void GlobalAlias::setAliasee(Constant *Aliasee) {
  assert((!Aliasee || Aliasee->getType() == getType()) &&
         "Alias and aliasee types should match!");
  ...
}
```
Before the change, the `GlobalAlias::create` code-path sets the aliasee via the `GlobalIndirectSymbol::GlobalIndirectSymbol` constructor, which unceremoniously sets `Op<0>() = Symbol;` without performing the type check (which it can't, because for GlobalIFunc that check would be incorrect).


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

https://reviews.llvm.org/D108872