[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:1220-1221
+    Result = IC.Builder.CreateLaunderInvariantGroup(StrippedInvariantGroupsArg);
+  else if (II.getIntrinsicID() == Intrinsic::strip_invariant_group)
+    Result = IC.Builder.CreateStripInvariantGroup(StrippedInvariantGroupsArg);
+  else
----------------
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?)


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


Repository:
  rL LLVM

https://reviews.llvm.org/D47423