[PATCH] D79587: [CodeGen][SVE] Legalisation of extends with scalable types

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:4337
+      // Collect the (promoted) operands
+      SDValue Ops[] = { GetPromotedInteger(InOp0), BaseIdx };
+
----------------
In general, there are four possibilities for legalization of the operand vector:

1. Legal.  In this case, I guess the operation you want is essentially ANY_EXTEND_VECTOR_INREG.  You're currently handling this in target-specific code; I guess that's okay for now.
2. Promote. In this case, you can promote both the operand and the result, what you're doing here.
3. Widen.  In this case, you just widen the operand.
4. Split.  In this case, you pick the correct half (or construct it with a shuffle), and extract from that.

-----

Currently, you're assuming the the result of GetPromotedInteger() has an element type narrower or equal to the element type of NOutVT.  I'm not sure that's true on all targets, but I guess it's true for SVE.  Better to explicitly check, I think.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:4344
+    }
+  }
+
----------------
Please put a report_fatal_error here for scalable vectors, so we don't end up with some obscure invalid BUILD_VECTOR error.

Alternatively, I guess you could call ExpandExtractFromVectorThroughStack().


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:906
+        if (VT.getVectorElementType() != MVT::i1)
+          setOperationAction(ISD::EXTRACT_SUBVECTOR, VT, Custom);
       }
----------------
Can you restrict this to specifically the types you're interested in handling?  It looks like you only implemented handling for the following types: nxv8i8, nxv4i16, nxv2i32.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:8559
+  if (Op.getValueType().isScalableVector())
+    return SDValue();
+
----------------
I think you can just assert the type isn't scalable?  It shouldn't be possible to get here.  You're only marking EXTRACT_SUBVECTOR Custom for illegal types.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:10676
+    ConstantSDNode *C = dyn_cast<ConstantSDNode>(Dup->getOperand(0));
+    uint64_t ExtVal = C->getZExtValue();
+
----------------
Do you need to truncate ExtVal somewhere, so you don't end up with a DUP with an over-wide constant?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:13300
+    // is another unpack:
+    // 4i32 sign_extend_inreg (4i32 uunpklo(8i16 uunpklo (16i8 opnd)), from 4i8)
+    // ->
----------------
It seems a little fragile to assume the inner VT of the SIGN_EXTEND_INREG is exactly the type you're expecting here.  Probably worth at least adding an assertion to encode the assumptions you're making.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:13816
+  if (!isTypeLegal(InVT)) {
+    // Bubble truncates to illegal types to the surface.
+    if (In->getOpcode() == ISD::TRUNCATE) {
----------------
"Bubble truncates to illegal types to the surface" is an optimization?


================
Comment at: llvm/test/CodeGen/AArch64/sve-sext-zext.ll:195
+; CHECK-DAG: sunpklo z2.h, z0.b
+; CHECK-DAG: sunpkhi z1.h, z0.b
+; CHECK-DAG: mov z0.d, z2.d
----------------
Please just generate the checks with update_llc_test_checks.py.


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

https://reviews.llvm.org/D79587