[PATCH] D47423: Simplify recursive launder.invariant.group and strip

[Richard Smith - zygoloid via Phabricator via llvm-commits]

rsmith added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1227
+  if (Result->getType() != II.getType())
+    Result = IC.Builder.CreateBitCast(Result, II.getType());
+
----------------
Prazek wrote:
> rsmith wrote:
> > If we stripped an `addrspacecast`, we would need to recreate it here; a `bitcast` might mean something else.
> > 
> > Maybe it would be simpler to only deal with bitcasts here, and have separate instcombine transforms for moving strip/launder past GEPs and addrspacecasts? You should decide which is canonical: `addrspacecast(launder)` vs `launder(addrspacecast)`.
> > 
> > Whatever you do here should also ideally be able to handle non-zero-index GEPs.
> Can you give example of transformation of non-zero index GEPs?
> Aggree with the addrspacecast, I had it in the back of my head but somehow missed that.
> launder(addrspacecast) should be better.
For a non-zero-index GEP, I was thinking of a case like:

```
struct Y { virtual void f(); };
struct X { int q; Y y; };

void f(X *p) {
  Y *a = &launder(p)->y;
  Y *b = launder(&p->y);
  // ...
}
```

... for which I thought it would be nice if `a` and `b` could be GVN'd to the same thing. But I think now that I was wrong: `a` points to an object whose static type is `Y` and whose vptr is `Y`'s vptr, whereas `b` could point to an object derived from `Y` that has been placement-new'd at the right address. In particular, `a->f()` can be devirtualized to a call to `Y::f`, but `b->f()` cannot.

That said, the issue here is not the non-zero GEP; the issue is that the member access can be thought of as adding information to the invariant group that says 'a vptr load would load this value' (but does not say that it's actually valid to synthesize such a load through the pointer). But this does mean that I don't have any examples where transformation of a non-zero-index GEP would be useful, so it doesn't seem worth working on right now :)


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1294-1295
+        "simplifyInvariantGroupIntrinsic only handles launder and strip");
+  if (Result->getType()->getPointerAddressSpace() !=
+      II.getType()->getPointerAddressSpace())
+    Result = IC.Builder.CreateAddrSpaceCast(Result, II.getType());
----------------
I think this is still not right from the `bitcast` / `addrspacecast` perspective: if we previously had a `bitcast` from one address space to another, this will convert it into an `addrspacecast`, which may mean something else. As a pathological example, if we had:

```
%a = call launder(...)   ; i8 addrspace(0)*
%b = bitcast %a to i8 addrspace(1)*
%c = addrspacecast %b to i8 addrspace(2)*
%d = bitcast %c to i8 addrspacecast(3)*
%e = call launder(%d)
```

... then you need to rebuild an addrspacecast from address space 1 to address space 2, even though your source is in address space 0 and your destination is in address space 3. (And in general, you may need to rebuild more than one address space cast.)


Repository:
  rL LLVM

https://reviews.llvm.org/D47423