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

[silviu.baranga@arm.com via llvm-commits]

sbaranga 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));
----------------
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.


http://reviews.llvm.org/D12985