[PATCH] D147533: [AArch64][CodeGen][SME] Allow vectors large than hardware support to use SVE's load zero/sign-extend for fixed vectors

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:5223
 bool AArch64TargetLowering::isVectorLoadExtDesirable(SDValue ExtVal) const {
+  bool OverrideNEON = Subtarget->useSVEForFixedLengthVectors();
   return ExtVal.getValueType().isScalableVector() ||
----------------
Hi @dtemirbulatov, I personally feel this patch should be only trying to fix one thing at a time, but it seems to be trying to change both SVE fixed-length behaviour as well as streaming SVE behaviour. Perhaps it's better to focus on just the streaming SVE case for now, in which case I think you can more simply write this as:

  bool OverrideNEON = Subtarget->useSVEForFixedLengthVectors() || Subtarget->forceStreamingCompatibleSVE();
  return ExtVal.getValueType().isScalableVector() ||
         useSVEForFixedLengthVectorVT(
             ExtVal.getValueType(), OverrideNEON);


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:5227
+         (OverrideNEON && ExtVal.getValueType().getFixedSizeInBits() >
+                              Subtarget->getMinSVEVectorSizeInBits());
 }
----------------
Just for reference, I'm not sure if this is really what you want for the SVE fixed-length case (no streaming SVE) because `getMinSVEVectorSizeInBits()` could return 256 or greater. In this case it has nothing to do with NEON-sized vectors, which are 64-bit or 128-bit. In any case, I feel like the normal SVE fixed-length case should be done in a different patch.


================
Comment at: llvm/test/CodeGen/AArch64/sve-streaming-mode-fixed-length-int-mulh.ll:171
 ; CHECK:       // %bb.0:
-; CHECK-NEXT:    ldp q0, q1, [x0]
+; CHECK-NEXT:    mov x8, #4 // =0x4
+; CHECK-NEXT:    mov x9, #8 // =0x8
----------------
Hi @dtemirbulatov, this looks like a regression to me. We've gone from 20 instructions to 29, which doesn't seem right. Can you investigate why this is happening?


================
Comment at: llvm/test/CodeGen/AArch64/sve-streaming-mode-fixed-length-int-mulh.ll:248
 ; CHECK:       // %bb.0:
-; CHECK-NEXT:    ldp q0, q1, [x0]
+; CHECK-NEXT:    mov x8, #2 // =0x2
+; CHECK-NEXT:    mov x9, #4 // =0x4
----------------
Again, this has gotten worse - from 20 -> 29 instructions.


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

https://reviews.llvm.org/D147533