[ARM] Take into account address spaces in interleaved access vectorization

[jketema]

Summary

The vector being loaded might not be in address space 0. In this case the vldn/vstn call creation will trigger an assert in CallInst:Create, because the argument type and the parameter are in different address spaces. Resolve this by adding an appropriate address space cast.

[sbaranga]

lib/Target/ARM/ARMISelLowering.cpp

11708

This is interesting.. We ideally wouldn't want to introduce address space casts when not necessary (I can imagine some users using the address space qualifier to help with alias analysis).

We should be able to get a variant of the vldn intrinsic that operates on the correct address space.

[jketema]

lib/Target/ARM/ARMISelLowering.cpp

11708

Maybe. I would need some pointers on how to change the code in that case, because this would require changes to IntrinsicsARM.td, which I'm not very familiar with.

I'm also wondering: is alias analysis information still being used during instruction lowering or later?

[sbaranga]

lib/Target/ARM/ARMISelLowering.cpp

11708

I've looked into this and changing IntrinisicsARM.td should be fairly simple, all that's needed is to replace in definitions like int_arm_neon_vld2 the llvm_ptr_ty argument with a llvm_anyptr_ty.

However it turns out that this changes the intrinisic name mangling (which might be a problem with backward compatibility) and also requires clang changes. This is of course a bit complicated..

[jketema]

lib/Target/ARM/ARMISelLowering.cpp

11708

Thanks for the pointer regarding llvm_anyptr_ty.

Maybe map, say, __builtin_neon_vld3_v to @llvm.arm.neon.vld3.v2i32.p0i8 instead of @llvm.arm.neon.vld3.v2i32 and keep that as the only exposed version? Or are there objections to that solution too?

[sbaranga]

lib/Target/ARM/ARMISelLowering.cpp

11708

Yes, we would have to do that and also add auto-upgrade support for the old forms of the intrinsics. Perhaps an email should be sent to llvm-dev as well.

[jketema]

lib/Target/ARM/ARMISelLowering.cpp

11708

I'm not sure what you're saying. Sticking with vld3 Do you mean you both want to expose @llvm.arm.neon.vld3.v2i32.p0i8 and @llvm.arm.neon.vld3.v2i32? Or do you want more exposed?

I'm also not quite sure what you mean by auto-upgrade support?

[sbaranga]

lib/Target/ARM/ARMISelLowering.cpp

11708

I think we should make the change to IntrinsicARM.td (replace llvm_ptr_ty with llvm_anyptr_ty for the vldN intrinsics). At that point we will only have the new style intrinsics in llvm IR (like @llvm.arm.neon.vld3.v2i32.p0i8) and the old ones ( like @llvm.arm.neon.vld3.v2i32) will not be supported anymore.

We therefore need to add a case in lib/IR/AutoUpgrade.cpp for the old style vldN intrinsics so that we can convert old style intrinsics to the new one and not break backwards compatibility.

Regarding the clang changes (__builtin_neon_vld*..), yes, these should be emitting @llvm.arm.neon.vld3.v2i32.p0i8.

[jketema]

lib/Target/ARM/ARMISelLowering.cpp

11708

Got it, thanks for the explanation. I was not aware of lib/IR/AutoUpgrade.cpp.

I'll update this patch to take your comments into account and whip up an associated patch for clang. Once that's done I'll also send a message to llvm-dev to ask for more feedback.

[jketema]

This now upgrades the vld[234] and vst[234] intrinsics to take into account an address space, as discussed.

Open issues as far as I'm concerned:

• Should vld1 and vst1 also be upgraded for uniformity? Similarly for the vld[234]lane and vst[234]lane instructions?
• Should all the tests referring to vld[234] and vst[234] be updated to use the new intrinsic names, or are they allowed to depend on the auto-upgrade code?

[sbaranga]

Thanks, Jeroen!

In D12985#252574, @jketema wrote:

This now upgrades the vld[234] and vst[234] intrinsics to take into account an address space, as discussed.

Open issues as far as I'm concerned:

• Should vld1 and vst1 also be upgraded for uniformity? Similarly for the vld[234]lane and vst[234]lane instructions?

They seem to have the same issues, so I would say yes.

• Should all the tests referring to vld[234] and vst[234] be updated to use the new intrinsic names, or are they allowed to depend on the auto-upgrade code?

I think they should use the new names, and have different tests to check that the old ones get auto-upgraded.

[jketema]

This extends the patch to cover all of vld[1234], vls[234]lane, vst[1234], and vst[234]lane. All tests now use the updated intrinsic names, and new tests have been added to test auto-upgrading.

Should I still email llvm-dev about this? If so, what exaclty should I ask about.

Forgot to say in my previous comment. The related clang patch is http://reviews.llvm.org/D13127

[sbaranga]

In D12985#254767, @jketema wrote:

This extends the patch to cover all of vld[1234], vls[234]lane, vst[1234], and vst[234]lane. All tests now use the updated intrinsic names, and new tests have been added to test auto-upgrading.

Should I still email llvm-dev about this? If so, what exaclty should I ask about.

Forgot to say in my previous comment. The related clang patch is http://reviews.llvm.org/D13127

Both patches look good!

The purpose of the email to llvm-dev would be to announce the interface change and to ask if this would be a problem for anyone (it shouldn't be since you've added the auto-upgrade).

[sbaranga]

LGTM

[jketema]

Committed. Thanks for all the feedback!

[sbaranga]

Thanks for all the work! :)

lib/Target/ARM/ARMISelLowering.cpp

static const Intrinsic::ID LoadInts[3] = {Intrinsic::arm_neon_vld2,
                                             Intrinsic::arm_neon_vld4};
 
   Function *VldnFunc =
       Intrinsic::getDeclaration(LI->getModule(), LoadInts[Factor - 2], VecTy);
 
   IRBuilder<> Builder(LI);
   SmallVector<Value *, 2> Ops;
 
-  Type *Int8Ptr = Builder.getInt8PtrTy(LI->getPointerAddressSpace());
-  Ops.push_back(Builder.CreateBitCast(LI->getPointerOperand(), Int8Ptr));
+  Type *Int8PtrAddr = Builder.getInt8PtrTy(LI->getPointerAddressSpace());
+  Type *Int8Ptr = Builder.getInt8PtrTy();
+  Ops.push_back(Builder.CreateAddrSpaceCast(
+      Builder.CreateBitCast(LI->getPointerOperand(), Int8PtrAddr), Int8Ptr));
   Ops.push_back(Builder.getInt32(LI->getAlignment()));
 
   CallInst *VldN = Builder.CreateCall(VldnFunc, Ops, "vldN");
 
   // Replace uses of each shufflevector with the corresponding vector loaded
   // by ldN.
   for (unsigned i = 0; i < Shuffles.size(); i++) {
     ShuffleVectorInst *SV = Shuffles[i];
bool ARMTargetLowering::lowerInterleavedStore(StoreInst *SI,
   static Intrinsic::ID StoreInts[3] = {Intrinsic::arm_neon_vst2,
                                        Intrinsic::arm_neon_vst3,
                                        Intrinsic::arm_neon_vst4};
   Function *VstNFunc = Intrinsic::getDeclaration(
       SI->getModule(), StoreInts[Factor - 2], SubVecTy);
 
   SmallVector<Value *, 6> Ops;
 
-  Type *Int8Ptr = Builder.getInt8PtrTy(SI->getPointerAddressSpace());
-  Ops.push_back(Builder.CreateBitCast(SI->getPointerOperand(), Int8Ptr));
+  Type *Int8PtrAddr = Builder.getInt8PtrTy(SI->getPointerAddressSpace());
+  Type *Int8Ptr = Builder.getInt8PtrTy();
+  Ops.push_back(Builder.CreateAddrSpaceCast(
+    Builder.CreateBitCast(SI->getPointerOperand(), Int8PtrAddr), Int8Ptr));
 
   // Split the shufflevector operands into sub vectors for the new vstN call.
   for (unsigned i = 0; i < Factor; i++)
     Ops.push_back(Builder.CreateShuffleVector(
         Op0, Op1, getSequentialMask(Builder, NumSubElts * i, NumSubElts)));
 
   Ops.push_back(Builder.getInt32(SI->getAlignment()));
   Builder.CreateCall(VstNFunc, Ops);

test/CodeGen/ARM/arm-interleaved-accesses.ll

 define void @store_undef_mask_factor4(i32* %ptr, <4 x i32> %v0, <4 x i32> %v1, <4 x i32> %v2, <4 x i32> %v3) {
   %base = bitcast i32* %ptr to <16 x i32>*
   %v0_v1 = shufflevector <4 x i32> %v0, <4 x i32> %v1, <8 x i32> <i32 0, i32 1, i32 2, i32 3, i32 4, i32 5, i32 6, i32 7>
   %v2_v3 = shufflevector <4 x i32> %v2, <4 x i32> %v3, <8 x i32> <i32 0, i32 1, i32 2, i32 3, i32 4, i32 5, i32 6, i32 7>
   %interleaved.vec = shufflevector <8 x i32> %v0_v1, <8 x i32> %v2_v3, <16 x i32> <i32 0, i32 4, i32 8, i32 undef, i32 undef, i32 5, i32 9, i32 13, i32 2, i32 6, i32 10, i32 14, i32 3, i32 7, i32 11, i32 15>
   store <16 x i32> %interleaved.vec, <16 x i32>* %base, align 4
   ret void
 }
+
+; The following test cases check that address spaces are properly handled
+
+; CHECK-LABEL: load_address_space
+; CHECK: vld3.32
+define void @load_address_space(<4 x i32> addrspace(1)* %A, <2 x i32>* %B) {
+ %tmp = load <4 x i32>, <4 x i32> addrspace(1)* %A
+ %interleaved = shufflevector <4 x i32> %tmp, <4 x i32> undef, <2 x i32> <i32 0, i32 3>
+ store <2 x i32> %interleaved, <2 x i32>* %B
+ ret void
+}
+
+; CHECK-LABEL: store_address_space
+; CHECK: vst2.32
+define void @store_address_space(<2 x i32>* %A, <2 x i32>* %B, <4 x i32> addrspace(1)* %C) {
+ %tmp0 = load <2 x i32>, <2 x i32>* %A
+ %tmp1 = load <2 x i32>, <2 x i32>* %B
+ %interleaved = shufflevector <2 x i32> %tmp0, <2 x i32> %tmp1, <4 x i32> <i32 0, i32 2, i32 1, i32 3>
+ store <4 x i32> %interleaved, <4 x i32> addrspace(1)* %C
+ ret void
+}