[PATCH] D77590: [ARM] MVE saturating truncates

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

About the tests: if I haven't missed anything, I don't see any VQMOVNT variants being generated?



================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:14758
+    return SDValue();
+
+  auto IsSignedSaturate = [&](SDNode *Min, SDNode *Max) {
----------------
Can we do an early exit here?

  if (VT != MVT::v4i32 || VT != MVT::v8i16)
    return SDValue();

I think that then also saves a few else-clauses in the different if-elseif-else statements below.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:14796
+
+    // Create a VQMONVB, Signed extended in to the top half. That extend will
+    // hopefully be removed if only the bottom bits are demanded (though a
----------------
Typo? `VQMONVB` -> `VQMOVB`?

Also: top half -> bottom half?


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:14840
+
+    // Create a VQMONVB, ZExt in to the top half with an AND. That extend will
+    // hopefully be removed if only the bottom bits are demanded (though a
----------------
now starting to doubt if I read the ARMARM correct, but same typo here?


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.h:205
 
+      // MVE Saturating truncates
+      VQMOVNs,
----------------
nit: perhaps a comment explaining more what the opcode is:

  // Vector (V) Saturating (Q) Move and Narrow (N),  signed/unsigned (s/u)


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

https://reviews.llvm.org/D77590