[PATCH] D126487: [SVE] Fixed custom lowering of ISD::INSERT_SUBVECTOR.

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11480
 
-    EVT WideVT;
-    SDValue ExtVec;
+    EVT NarrowVT = getPackedSVEVectorVT(VT.getVectorElementCount());
+    EVT WideVT = getPackedSVEVectorVT(InVT.getVectorElementCount());
----------------
david-arm wrote:
> paulwalker-arm wrote:
> > david-arm wrote:
> > > paulwalker-arm wrote:
> > > > david-arm wrote:
> > > > > Are these names the wrong way around? I would have expected the wider `VT` to be called `WideVT`. We're inserting a narrower InVT subvector into a wider VT vector.
> > > > It's wider in the context of the element type since both types have the same total bit length. `WideVT` is the wider VT because it's created based on the element count.  `InVT` (i.e. the subvector) has fewer elements than `VT` and thus its element type will need to be wider in order to match the total bit length of `NarrowVT`.
> > > OK, fair enough. So the width or narrowness refers to the element types then? I just found it really confusing that's all, as intuitively I was expecting an insert subvector operation to insert a narrower VT into a wider one. I guess what you actually mean here is WideElementVT and NarrowElementVT. I was thinking of widening in the legalisation sense, i.e. widen a <vscale x 3 x f32> -> <vscale x 4 x f32>.
> > Although true for the original insert subvector operation the confusion here is that when lowering we're actually merging two vectors of equal length.
> Yeah I see. In that case would it be possible to leave a simple comment before merging this patch just explaining that a bit of what you said earlier, i.e. that narrow and wide here refer to the element types?
Sure, will do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126487