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

[Itay Bookstein via Phabricator via llvm-commits]

ibookstein added inline comments.


================
Comment at: llvm/lib/AsmParser/LLParser.cpp:1067
-  // The module owns this now
-  GA.release();
 
----------------
MaskRay wrote:
> Is this unneeded now?
Note that `GV` is now a borrowed reference from either `GA` or `GI`, which are both `std::unique_ptr`-s. Their ownership is transferred into the `Module` in the `release()` calls in the 5 lines immediately above. 


================
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,
----------------
ibookstein wrote:
> 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).
@MaskRay this can also be pushed separately if you'd like


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

https://reviews.llvm.org/D108872