[PATCH] D82448: [AArch64][SVE] Add bfloat16 support to store intrinsics

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli requested changes to this revision.
fpetrogalli added a comment.
This revision now requires changes to proceed.

Thank you for updating the patch with the missing tests. I only have one request for the code involving assertions, and the use of `let Predicates = ...`.

Francesco



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:12131-12132
 
+  const bool hasBF16 =
+    static_cast<const AArch64Subtarget &>(DAG.getSubtarget()).hasBF16();
+
----------------
I think there is no need to set up a variable here, you can fold this directly in the assertion. Same for the similar change below.


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:1567-1569
+  defm : pred_store<nxv8i16,  nxv8i1, trunc_masked_store_i8, ST1B_H, ST1B_H_IMM, am_sve_regreg_lsl0>;
+  defm : pred_store<nxv8i16,  nxv8i1, nontrunc_masked_store, ST1H,   ST1H_IMM,   am_sve_regreg_lsl1>;
+  defm : pred_store<nxv8f16,  nxv8i1, nontrunc_masked_store, ST1H,   ST1H_IMM,   am_sve_regreg_lsl1>;
----------------
nit: I think this change is not necessary, but if you really want to do it, you should probably move the 4th column, not the second one. 


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:1571
+
+  let Predicates = [HasBF16] in {
+    defm : pred_store<nxv8bf16, nxv8i1, nontrunc_masked_store, ST1H,   ST1H_IMM,   am_sve_regreg_lsl1>;
----------------
Doesn't this also need `HasSVE`?


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

https://reviews.llvm.org/D82448