[PATCH] D12985: [ARM] Take into account address spaces in interleaved access vectorization
Jeroen Ketema via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 23 02:21:12 PDT 2015
jketema added inline comments.
Comment at: lib/Target/ARM/ARMISelLowering.cpp:11708
@@ +11707,3 @@
+ Type *Int8Ptr = Builder.getInt8PtrTy();
+ 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.
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.
More information about the llvm-commits