[PATCH] D137341: [VectorCombine] widen a load with subvector insert

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:321-322
+  IRBuilder<> Builder(Load);
+  Value *CastedPtr =
+      Builder.CreatePointerBitCastOrAddrSpaceCast(SrcPtr, Ty->getPointerTo(AS));
+  Value *VecLd = Builder.CreateAlignedLoad(Ty, CastedPtr, Alignment);
----------------
arsenm wrote:
> spatel wrote:
> > arsenm wrote:
> > > Shouldn't need to create an addrspacecast here
> > I copied this line from the existing fold for a load+insertelt, and that was last changed with D121787. 
> > 
> > Is that not relevant with this transform and/or the change to opaque pointers? I'm not familiar with all of the addrspace corner-cases, so I'm not sure what to do here. 
> > 
> > Add tests derived from D121787?
> > 
> > 
> > ```
> > define <4 x i32> @load_from_other_as(ptr addrspace(5) align 16 dereferenceable(16) %p) {
> >   %asc = addrspacecast ptr addrspace(5) %p to ptr
> >   %l = load <2 x i32>, ptr %asc, align 4
> >   %s = shufflevector <2 x i32> %l, <2 x i32> poison, <4 x i32> <i32 0, i32 1, i32 undef, i32 undef>
> >   ret <4 x i32> %s
> > }
> > 
> > ```
> > 
> > 
> > 
> Opaque pointers are unrelated to the address space; there's no change there.
> 
> I don't see how it's relevant for this transform. You're widening a load, which should always be into a load with the same address space that the original load used. The only cast you should need here is the element bitcast for typed pointers
I think the issue is that `stripPointerCasts()` will peek through addrspacecasts. 

So if we do that, then we need to cast the source pointer back to the required destination addrspace. There was a comment in the code about this before D121787 changed the code to include a cast. 

It's not clear to me from the descriptions if we can use a different stripPointer* API to avoid the issue. But if we do that, then it would be better to change the existing code too, so these 2 transforms are not diverging in implementation.

I'll add a test with addrspacecast and update here, so we have some test coverage for this.


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

https://reviews.llvm.org/D137341