[PATCH] D137321: [AArch64][SVE2] Add the SVE2.1 BF16 instructions

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64.td:166
 def FeatureSVE2p1: SubtargetFeature<"sve2p1", "HasSVE2p1", "true",
   "Enable Scalable Vector Extension 2.1 instructions", [FeatureSVE2]>;
 
----------------
unrelated to this patch but I noticed this is missing `(FEAT_SVE2p1)` like the other feature descriptions


================
Comment at: llvm/lib/Target/AArch64/AArch64.td:169
+def FeatureB16B16 : SubtargetFeature<"b16b16", "HasB16B16", "true",
+  "Enable SVE2.1 or SME2.1 non-widening BFloat16 instructions (FEAT_B16B16)", []>;
+
----------------
BFloat16 to BFloat16


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:3701
+//===----------------------------------------------------------------------===//
+// SVE2.1 instructions
+//===----------------------------------------------------------------------===//
----------------
SVE2.1 or SME2.1 non-widening BFloat16 to BFloat16 instructions?


================
Comment at: llvm/lib/Target/AArch64/AArch64SchedNeoverseN2.td:22
+  list<Predicate> UnsupportedFeatures = !listconcat(SMEUnsupported.F,
+                                                    SVEUnsupported.F);
 }
----------------
https://reviews.llvm.org/D137245#inline-1324912 applies here as well


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:2196
 
+multiclass sve_fp_bfma_by_indexed_elem<string asm, bit opc> {
+  def NAME : sve_fp_fma_by_indexed_elem<{0, ?}, 0b1, opc, asm, ZPR16, ZPR3b16,
----------------
single def so this should be a class


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:2196
 
+multiclass sve_fp_bfma_by_indexed_elem<string asm, bit opc> {
+  def NAME : sve_fp_fma_by_indexed_elem<{0, ?}, 0b1, opc, asm, ZPR16, ZPR3b16,
----------------
c-rhodes wrote:
> single def so this should be a class
`sve2p1` like the class below?


================
Comment at: llvm/lib/Target/AArch64/SVEInstrFormats.td:2258
 
+multiclass sve2p1_fp_bfmul_by_indexed_elem<string asm> {
+  def NAME : sve_fp_fmul_by_indexed_elem<{0, ?}, 0b1, asm, ZPR16, ZPR3b16, VectorIndexH32b> {
----------------
single def so this should be a class


================
Comment at: llvm/test/MC/AArch64/SVE2p1/bfadd-diagnostics.s:6
+
+movprfx z23.h, p8/m, z31.h
+bfadd z23.h, p8/m, z23.h, z13.h
----------------
movprfx is only necessary for the last test


================
Comment at: llvm/test/MC/AArch64/SVE2p1/bfadd-diagnostics.s:13
+movprfx z23.h, p8/m, z31.h
+bfadd z23.h, p8/z, z23.h, z13.h
+// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: invalid restricted predicate register, expected p0..p7 (without element suffix)
----------------
only merging predication can be specified? This test is no different from the above, I'd either remove it or change it so the predicate register is valid but with zeroing prediction.


================
Comment at: llvm/test/MC/AArch64/SVE2p1/bfadd-diagnostics.s:22
+movprfx z23.h, p1/m, z31.h
+bfadd z23.h, p1/z, z23.s, z13.s
+// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: invalid operand for instruction
----------------
m


================
Comment at: llvm/test/MC/AArch64/SVE2p1/bfadd-diagnostics.s:27-34
+movprfx z23.h, p1/m, z31.h
+// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: instruction is unpredictable when following a movprfx, suggest replacing movprfx with mov
+// CHECK-NEXT: movprfx z23.h, p1/m, z31.h
+// CHECK-NOT: [[@LINE-1]]:{{[0-9]+}}:
+bfadd z23.s, p1/z, z23.h, z13.h
+// CHECK: [[@LINE-1]]:{{[0-9]+}}: error: invalid element width
+// CHECK-NEXT: bfadd z23.s, p1/z, z23.h, z13.h
----------------
not sure what's going on here, it's the unpredicated form that can't be preceded by movprfx?


================
Comment at: llvm/test/MC/AArch64/SVE2p1/bfmax-diagnostics.s:6
+
+movprfx z23.h, p8/m, z31.h
+bfmax z23.h, p8/m, z23.h, z13.h
----------------
same comments as bfadd-diagnostics.s apply here


================
Comment at: llvm/test/MC/AArch64/SVE2p1/bfmaxnm-diagnostics.s:6
+
+movprfx z23.h, p8/m, z31.h
+bfmaxnm z23.h, p8/m, z23.h, z13.h
----------------
same comments as bfadd-diagnostics.s apply here


================
Comment at: llvm/test/MC/AArch64/SVE2p1/bfmin-diagnostics.s:6
+
+movprfx z23.h, p8/m, z31.h
+bfmin z23.h, p8/m, z23.h, z13.h
----------------
same comments as bfadd-diagnostics.s apply here


================
Comment at: llvm/test/MC/AArch64/SVE2p1/bfminnm-diagnostics.s:6
+
+movprfx z23.h, p8/m, z31.h
+bfminnm z23.h, p8/m, z23.h, z13.h
----------------
same comments as bfadd-diagnostics.s apply here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137321