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

Richard Smith - zygoloid via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jul 12 16:16:52 PDT 2018


rsmith accepted this revision.
rsmith added inline comments.
This revision is now accepted and ready to land.


================
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());
----------------
Prazek wrote:
> 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.
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D47423





More information about the llvm-commits mailing list