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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

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

> In D99240#2649567 <https://reviews.llvm.org/D99240#2649567>, @pcc wrote:
>
>> Can you make it so that the load ends up being replaced with the loaded value, without replacing the GEP itself? That would seem to address the MSVC issue.
>
> What's the benefit of that over this approach?

In general, the use of an alias is deliberate. We shouldn't be indiscriminately replacing them with aliasees as that may break semantics or lead to QoI issues. Instead, we should be making targeted replacements where they are known to be safe. In particular, replacing the load with the loaded value is safe because linkonce_odr (or external, etc.) on the alias guarantees that the value loaded from any copy of the aliasee is the same. So it doesn't matter whether we (a compile time) read from this translation unit's copy of the global or some other copy.

> We'd have to potentially end up doing the same thing for stores and maybe other places.

Can you show an example of a beneficial transformation that would apply to stores?



================
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:
> > 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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99240