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

[Piotr Padlewski via Phabricator via llvm-commits]

Prazek added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1220-1221
+    Result = IC.Builder.CreateLaunderInvariantGroup(StrippedInvariantGroupsArg);
+  else if (II.getIntrinsicID() == Intrinsic::strip_invariant_group)
+    Result = IC.Builder.CreateStripInvariantGroup(StrippedInvariantGroupsArg);
+  else
----------------
rsmith wrote:
> I think we can do a little better for the `strip(strip(p))` case. If we have:
> 
> ```
> T *p = ...;
> T *q = strip(p);
> T *r = strip(q);
> ```
> 
> ... it would seem preferable to rewrite that as:
> 
> ```
> T *r = q;
> ```
> 
> rather than as:
> 
> ```
> T *r = strip(p);
> ```
> 
> (Will GVN rewrite the latter as the former anyway?)
No, none of the optimization can remove strips. I don't think it is legal in our model and I don't see it how it could work


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1227
+  if (Result->getType() != II.getType())
+    Result = IC.Builder.CreateBitCast(Result, II.getType());
+
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D47423