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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm 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);
----------------
spatel wrote:
> 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.
OK, I didn't realize this was stripping away casts. As per the new langref address space rules, it should be legal to change the address space of non-volatile, known dereferenceable pointer to the original address space. However, I don't believe this pass should be responsible for changing it here. I'd expect pure load widening to not look through addrspacecast.


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

https://reviews.llvm.org/D137341