[PATCH] D136862: [AArch64][SME2] Add CodeGen support for target("aarch64.svcount").

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks for the review @paulwalker-arm, I think I've addressed most of your comments, with the exception of giving predicate-as-counter its own register class, as that one probably makes more sense to pull out into a separate patch.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:17556-17557
 
+  if (N->getValueType(0).isScalableTargetExtVT())
+    return SDValue();
+
----------------
paulwalker-arm wrote:
> What is it about this combine that's problematic? Can you add a code comment for the rational.
> 
> Is it specially related to scalable target types or just target types in general?
This bailout was a bit too rigorous and I've removed it in favour of more specific bailouts.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:9281-9293
+    TVal = DAG.getNode(AArch64ISD::REINTERPRET_CAST, DL, MVT::nxv16i1, TVal);
+    FVal = DAG.getNode(AArch64ISD::REINTERPRET_CAST, DL, MVT::nxv16i1, FVal);
+    EVT CCVT = CCVal.getValueType();
+    SDValue ID =
+        DAG.getTargetConstant(Intrinsic::aarch64_sve_whilelo, DL, CCVT);
+    SDValue Zero = DAG.getConstant(0, DL, CCVT);
+    SDValue SplatVal = DAG.getNode(ISD::SIGN_EXTEND_INREG, DL, CCVT, CCVal,
----------------
paulwalker-arm wrote:
> Can this be simplified to:
> ```
> REINTERPRET_CAST(SELECT(CCVal, REINTERPRET_CAST(TVAL), REINTERPRET_CAST(FVAL)))
> ```
> Then we'll renter this function and reuse the existing predicate lowering code. Perhaps you tried this and it didn't work out?
Thanks for the suggestion, using SELECT worked fine.


================
Comment at: llvm/lib/Target/AArch64/AArch64RegisterInfo.td:894
                                   "AArch64",
-                                  [ nxv16i1, nxv8i1, nxv4i1, nxv2i1, nxv1i1 ], 16,
+                                  [ nxv16i1, nxv8i1, nxv4i1, nxv2i1, nxv1i1, aarch64svcount ], 16,
                                   (sequence "P%u", firstreg, lastreg)> {
----------------
paulwalker-arm wrote:
> Would it be overkill for `aarch64svcount` to have its own register class? I'm thinking it'll be nice to have isel protect us from incorrectly mixing predicate types and instructions. I'm also not sure if things like `SDTCisSameNumEltsAs` will cause is trouble, although I guess you've not hit anything so far.
> 
> The sequence below is using the non-count naming. Does this matter?
I tried to give aarch64svcount it's own register class, but this requires changes (to add a new register class, etc) that complicate this patch quite a bit. I can follow this up in a separate patch.

> The sequence below is using the non-count naming. Does this matter?
No that's fine, because it uses the same P registers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136862