[PATCH] D60828: [ARM] Fix armv8 features tree and add fp16fml

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a reviewer: simon_tatham.
ostannard added inline comments.


================
Comment at: lib/Basic/Targets/ARM.cpp:443
       HasLegalHalfType = true;
+      HW_FP |= HW_FP_SP | HW_FP_DP | HW_FP_HP;
+      FPU |= VFP4FPU;
----------------
Is it always correct to set HW_FP_DP here, now that MVE can have full fp16 without double-precision? I'll add Simon since he's working on that.


================
Comment at: lib/Basic/Targets/ARM.cpp:444
+      HW_FP |= HW_FP_SP | HW_FP_DP | HW_FP_HP;
+      FPU |= VFP4FPU;
     } else if (Feature == "+dotprod") {
----------------
Should this be FPARMV8, since fullfp16 doesn't apply to earlier architectures? Maybe MVE  complicates this even further?


================
Comment at: lib/Basic/Targets/ARM.cpp:446
     } else if (Feature == "+dotprod") {
+      FPU |= NeonFPU;
+      HW_FP |= HW_FP_SP | HW_FP_DP;
----------------
Should this also add FPARMV8?


================
Comment at: lib/Basic/Targets/ARM.cpp:452
+      HasLegalHalfType = true;
+      FPU |= VFP4FPU;
+      HW_FP |= HW_FP_SP | HW_FP_DP | HW_FP_HP;
----------------
Again, should this be FPARMV8?


================
Comment at: lib/Basic/Targets/ARM.cpp:453
+      FPU |= VFP4FPU;
+      HW_FP |= HW_FP_SP | HW_FP_DP | HW_FP_HP;
     }
----------------
You are adding HW_FP_HP twice.


================
Comment at: test/CodeGen/arm_neon_intrinsics.c:8
+// RUN: %clang -O1 -target armv8a-linux-eabi -march=armv8a+fp16fml\
+// RUN:  -S -emit-llvm -o - %s | FileCheck %s.v8
+
----------------
Does the generate code differ enough to have a second copy of it? Actually, I assume the problem here is that we are not setting the correct preprocessor macros? in which case, it would be better to test them directly, than by re-running this 21kloc test.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60828