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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
