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

[Jeroen Ketema via llvm-commits]

jketema added inline comments.

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:11708
@@ +11707,3 @@
+  Type *Int8Ptr = Builder.getInt8PtrTy();
+  Ops.push_back(Builder.CreateAddrSpaceCast(
+      Builder.CreateBitCast(LI->getPointerOperand(), Int8PtrAddr), Int8Ptr));
----------------
sbaranga wrote:
> jketema wrote:
> > sbaranga wrote:
> > > jketema wrote:
> > > > sbaranga wrote:
> > > > > jketema wrote:
> > > > > > sbaranga wrote:
> > > > > > > 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.
> > > > > > 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?
> > > > > 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..
> > > > 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?
> > > 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.
> > 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?
> 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.
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.


http://reviews.llvm.org/D12985