[PATCH] D54507: [ThinLTO] Handle chains of aliases

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson marked 4 inline comments as done.
tejohnson added a comment.

New version which more closely mirrors logic proposed in D29781 <https://reviews.llvm.org/D29781> comments. One difference is that I update all aliases in the chain in one recursive pass. In any case, this should make it easy to implement D29781 <https://reviews.llvm.org/D29781> on top as a follow on.



================
Comment at: lib/Transforms/Utils/CanonicalizeAliases.cpp:51
+    // Convert to a direct alias of base object.
+    GA.replaceUsesOfWith(
+        GA2, ConstantExpr::getBitCast(GA2->getBaseObject(), GA2->getType()));
----------------
pcc wrote:
> tejohnson wrote:
> > tejohnson wrote:
> > > pcc wrote:
> > > > This isn't quite right; it will rewrite
> > > > ```
> > > > ga1 = alias ga2
> > > > ga2 = alias gep(gv, 0, 1)
> > > > ```
> > > > as
> > > > ```
> > > > ga1 = alias gv
> > > > ```
> > > > i.e. you will drop the offset from the alias. The implementation in https://reviews.llvm.org/D29781#1328720 handles this case correctly.
> > > You're right. We want to look at the direct aliasee (recursively). Made that change and enhanced the test case to cover this.
> > Actually, I think my new approach won't quite work if both aliases in the chain involve expressions - probably needs something like your ConstantExpr case in D29781. Need to construct an example.
> Yes, here's an example that won't canonicalize correctly:
> ```
> ga1 = alias gep(ga2, 0, 1)
> ga2 = alias gv
> ```
Added that case to my test as well.


================
Comment at: lib/Transforms/Utils/CanonicalizeAliases.cpp:48
+  if (!GA2)
+    return Aliasee;
+
----------------
pcc wrote:
> With the current code you don't actually need the recursive call here, it could just be
> ```
> while (auto *GA2 = dyn_cast<GlobalAlias>(Aliasee))
>   Aliasee = GA2->getAliasee();
> ```
> (but ignore if you plan to make this recursive).
I decided to make it recursive, so we can fix up all aliases in the chain in one go.


================
Comment at: lib/Transforms/Utils/CanonicalizeAliases.cpp:51
+  auto *NewAliasee = canonicalizeAlias(GA2, Changed);
+  GA->replaceUsesOfWith(GA2,
+                        ConstantExpr::getBitCast(NewAliasee, GA2->getType()));
----------------
pcc wrote:
> This is just `GA->setAliasee(Aliasee)`, no?
Right, that was leftover from an older flawed version.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54507