[PATCH] D129561: [RS4GC] Fix over-restrictive assert in atomic memcpy/move handling

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1705
       auto GetBaseAndOffset = [&](Value *Derived) {
-        assert(PointerToBase.count(Derived));
         unsigned AddressSpace = Derived->getType()->getPointerAddressSpace();
----------------
mkazantsev wrote:
> apilipenko wrote:
> > mkazantsev wrote:
> > > apilipenko wrote:
> > > > mkazantsev wrote:
> > > > > mkazantsev wrote:
> > > > > > mkazantsev wrote:
> > > > > > > apilipenko wrote:
> > > > > > > > I suggest handling the undef case explicitly. Just return undef if the input is undef. 
> > > > > > > Same goes for poison I guess.
> > > > > > It's a no-go. Aside from undef and poison, here can be things like
> > > > > > ```
> > > > > > i32 addrspace(1)* bitcast (i8 addrspace(1)* getelementptr inbounds (i8, i8 addrspace(1)* null, i64 16) to i32 addrspace(1)*)
> > > > > > ```
> > > > > > etc. So the expression can be arbitrarily complex.
> > > > > General idea is that undef may not be the immediate value, but a part of any other expression. Therefore, there is no way to guarantee it's not involved somewhere. The assert is just over-conservative w.r.t. this situation.
> > > > I see your point.
> > > > 
> > > > Looks like RS4GC doesn't track liveness for constants. Instead, it assumes null base for them. See https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp#L537
> > > > I think we should do the same here to be consistent. 
> > > No, we can't do it. Imagine you had smth like
> > > ```
> > > i32 addrspace(1)* bitcast (i8 addrspace(1)* getelementptr inbounds (i8, i8 addrspace(1)* %base, i64 16) to i32 addrspace(1)*)
> > > 
> > > ```
> > > and then unreached code opt made it into
> > > ```
> > > i32 addrspace(1)* bitcast (i8 addrspace(1)* getelementptr inbounds (i8, i8 addrspace(1)* %base, i64 undef) to i32 addrspace(1)*)
> > > 
> > > ```
> > > 
> > > It's unreached code opts. We can't make any assumptions here.
> > In this case, the pointer should be in the live set.
> No it's not. Just run the patch's test and see.
I'm wrong here, I was checking only null constant and that's why.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129561/new/

https://reviews.llvm.org/D129561