Simplify recursive launder.invariant.group and strip

[Prazek]

Summary

This patch is crucial for proving equality laundered/stripped
pointers. eg:

bool foo(A *a) {
  return a == std::launder(a);
}

Clang with -fstrict-vtable-pointers will emit something like:

define dso_local zeroext i1 @_Z3fooP1A(%struct.A* %a) {
entry:
  %c = bitcast %struct.A* %a to i8*
  %call = tail call i8* @llvm.launder.invariant.group.p0i8(i8* %c)
  %0 = bitcast %struct.A* %a to i8*
  %1 = tail call i8* @llvm.strip.invariant.group.p0i8(i8* %0)
  %2 = tail call i8* @llvm.strip.invariant.group.p0i8(i8* %call)
  %cmp = icmp eq i8* %1, %2
  ret i1 %cmp
}

and because %2 can be replaced with @llvm.strip.invariant.group(%0)
and that %2 and %1 will produce the same value (because strip is readnone)
we can replace compare with true.

[amharc]

Otherwise looks good to me. Obviously, you should wait for someone more competent than me to approve it ;)

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

1212 ↗

(On Diff #148751)

nit: you use II.getArgOperand(0) in the line above and II.getOperand(0) here. Those are obviously equivalent, but maybe stick to one of those (possibly extracting it into auto *Arg = II.getArgOperand(0);)?

[Prazek]

fixed nit

[kuhar]

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

1223 ↗

(On Diff #148756)

llvm_unreachable would be preferred here

[Prazek]

unreachable

[Prazek]

updated test

[xbolva00]

Looks fine

[rsmith]

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

1220–1221 ↗

(On Diff #150706)

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?)

1227 ↗

(On Diff #150706)

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.

[Prazek]

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

1220–1221 ↗

(On Diff #150706)

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

1227 ↗

(On Diff #150706)

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.

[Prazek]

fixed addrspace cast

[amharc]

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

1220–1221 ↗

(On Diff #150706)

Isn't strip supposed to be pure, and hence two calls to strip(p) should be CSE'd by GVN?

[Prazek]

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

1220–1221 ↗

(On Diff #150706)

Oh sorry Richard, I thought you mean

T *r = p;

The outcome of this transformation will be

T *q = strip(p);
T* r = strip(p);

And then GVN will see that strip(p) is always the same and will do

T* q = strip(p);
T* r = q;

So that is exactly what you said.

[rsmith]

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

1294–1295 ↗

(On Diff #153671)

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

1227 ↗

(On Diff #150706)

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

[Prazek]

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

1294–1295 ↗

(On Diff #153671)

AFAIK the addrspace casts are just like bitcasts, so code like this:

define i8 addrspace(3)* @foo(i8* %a) {

  %b = addrspacecast i8* %a to i8 addrspace(1)*
  %c = addrspacecast i8 addrspace(1)* %b to i8 addrspace(2)*
  %d = addrspacecast i8 addrspace(2)* %c to i8 addrspace(3)*
  
  ret i8 addrspace(3)* %d
}

Is optimized by opt to:

define i8 addrspace(3)* @foo(i8* readnone %a) local_unnamed_addr #0 {
  %d = addrspacecast i8* %a to i8 addrspace(3)*
  ret i8 addrspace(3)* %d
}

I didn't use bitcasts because if I am not wrong, you cant cast between addrspace with bitcast. Do you think that with this assumption the transformation is correct?

[rsmith]

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

1294–1295 ↗

(On Diff #153671)

I can't find any restriction that would prevent bitcasting between pointers to different address spaces; the only restriction on pointer bitcast I can find in the LangRef is that the source and destination pointer types be the same size. In practice, LLVM appears to produce inttoptr(ptrtoint(p)) instead of a cross-address-space bitcast, though, so maybe it's not actually permitted.

Feedback from someone who knows the IR rules better than I do would be useful :)

[Prazek]

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

1294–1295 ↗

(On Diff #153671)

I tried the bitcasting that you showed and it failed like:

opt-5.0: test.ll:3:18: error: invalid cast opcode for cast from 'i8*' to 'i8 addrspace(1)*'

So I guess the LangRef should be more precise about it.

But anyway, Even if it would be ok, it does not change anything - we can still get rid of the bitcasts/addrspacecasts when going throught multiple conversions.

[rsmith]

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

1294–1295 ↗

(On Diff #153671)

OK, if the verifier rejects the bitcasts then I'm happy assuming they're not valid IR, and the inttoptr(ptrtoint(X)) construction is the only way to get that effect.

llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp

static Instruction *simplifyMaskedGather(IntrinsicInst &II, InstCombiner &IC) {
   // If the mask is all zeros, return the "passthru" argument of the gather.
   auto *ConstMask = dyn_cast<Constant>(II.getArgOperand(2));
   if (ConstMask && ConstMask->isNullValue())
     return IC.replaceInstUsesWith(II, II.getArgOperand(3));
 
   return nullptr;
 }
 
+/// This function transforms launder.invariant.group and strip.invariant.group
+/// like:
+/// launder(launder(%x)) -> launder(%x)       (the result is not the argument)
+/// launder(strip(%x)) -> launder(%x)
+/// strip(strip(%x)) -> strip(%x)             (the result is not the argument)
+/// strip(launder(%x)) -> strip(%x)
+/// This is legal because it preserves the most recent information about
+/// the presence or absence of invariant.group.
+static Instruction *simplifyInvariantGroupIntrinsic(IntrinsicInst &II,
+                                                    InstCombiner &IC) {
+  auto *Arg = II.getArgOperand(0);
+  auto *StrippedArg = Arg->stripPointerCasts();
+  auto *StrippedInvariantGroupsArg = Arg->stripPointerCastsAndInvariantGroups();
+  if (StrippedArg == StrippedInvariantGroupsArg)
+    return nullptr; // No launders/strips to remove.
+
+  Value *Result = nullptr;
+
+  if (II.getIntrinsicID() == Intrinsic::launder_invariant_group)
+    Result = IC.Builder.CreateLaunderInvariantGroup(StrippedInvariantGroupsArg);
+  else if (II.getIntrinsicID() == Intrinsic::strip_invariant_group)
+    Result = IC.Builder.CreateStripInvariantGroup(StrippedInvariantGroupsArg);
+  else
+    llvm_unreachable(
+        "simplifyInvariantGroupIntrinsic only handles launder and strip");
+  if (Result->getType()->getPointerAddressSpace() !=
+      II.getType()->getPointerAddressSpace())
+    Result = IC.Builder.CreateAddrSpaceCast(Result, II.getType());
+  if (Result->getType() != II.getType())
+    Result = IC.Builder.CreateBitCast(Result, II.getType());
+
+  return cast<Instruction>(Result);
+}
+
 static Instruction *simplifyMaskedScatter(IntrinsicInst &II, InstCombiner &IC) {
   // If the mask is all zeros, a scatter does nothing.
   auto *ConstMask = dyn_cast<Constant>(II.getArgOperand(3));
   if (ConstMask && ConstMask->isNullValue())
     return IC.eraseInstFromFunction(II);
 
   return nullptr;
 }
if (Value *SimplifiedMaskedOp = simplifyMaskedLoad(*II, Builder))
       return replaceInstUsesWith(CI, SimplifiedMaskedOp);
     break;
   case Intrinsic::masked_store:
     return simplifyMaskedStore(*II, *this);
   case Intrinsic::masked_gather:
     return simplifyMaskedGather(*II, *this);
   case Intrinsic::masked_scatter:
     return simplifyMaskedScatter(*II, *this);
-
+  case Intrinsic::launder_invariant_group:
+  case Intrinsic::strip_invariant_group:
+    if (auto *SkippedBarrier = simplifyInvariantGroupIntrinsic(*II, *this))
+      return replaceInstUsesWith(*II, SkippedBarrier);
+    break;
   case Intrinsic::powi:
     if (ConstantInt *Power = dyn_cast<ConstantInt>(II->getArgOperand(1))) {
       // 0 and 1 are handled in instsimplify
 
       // powi(x, -1) -> 1/x
       if (Power->isMinusOne())
         return BinaryOperator::CreateFDiv(ConstantFP::get(CI.getType(), 1.0),
                                           II->getArgOperand(0));

llvm/trunk/test/Analysis/ValueTracking/invariant.group.ll

 ; RUN: opt -S -instsimplify -instcombine < %s | FileCheck %s
 
 ; CHECK-LABEL: define void @checkNonnullLaunder()
 define void @checkNonnullLaunder() {
-; CHECK:   %p = call i8* @llvm.launder.invariant.group.p0i8(i8* nonnull %0)
-; CHECK:   %p2 = call i8* @llvm.launder.invariant.group.p0i8(i8* nonnull %p)
-; CHECK:   call void @use(i8* nonnull %p2)
+; CHECK:   %[[p:.*]] = call i8* @llvm.launder.invariant.group.p0i8(i8* nonnull %0)
+; CHECK:   call void @use(i8* nonnull %[[p]])
 entry:
   %0 = alloca i8, align 8
 
   %p = call i8* @llvm.launder.invariant.group.p0i8(i8* %0)
   %p2 = call i8* @llvm.launder.invariant.group.p0i8(i8* %p)
   call void @use(i8* %p2)
 
   ret void
 }
 
 ; CHECK-LABEL: define void @checkNonnullStrip()
 define void @checkNonnullStrip() {
-; CHECK:   %p = call i8* @llvm.strip.invariant.group.p0i8(i8* nonnull %0)
-; CHECK:   %p2 = call i8* @llvm.strip.invariant.group.p0i8(i8* nonnull %p)
-; CHECK:   call void @use(i8* nonnull %p2)
+; CHECK:   %[[p:.*]] = call i8* @llvm.strip.invariant.group.p0i8(i8* nonnull %0)
+; CHECK:   call void @use(i8* nonnull %[[p]])
 entry:
   %0 = alloca i8, align 8
 
   %p = call i8* @llvm.strip.invariant.group.p0i8(i8* %0)
   %p2 = call i8* @llvm.strip.invariant.group.p0i8(i8* %p)
   call void @use(i8* %p2)
 
   ret void
 }
 
 declare i8* @llvm.launder.invariant.group.p0i8(i8*)
 declare i8* @llvm.strip.invariant.group.p0i8(i8*)
 
 declare void @use(i8*)

llvm/trunk/test/Transforms/InstCombine/invariant.group.ll

-; RUN: opt -instcombine -S < %s | FileCheck %s
+; RUN: opt -instcombine -early-cse -S < %s | FileCheck %s
 
 
 ; CHECK-LABEL: define i8* @simplifyNullLaunder()
 define i8* @simplifyNullLaunder() {
 ; CHECK-NEXT: ret i8* null
   %b2 = call i8* @llvm.launder.invariant.group.p0i8(i8* null)
   ret i8* %b2
 }

 
 ; CHECK-LABEL: define i8 addrspace(42)* @simplifyUndefLaunder2()
 define i8 addrspace(42)* @simplifyUndefLaunder2() {
 ; CHECK-NEXT: ret i8 addrspace(42)* undef
   %b2 = call i8 addrspace(42)* @llvm.launder.invariant.group.p42i8(i8 addrspace(42)* undef)
   ret i8 addrspace(42)* %b2
 }
 
-declare i8* @llvm.launder.invariant.group.p0i8(i8*)
-declare i8 addrspace(42)* @llvm.launder.invariant.group.p42i8(i8 addrspace(42)*)
-
-
 ; CHECK-LABEL: define i8* @simplifyNullStrip()
 define i8* @simplifyNullStrip() {
 ; CHECK-NEXT: ret i8* null
   %b2 = call i8* @llvm.strip.invariant.group.p0i8(i8* null)
   ret i8* %b2
 }
 
 ; CHECK-LABEL: define i8* @dontSimplifyNullStripNonNullOpt()

 
 ; CHECK-LABEL: define i8 addrspace(42)* @simplifyUndefStrip2()
 define i8 addrspace(42)* @simplifyUndefStrip2() {
 ; CHECK-NEXT: ret i8 addrspace(42)* undef
   %b2 = call i8 addrspace(42)* @llvm.strip.invariant.group.p42i8(i8 addrspace(42)* undef)
   ret i8 addrspace(42)* %b2
 }
 
+; CHECK-LABEL: define i8* @simplifyLaunderOfLaunder(
+define i8* @simplifyLaunderOfLaunder(i8* %a) {
+; CHECK:   call i8* @llvm.launder.invariant.group.p0i8(i8* %a)
+; CHECK-NOT: llvm.launder.invariant.group
+  %a2 = call i8* @llvm.launder.invariant.group.p0i8(i8* %a)
+  %a3 = call i8* @llvm.launder.invariant.group.p0i8(i8* %a2)
+  ret i8* %a3
+}
+
+; CHECK-LABEL: define i8* @simplifyStripOfLaunder(
+define i8* @simplifyStripOfLaunder(i8* %a) {
+; CHECK-NOT: llvm.launder.invariant.group
+; CHECK:   call i8* @llvm.strip.invariant.group.p0i8(i8* %a)
+  %a2 = call i8* @llvm.launder.invariant.group.p0i8(i8* %a)
+  %a3 = call i8* @llvm.strip.invariant.group.p0i8(i8* %a2)
+  ret i8* %a3
+}
+
+; CHECK-LABEL: define i1 @simplifyForCompare(
+define i1 @simplifyForCompare(i8* %a) {
+  %a2 = call i8* @llvm.launder.invariant.group.p0i8(i8* %a)
+
+  %a3 = call i8* @llvm.strip.invariant.group.p0i8(i8* %a2)
+  %b2 = call i8* @llvm.strip.invariant.group.p0i8(i8* %a)
+  %c = icmp eq i8* %a3, %b2
+; CHECK: ret i1 true
+  ret i1 %c
+}
+
+; CHECK-LABEL: define i16* @skipWithDifferentTypes(
+define i16* @skipWithDifferentTypes(i8* %a) {
+  %a2 = call i8* @llvm.launder.invariant.group.p0i8(i8* %a)
+  %c1 = bitcast i8* %a2 to i16*
+
+  ; CHECK: %[[b:.*]] = call i8* @llvm.strip.invariant.group.p0i8(i8* %a)
+  %a3 = call i16* @llvm.strip.invariant.group.p0i16(i16* %c1)
+  ; CHECK-NEXT: %[[r:.*]] = bitcast i8* %[[b]] to i16*
+  ; CHECK-NEXT: ret i16* %[[r]]
+  ret i16* %a3
+}
+
+; CHECK-LABEL: define i16 addrspace(42)* @skipWithDifferentTypesAddrspace(
+define i16 addrspace(42)* @skipWithDifferentTypesAddrspace(i8 addrspace(42)* %a) {
+  %a2 = call i8 addrspace(42)* @llvm.launder.invariant.group.p42i8(i8 addrspace(42)* %a)
+  %c1 = bitcast i8 addrspace(42)* %a2 to i16 addrspace(42)*
+
+  ; CHECK: %[[b:.*]] = call i8 addrspace(42)* @llvm.strip.invariant.group.p42i8(i8 addrspace(42)* %a)
+  %a3 = call i16 addrspace(42)* @llvm.strip.invariant.group.p42i16(i16 addrspace(42)* %c1)
+  ; CHECK-NEXT: %[[r:.*]] = bitcast i8 addrspace(42)* %[[b]] to i16 addrspace(42)*
+  ; CHECK-NEXT: ret i16 addrspace(42)* %[[r]]
+  ret i16 addrspace(42)* %a3
+}
+
+; CHECK-LABEL: define i16 addrspace(42)* @skipWithDifferentTypesDifferentAddrspace(
+define i16 addrspace(42)* @skipWithDifferentTypesDifferentAddrspace(i8* %a) {
+  %cast = addrspacecast i8* %a to i8 addrspace(42)*
+  %a2 = call i8 addrspace(42)* @llvm.launder.invariant.group.p42i8(i8 addrspace(42)* %cast)
+  %c1 = bitcast i8 addrspace(42)* %a2 to i16 addrspace(42)*
+
+  ; CHECK: %[[b:.*]] = call i8* @llvm.strip.invariant.group.p0i8(i8* %a)
+  %a3 = call i16 addrspace(42)* @llvm.strip.invariant.group.p42i16(i16 addrspace(42)* %c1)
+  ; CHECK-NEXT: %[[r:.*]] = bitcast i8* %[[b]] to i16*
+  ; CHECK-NEXT: %[[r2:.*]] = addrspacecast i16* %[[r]] to i16 addrspace(42)*
+  ; CHECK-NEXT: ret i16 addrspace(42)* %[[r2]]
+  ret i16 addrspace(42)* %a3
+}
+
+declare i8* @llvm.launder.invariant.group.p0i8(i8*)
+declare i8 addrspace(42)* @llvm.launder.invariant.group.p42i8(i8 addrspace(42)*)
 declare i8* @llvm.strip.invariant.group.p0i8(i8*)
 declare i8 addrspace(42)* @llvm.strip.invariant.group.p42i8(i8 addrspace(42)*)
+declare i16* @llvm.strip.invariant.group.p0i16(i16* %c1)
+declare i16 addrspace(42)* @llvm.strip.invariant.group.p42i16(i16 addrspace(42)* %c1)
 
 attributes #0 = { "null-pointer-is-valid"="true" }