[PATCH] D159191: [DAGCombiner][SVE] Add support for illegal extending masked loads

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added a comment.

Is there value in adding a single fixed length test?



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:5353
+  if (auto *Ld = dyn_cast<MaskedLoadSDNode>(ExtVal->getOperand(0))) {
+    if (!isLoadExtLegalOrCustom(ISD::ZEXTLOAD, ExtVT, Ld->getValueType(0))) {
+      unsigned NumExtMaskedLoads = 0;
----------------
Is this strictly necessary? Perhaps it is but I'm wondering if you're really just trying to limit this combine to cases where ExtVT is bigger than legal?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:5356
+      for (auto *U : Ld->getMask()->uses())
+        NumExtMaskedLoads += isa<MaskedLoadSDNode>(U) ? 1 : 0;
+
----------------
Up to you but personally I think a straight forward:
```
if (isa<MaskedLoadSDNode>(U))
  ++NumExtMaskedLoads;
```
is cleaner.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:5363
+
+  return ExtVT.isScalableVector() || Subtarget->useSVEForFixedLengthVectors();
 }
----------------
I think this check probably want to be first so that we only start processing ExtVal when we know extending loads are a possibility.


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

https://reviews.llvm.org/D159191