[PATCH] D79710: [clang][BFloat] add create/set/get/dup intrinsics

[Dave Green via Phabricator via cfe-commits]

dmgreen added inline comments.


================
Comment at: clang/include/clang/Basic/arm_neon.td:1860
+
+  def VGET_HIGH_BF : NoTestOpInst<"vget_high", ".Q", "b", OP_HI>;
+  def VGET_LOW_BF  : NoTestOpInst<"vget_low", ".Q", "b", OP_LO>;
----------------
Do you know what InstName = "vmov" does, and is it needed here?

I'm pretty sure it's a vestige of an earlier implementation of the neon emitter.


================
Comment at: clang/include/clang/Basic/arm_neon.td:1867
+  def SCALAR_VDUP_LANE_BF : IInst<"vdup_lane", "1.I", "Sb">;
+  def SCALAR_VDUP_LANEQ_BF : IInst<"vdup_laneq", "1QI", "Sb">;
+}
----------------
Does this need let isLaneQ = 1, like the other vdup_laneq's?


================
Comment at: clang/include/clang/Basic/arm_neon_incl.td:293
+
+  string CartesianProductWith = "";
 }
----------------
Is this needed in this patch?


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:6309
+  case NEON::BI__builtin_neon_vget_lane_bf16:
+  case NEON::BI__builtin_neon_vduph_lane_bf16:
   case NEON::BI__builtin_neon_vgetq_lane_i8:
----------------
How come these are needed for vduph_lane_bf16 if they were not needed for vduph_lane_f16?


================
Comment at: clang/test/CodeGen/aarch64-bf16-getset-intrinsics.c:2
+// RUN: %clang_cc1 -triple aarch64-arm-none-eabi -target-feature +neon -target-feature +bf16 \
+// RUN:  -O2 -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK64
+// RUN: %clang_cc1 -triple armv8.6a-arm-none-eabi -target-feature +neon -target-feature +bf16 -mfloat-abi hard \
----------------
It's best to have auto generated tests if we can, and ideally not rely on running the entire -O2 pipeline. I think a lot of the other tests use clang ... -disable-O0-optnone | opt -S -mem2reg | Filecheck ...

Also this aarch64 tests is running arm codegen too? Not sure if that matters, but I don't immediately see it being done elsewhere.


================
Comment at: clang/test/CodeGen/aarch64-bf16-getset-intrinsics.c:19-20
+// CHECK-LABEL: test_vdup_n_bf16
+// CHECK64: %vecinit.i = insertelement <4 x bfloat> undef, bfloat %v, i32 0
+// CHECK32: %vecinit.i = insertelement <4 x bfloat> undef, bfloat %v, i32 0
+// CHECK: %vecinit{{.*}} = shufflevector <4 x bfloat> %vecinit.i, <4 x bfloat> undef, <4 x i32> zeroinitializer
----------------
A lot of these 32 and 64 bit lines look the same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79710