[PATCH] D133433: [AArch64]: Force generating code compatible to streaming mode

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:1398
+    if (Subtarget->forceStreamingCompatibleSVE()) {
+      for (MVT VT : {MVT::v8i8, MVT::v16i8, MVT::v4i16, MVT::v8i16, MVT::v2i32,
+                     MVT::v4i32, MVT::v1i64, MVT::v2i64})
----------------
I don't see tests for this and some of the other MVTs.  Do we need explicit handling for `MVT::v1i64` and `MVT::v1f64`? I would have thought these would just emit a scalar access, although there's no tests to show this.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:1400
+                     MVT::v4i32, MVT::v1i64, MVT::v2i64})
+        if (useSVEForFixedLengthVectorVT(VT, true))
+          addTypeForStreamingSVE(VT);
----------------
Is this check (plus the one for the float loop) necessary? I would expect that when `forceStreamingCompatibleSVE()` returns true we have no choice but to enable the custom lowering.


================
Comment at: llvm/lib/Target/AArch64/AArch64Subtarget.cpp:440-443
+  if (ForceStreamingCompatibleSVE) {
+    assert(hasSVE() && "Expected SVE to be available");
+    return hasSVE();
+  }
----------------
```
if (forceStreamingCompatibleSVE())
  return true;
```
Doing this might mean `useSVEForFixedLengthVectors` can remain defined in the header file.


================
Comment at: llvm/lib/Target/AArch64/AArch64Subtarget.cpp:452
+    assert(hasSVE() && "Expected SVE to be available");
+    return hasSVE();
+  }
----------------
Should this also include `|| hasSME()`?


================
Comment at: llvm/test/CodeGen/AArch64/sve-streaming-mode-fixed-length-stores.ll:26
+; CHECK-NEXT:    ld1b { z0.b }, p0/z, [x8]
+; CHECK-NEXT:    str q0, [x0]
+; CHECK-NEXT:    ret
----------------
The patch summary mentions lowering stores and you've added this test but I don't see any code to enable such lowering and hence we are seeing NEON str instructions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133433