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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:420-421
+    addRegisterClass(MVT::aarch64svcount, &AArch64::PPRRegClass);
+    setOperationAction(ISD::LOAD, MVT::aarch64svcount, Custom);
+    setOperationAction(ISD::STORE, MVT::aarch64svcount, Custom);
+    setOperationAction(ISD::SELECT, MVT::aarch64svcount, Custom);
----------------
I've tried and now there is isel for `ISD::BITCAST` this can be
```
setOperationPromotedToType(ISD::LOAD, MVT::aarch64svcount, MVT::nxv16i1);
setOperationPromotedToType(ISD::STORE, MVT::aarch64svcount, MVT::nxv16i1);
```


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:422-423
+    setOperationAction(ISD::STORE, MVT::aarch64svcount, Custom);
+    setOperationAction(ISD::SELECT, MVT::aarch64svcount, Custom);
+    setOperationAction(ISD::SELECT_CC, MVT::aarch64svcount, Expand);
+  }
----------------
Do code generation tests for these exist?


================
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)> {
----------------
sdesmalen wrote:
> 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.
Thanks for the investigation. Sure, having this as a follow on patch works for me.


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:2537
+
+    def : Pat<(nxv16i1 (bitconvert (aarch64svcount PNR:$src))), (nxv16i1 PPR:$src)>;
+    def : Pat<(aarch64svcount (bitconvert (nxv16i1 PPR:$src))), (aarch64svcount PNR:$src)>;
----------------
Does `PNR` exist? or perhaps it's a typo? When experimenting with `setOperationPromotedToType` I hit build errors and had to change these entries.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136862