[PATCH] D76062: [PATCH] [ARM] ARMv8.6-a command-line + BFloat16 Asm Support

Sjoerd Meijer via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Mar 19 05:21:57 PDT 2020


SjoerdMeijer added a comment.

Besides the irrelevant formatting nits, one minor question about the clang test.



================
Comment at: clang/test/Driver/aarch64-cpus.c:622
+
+// The BFloat16 extension is a mandatory component of the Armv8.6-A extensions, but is permitted as an
+// optional feature for any implementation of Armv8.2-A to Armv8.5-A (inclusive)
----------------
Do we need 2 additional tests here?
- one for v8.6,
- and another with SVE?




================
Comment at: llvm/lib/Target/AArch64/AArch64InstrFormats.td:7806
+  def v4f16 : BaseSIMDThreeSameVectorBFDot<0, U, asm, ".2s", ".4h", V64,
+                                         v2f32, v8i8>;
+  def v8f16 : BaseSIMDThreeSameVectorBFDot<1, U, asm, ".4s", ".8h", V128,
----------------
nit: indentation is a bit off here


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrFormats.td:7808
+  def v8f16 : BaseSIMDThreeSameVectorBFDot<1, U, asm, ".4s", ".8h", V128,
+                                         v4f32, v16i8>;
+}
----------------
here too


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrFormats.td:7812
+class BaseSIMDThreeSameVectorBF16DotI<bit Q, bit U, string asm,
+                                          string dst_kind, string lhs_kind,
+                                          string rhs_kind,
----------------
and here


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrFormats.td:7820
+                        asm, "", dst_kind, lhs_kind, rhs_kind,
+        []> {
+
----------------
and this can be on the same line as above?


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrFormats.td:7867
+                                V128, asm, ".4s",
+                          []> {
+  let AsmString = !strconcat(asm, "{\t$Rd", ".4s", ", $Rn", ".8h",
----------------
and perhaps this one. But looks intentional, perhaps it's fine then, I don't know.


================
Comment at: llvm/lib/Target/ARM/ARMInstrNEON.td:8936
+           N3RegFrm, IIC_VDOTPROD, "", "", []>
+{
+
----------------
on the same line as above?


================
Comment at: llvm/lib/Target/ARM/ARMInstrNEON.td:8937
+{
+
+    let hasNoSchedulingInfo = 1;
----------------
no newline?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76062





More information about the cfe-commits mailing list