[PATCH] D99240: [ConstantFolding] Look through local aliases when simplify GEPs

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/lib/IR/Value.cpp:612-613
       V = cast<GlobalAlias>(V)->getAliasee();
+    } else if (StripKind == PSK_ZeroIndicesAndLocalAliases &&
+               isa<GlobalAlias>(V) && cast<GlobalAlias>(V)->hasLocalLinkage()) {
+      V = cast<GlobalAlias>(V)->getAliasee();
----------------
nikic wrote:
> pcc wrote:
> > nikic wrote:
> > > pcc wrote:
> > > > aeubanks wrote:
> > > > > pcc wrote:
> > > > > > aeubanks wrote:
> > > > > > > pcc wrote:
> > > > > > > > rnk wrote:
> > > > > > > > > I wonder if it would be safe to power up all of the other versions of pointer stripping to look through local aliases.
> > > > > > > > I don't think we should. It wouldn't be safe to allow passes to replace `@alias` with `@aliasee` with this IR:
> > > > > > > > ```
> > > > > > > > @alias = internal alias @aliasee
> > > > > > > > @aliasee = linkonce_odr global i32 42
> > > > > > > > ```
> > > > > > > > We should also avoid the QoI issue (missing symbol table entry) that would result from such replacement given this IR:
> > > > > > > > ```
> > > > > > > > @alias = internal alias @aliasee
> > > > > > > > @aliasee = private global i32 42
> > > > > > > > ```
> > > > > > > Why is the first transform not safe?
> > > > > > Because it would replace a reference to this object file's copy of `@aliasee` with a reference to the copy of `@aliasee` selected by the linker, and those could potentially be different (and have different addresses, which is observable).
> > > > > It doesn't end up using the copy of `@aliasee` selected by the linker?
> > > > > 
> > > > > globalopt actually performs this transform:
> > > > > ```
> > > > > $ cat /tmp/y.ll
> > > > > @alias = internal alias i32, i32* @aliasee
> > > > > @aliasee = linkonce_odr global i32 42
> > > > > 
> > > > > define i32 @foo() {
> > > > >   %i = load i32, i32* @alias
> > > > >   ret i32 %i
> > > > > }
> > > > > $ opt -passes=globalopt /tmp/y.ll -S
> > > > > @aliasee = linkonce_odr local_unnamed_addr global i32 42
> > > > > 
> > > > > define i32 @foo() local_unnamed_addr {
> > > > >   %i = load i32, i32* @aliasee, align 4
> > > > >   ret i32 %i
> > > > > }
> > > > > ```
> > > > > It doesn't end up using the copy of @aliasee selected by the linker?
> > > > 
> > > > No, it doesn't. `@alias` is specifically a reference to this object file's copy of the data defined by the `@aliasee` global.
> > > > 
> > > > > globalopt actually performs this transform:
> > > > 
> > > > I think that's a bug then.
> > > > Because it would replace a reference to this object file's copy of `@aliasee` with a reference to the copy of `@aliasee` selected by the linker, and those could potentially be different (and have different addresses, which is observable).
> > > 
> > > I don't think I understand the problem. Ignoring unnamed_addr, `@alias` and `@aliasee` are required to have the same address. If another copy of `@aliasee` is chosen by the linker, then `@alias` is required to point to that other copy, which is compatible with replacing uses of `@alias` with `@aliasee` in the first place.
> > Sorry, but that's not how aliases work. An alias is just another symbol that points to the same section as the aliasee. There's nothing linking them at the object file level. So they are each independently subject to symbol resolution by the linker.
> This is how aliases are specified by LangRef:
> 
> > Aliases that are not unnamed_addr are guaranteed to have the same address as the aliasee expression. unnamed_addr ones are only guaranteed to point to the same content.
> 
> If this doesn't match your understanding of how aliases work, please submit a patch to adjust the specification.
I think that's talking about prior to linker symbol resolution. The LangRef could certainly be clarified on that point though. I'll see if I can get a chance to update it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99240