[PATCH] D135564: [AArch64-SVE]: Force generating code compatible to streaming mode.

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

Hi @hassnaa-arm, I think it's almost there! Most of the tests look good to me. I just had a few minor comments ...



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11422
+                   const APInt &Bits, const SDValue *LHS = nullptr,
+                   const AArch64Subtarget *const Subtarget = nullptr) {
+  EVT VT = Op.getValueType();
----------------
I don't think you need to pass in the `Subtarget` here. In the code below you can just do

  if (VT.isFixedLengthVector() &&
      DAG.getSubtarget<AArch64Subtarget>().forceStreamingCompatibleSVE()
    return SDValue();


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11473
+static SDValue
+tryAdvSIMDModImm16(unsigned NewOp, SDValue Op, SelectionDAG &DAG,
+                   const APInt &Bits, const SDValue *LHS = nullptr,
----------------
Same comment as above for `tryAdvSIMDModImm32`


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14048
                                                   unsigned Factor) const {
+  // Skip if streaming compatible SVE is enabled,
+  // because it generates invalid code in streaming mode when SVE length is not
----------------
nit: The comment can probably be formatted better I think so that you use up 80 chars, i.e.:

  // Skip if streaming compatible SVE is enabled, because it generates invalid
  // code in streaming mode when SVE length is not specified.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:15819
+                                 TargetLowering::DAGCombinerInfo &DCI,
+                                 const AArch64Subtarget *const Subtarget) {
   SelectionDAG &DAG = DCI.DAG;
----------------
Again, you can avoid passing in the subtarget here if you make the changes to `tryAdvSIMDModImm32` and `tryAdvSIMDModImm16`.


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:3044
             (i64 (UMOVvi64 (v2i64 (EXTRACT_SUBREG ZPR:$vec, zsub)), VectorIndexD:$index))>;
   }
+  let Predicates = [NotInStreamingSVEMode] in {
----------------
When we guard something by a predicate we normally add a comment on the final brace '}' to make it easy to see, i.e. something like:

  } // End NotInStreamingSVEMode


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:3058
             (i64 (SMOVvi32to64 (v4i32 (EXTRACT_SUBREG ZPR:$vec, zsub)), VectorIndexS:$index))>;
-
+  }
   // Extract first element from vector.
----------------
  } // End NotInStreamingSVEMode


================
Comment at: llvm/test/CodeGen/AArch64/sve-streaming-mode-fixed-length-concat.ll:10
+
+define <8 x i8> @concat_v8i8(<4 x i8> %op1, <4 x i8> %op2) vscale_range(2,0) #0 {
+; CHECK-LABEL: concat_v8i8:
----------------
I don't think we need to have `vscale_range(2,0)` on these tests, right? We want streaming SVE to work for vector lengths.


================
Comment at: llvm/test/CodeGen/AArch64/sve-streaming-mode-fixed-length-extract-vector-elt.ll:17
+; CHECK-NEXT:    ret
+    %r = extractelement <2 x half> %op1, i64 1
+    ret half %r
----------------
nit: I think in all these tests there should only be 2 spaces at the start of the IR, i.e.

`  %r = extractelement <2 x half> %op1, i64 1`

etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135564