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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
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()));
----------------
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
```


================
Comment at: lib/Transforms/Utils/CanonicalizeAliases.cpp:40
+
+namespace llvm {
+
----------------
I think you can change this to `namespace {` and then the functions below don't need to be static.


================
Comment at: lib/Transforms/Utils/CanonicalizeAliases.cpp:48
+  if (!GA2)
+    return Aliasee;
+
----------------
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).


================
Comment at: lib/Transforms/Utils/CanonicalizeAliases.cpp:51
+  auto *NewAliasee = canonicalizeAlias(GA2, Changed);
+  GA->replaceUsesOfWith(GA2,
+                        ConstantExpr::getBitCast(NewAliasee, GA2->getType()));
----------------
This is just `GA->setAliasee(Aliasee)`, no?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54507