[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:1294-1295
+        "simplifyInvariantGroupIntrinsic only handles launder and strip");
+  if (Result->getType()->getPointerAddressSpace() !=
+      II.getType()->getPointerAddressSpace())
+    Result = IC.Builder.CreateAddrSpaceCast(Result, II.getType());
----------------
rsmith wrote:
> Prazek wrote:
> > rsmith wrote:
> > > 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.)
> > 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?
> > 
> > 
> 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 :)
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D47423