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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

In D99240#2651342 <https://reviews.llvm.org/D99240#2651342>, @aeubanks wrote:

> Ah I see, https://github.com/llvm/llvm-project/blob/61a55c8812e790842799ba1de5bd81fe8afb3b16/llvm/lib/Transforms/IPO/GlobalOpt.cpp#L2953. It only works if the aliasee is a global value. Extending globalopt to handle aliasees that may be a constant expression containing a global value, rather than just a global value, would probably work as well. Either way is fine by me.
>
> globalopt specifically deletes aliases when possible, so it seems fair to do that here as well, even if that may remove entries from the symbol table.

The presence of a bug in globalopt doesn't justify introducing a bug here as well.



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99240