[PATCH] D54507: [ThinLTO] Handle chains of aliases
Teresa Johnson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Dec 13 08:44:28 PST 2018
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
More information about the llvm-commits
mailing list