[PATCH] D106028: [AArch64][SelectionDAG] Add legalization for widening LOAD/MLOAD.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Apologies for the delay in reviewing this, it has been on my to-do list for quite a while!

Thanks for adding support for `<vscale x 1 x eltty>` types and adding a test for widening the even-numbered ElementCounts (e.g. `<vscale x 6 x i16>`), it's nice to see that this works too.

Other than some minor comments, the patch looks mostly fine to me.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:4881-4883
+    EVT ConcatOpVT = EVT::getVectorVT(
+        *DAG.getContext(), NOutVT.getVectorElementType(),
+        N->getOperand(0).getValueType().getVectorElementCount());
----------------
nit: Is/would this be the same as:

  SDValue Op0 = N->getOperand(0);
  TLI.getTypeToTransformTo(*DAG.getContext(), Op0.getValueType());

?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:4011
+  if (VT.isScalableVector()) {
+    unsigned WidenNumElts = WidenVT.getVectorMinNumElements();
+    unsigned InNumElts = InVT.getVectorMinNumElements();
----------------
This code is identical to the block on lines 4022-4026, with the exception of `s/NumElements/MinNumElements`. Can you align the implementation in the two blocks and remove the `if (VT.isScalableVector())`? (and then move the `report_fatal_error` to line 4027)


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:4017
+
+  if (InVT.isScalableVector()) {
     report_fatal_error("Don't know how to widen the result of "
----------------
nit: unnecessary curly braces.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:4062
+    SDLoc dl(N);
+    EVT WidenVT = TLI.getTypeToTransformTo(*DAG.getContext(), VT);
+    EVT MaskVT = EVT::getVectorVT(*DAG.getContext(), MVT::i1,
----------------
nit: can you move `WidenVT` closer to where it's used? (above line 4079)


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:4048
+
+  // FIXME: Figure out how to replace constant "2".
+  if (VT.isScalableVector() && VT.getVectorMinNumElements() % 2 != 0) {
----------------
efriedma wrote:
> sdesmalen wrote:
> > Do we also want to do this for other element-counts that are not a power of 2? (e.g. `<vscale x 6 x i8>`)
> > 
> > Was this code intended to work for `<vscale x 1 x eltty>` ? (I tried that a few days ago with this patch and ran into some failures. I didn't really investigate further though)
> Should work for 1, I guess?  Not sure I actually tried it.  But note we only support a very limited set of operations.  See also D105591.
> 
> For `<vscale x 6 x i8>`, we can use a different sequence: generate extloads for `<vscale x 2 x i64>` and `<vscale x 4 x i32>`, and merge them together.  GenWidenVectorLoads should handle that in theory; not sure how well it works in practice.
> 
> I'll add more testcases, in any case.
Is the FIXME still relevant?

> For <vscale x 6 x i8>, we can use a different sequence: generate extloads for <vscale x 2 x i64> and <vscale x 4 x i32>, and merge them together. GenWidenVectorLoads should handle that in theory; not sure how well it works in practice.
> I'll add more testcases, in any case.
Nice. From what I can see from the test you've added, <vscale x 6 x i16> is handled correctly by GenWidenVectorLoads.


================
Comment at: llvm/test/CodeGen/AArch64/sve-split-load.ll:72
+
+define <vscale x 4 x i32> @load_widen_3i32(<vscale x 3 x i32>* %a) {
+; CHECK-LABEL: load_widen_3i32:
----------------
`sve-split-load.ll` may no longer be the best name for this file given that these new tests are about widening?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106028