[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:15073-15075
+  // (ty1 extract_subvector (ty2 ptrue) 0) -> (ty1 ptrue)
+  if (V.getOpcode() == AArch64ISD::PTRUE && N->getConstantOperandVal(1) == 0)
+    return getPTrue(DAG, DL, VT, V.getConstantOperandVal(0));
----------------
Is this universally true across all predicate patterns?  Given my comment below I'm wondering if you'll eventually just create fresh `PTRUE`s rather than have to answer this question.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16849-16851
+  // If this is a masked load followed by an UUNPKLO, fold this into a masked
+  // extending load.  We can do this even if this is already a masked
+  // {z,}extload.
----------------
This function also accepts `AArch64ISD::UUNPKHI` so you'll need to protect against that.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16866
+          MLD->getMemOperand(), MLD->getAddressingMode(), ISD::ZEXTLOAD,
+          /*IsExpanding=*/true);
+
----------------
I think you've confused extending with explanding? I don't think this is something we support for AArch64, which explains why most code in this file just omits this parameter.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17219-17225
+  if ((Value.getOpcode() == AArch64ISD::UZP1) && Value->hasOneUse() &&
+      Value.getOperand(0) == Value.getOperand(1) && MST->isUnindexed()) {
+    Value = Value.getOperand(0);
+    if (Value.getOpcode() == ISD::BITCAST) {
+      Mask = DAG.getNode(
+          ISD::EXTRACT_SUBVECTOR, DL,
+          Value.getOperand(0).getValueType().changeVectorElementType(MVT::i1),
----------------
Not quite sure but this feels too easy. What if you actually want to store the result of an `UZP1`?

I think the current value of the predicate plays a key role here.  The case you're trying to handle is when we're storing N elements where all those elements come from one side of an `UZIP1`.

Not sure if the loads have the same issue but I figure it's safest for the two to have symmetry.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128065