[PATCH] D64495: [AArch64] Implement __jcvt intrinsic from Armv8.3-A

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:237
+    break;
+  case llvm::AArch64::ArchKind::ARMV8_4A:
+    getTargetDefinesARMV84A(Opts, Builder);
----------------
It is a good change, but I think you should either add tests for these cases, or remove this (temporarily) because it is not adding much at the moment.


================
Comment at: clang/test/CodeGen/arm_acle.c:829
+
+// AArch64-v8_3: call i32 @llvm.aarch64.fjcvtzs
+#ifdef __ARM_64BIT_STATE
----------------
Same comment, better is to do a CHECK-LABEL first


================
Comment at: clang/test/CodeGen/builtins-arm64.c:61
 
+int32_t jcvt(double v) {
+  //CHECK: call i32 @llvm.aarch64.fjcvtzs
----------------
Although this file doesn't seem to do this, better is check the function name first, e.g.:

  // CHECK-LABEL: @jcvt(

followed by what you want to check:
  
  //CHECK: call i32 @llvm.aarch64.fjcvtzs


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64495