[PATCH] D145485: [IR] Generalize interleave/deinterleave intrinsics to factors > 2

[Luke Lau via Phabricator via llvm-commits]

luke added inline comments.


================
Comment at: llvm/include/llvm/IR/Intrinsics.td:2120-2128
-
-def int_experimental_vector_interleave2   : DefaultAttrsIntrinsic<[llvm_anyvector_ty],
-                                                                  [LLVMHalfElementsVectorType<0>,
-                                                                   LLVMHalfElementsVectorType<0>],
+def int_experimental_vector_interleave   : DefaultAttrsIntrinsic<[llvm_anyvector_ty],
+                                                                  [llvm_vararg_ty],
                                                                   [IntrNoMem]>;
 
-def int_experimental_vector_deinterleave2 : DefaultAttrsIntrinsic<[LLVMHalfElementsVectorType<0>,
----------------
paulwalker-arm wrote:
> luke wrote:
> > paulwalker-arm wrote:
> > > luke wrote:
> > > > paulwalker-arm wrote:
> > > > > luke wrote:
> > > > > > This is the main disadvantage of using just one intrinsic to represent all factors: We have to use variadic arguments which complicates the type signature, and we need to do the verification ourselves in Verifier.cpp. 
> > > > > > 
> > > > > > I also considered just creating separate intrinsics for each interleave factor, but didn't like the  duplication that would be required in `Legalise*Types.cpp`/`SelectionDAGBuilder.cpp`.
> > > > > I don't like this change because I feel it makes the intrinsics cumbersome to work with due to them being so open-ended when compare to the current form.
> > > > > 
> > > > > That said, based on the rational I don't think the change is necessary because when designing the initial support we concluded there was no need for the instrinsics and ISD nodes to follow the same interface and in fact good reasons for them to differ.  This means it's perfectly acceptable to add more intrinsics (i.e. int_experimental_vector_interleave3, int_experimental_vector_interleave4...) that all lower to the same ISD node.  This is why the intrinsic is numbered and the ISD is not.
> > > > Thanks for the review. I’m not very strongly opinionated on this, and would be happy to rework this to use separate distinct intrinsics. 
> > > > 
> > > > In RISC-V the maximum number of interleaving fields in a load/store is 8, so as long as we have up to interleave8/deinterleave8 that should be fine. Does adding those intrinsics sound like a good way forward?
> > > That would be consistent with the current design but the answer really depends on whether such intrinsics will actually be used.  This is why for AArch64 we've started with the 2-way variant only.  This allows us to teach loop vectorise how to use them, along with implementing any necessary combines etc.., and thus understand the pitfalls before moving on to other interleaving factors.
> > One thing I've ran into in D145495 is how you can get non-power-of-two element counts in your types like `nxv6i32` quite easily with other factors, which need a little bit of attention since they aren't valid MVTs. 
> You likely want to use EVTs during the early stages of code generation.  Once type legalisation has been carried out, I'd expect MVTs to then be sufficient.
The snag with using EVTs (IIUC) is that we end up introducing illegal types during the LegalizeDAG phase, which throws an assertion. 

For the ISD node
`t8: nxv2i32,nxv2i32,nxv2i32 = vector_interleave t2, t4, t6`
`t2`,`t4` and `t6` already come in as legal `nxv2i32` types.

When `vector_interleave` is then lowered during LegalizeDAG, if we just use EVTs and generate the "logical" `nxv6i32` type then we emit:
`t38: nxv6i32 = RISCVISD::VRGATHEREI16_VV_VL t24, t34, undef:nxv6i32, t36, Register:i64 $x0`

Which is now illegal, and asserts at LegalizeDAG.cpp:975.
I presume the design of LegalizeDAG is such that it relies on LegalizeTypes to make sure the types are legal coming in, and presumes that any further legalisation/target lowering will preserve the types.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145485