[PATCH] D128065: [AArch64][SVE] Fold target specific ext/trunc nodes into loads/stores

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17047
+      unsigned PgPattern = Mask->getConstantOperandVal(0);
+      EVT InVT = N->getValueType(0);
+
----------------
Is this not just VT? It's not related to any input from what I can see, or have a misunderstood the naming?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17051
+      if (PgPattern != AArch64SVEPredPattern::all &&
+          getNumElementsFromSVEPredPattern(PgPattern) *
+                  InVT.getVectorElementType().getSizeInBits() <=
----------------
`getNumElementsFromSVEPredPattern` can return 0 for cases other than `all`.  Perhaps
```
unsigned NumElts = getNumElementsFromSVEPredPattern(PgPattern);
if (NumElts && NumElts * InVT.getVectorElementType().getSizeInBits() <= MinSVESize)
```


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17058
+            N->getValueType(0), DL, MLD->getChain(), MLD->getBasePtr(),
+            MLD->getOffset(), Mask, MLD->getPassThru(), MLD->getMemoryVT(),
+            MLD->getMemOperand(), MLD->getAddressingMode(), ISD::ZEXTLOAD);
----------------
Is this correct? Given the new load has a different result type, I'd expect `PassThru` to also change.  Speaking of which, I think the combine is only equivalent when the original `PassThru` is zero or undef and the new one is forced to zero?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128065