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

[Diogo N. Sampaio via Phabricator via cfe-commits]

dnsampaio abandoned this revision.
dnsampaio added a comment.

Fixed.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60828



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Diogo N. Sampaio via Phabricator via cfe-commits]

dnsampaio planned changes to this revision.
dnsampaio added a comment.

Waiting for the outcome of D60691 .


Repository:
  rC Clang

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

https://reviews.llvm.org/D60828



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Diogo N. Sampaio via Phabricator via cfe-commits]

dnsampaio marked 5 inline comments as done.
dnsampaio 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;

ostannard wrote:
> 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.
If it is V8 and does not have double-precision, isn't that going to use the 
argument `+fp-only-sp` ? That will disable WH_FP_DP  using lines 436 and 456.
```
else if (Feature == "+fp-only-sp") {
HW_FP_remove |= HW_FP_DP;
```
```
HW_FP &= ~HW_FP_remove;
```




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") {

ostannard wrote:
> Should this be FPARMV8, since fullfp16 doesn't apply to earlier 
> architectures? Maybe MVE  complicates this even further?
Correct, it should also add FeatureFPARMv8.
See that this is a accumulative flag, as FPARMv8 implies VFP4FPU, both should 
be set.
 Actually, according the backend, `FeatureFPARMv8 -> FeatureVFP4 -> FeatureVFP3 
-> FeatureVFP2`. From my tests we already set FeatureVFP3, but not FeatureVFP4



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

ostannard wrote:
> Should this also add FPARMV8?
As well, yes.



Comment at: lib/Basic/Targets/ARM.cpp:452
+  HasLegalHalfType = true;
+  FPU |= VFP4FPU;
+  HW_FP |= HW_FP_SP | HW_FP_DP | HW_FP_HP;

ostannard wrote:
> Again, should this be FPARMV8?
True.



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
+

ostannard wrote:
> 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.
That indeed seems a better solution.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60828



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Diogo N. Sampaio via Phabricator via cfe-commits]

dnsampaio created this revision.
dnsampaio added reviewers: ostannard, DavidSpickett.
Herald added subscribers: cfe-commits, kristof.beyls, javed.absar.
Herald added a project: clang.

This patch adds the fp16fml feature parser as well fixes
the FPU and the HW_FP flags when +fullfp16 and +dotprod 
features are passed, to account for pre-requisite features.

The ARM backend (ARM.td) defines this tree of feature dependencies:

   fp16  vfp3
 |  /   |
vfp4neon
 |  \
  fp-armv8   FeatureDotProd
 |
  fullfp16
 |
  fp16fml

However, clang does not capture that when using +fullfp16 we
also have vfp4, so compiling
tools/clang/test/CodeGen/arm_neon_intrinsics.c

with

  clang -target armv8a-linux-eabi -march=armv8.4-a+fp16 -S -emit-llvm

will give an error because vfp4 is not added to the FPU flag.

As test now we can compile that test file with the command

  clang -target armv8a-linux-eabi -march=armv8-a+fp16fml -S -emit-llvm


Repository:
  rC Clang

https://reviews.llvm.org/D60828

Files:
  lib/Basic/Targets/ARM.cpp
  test/CodeGen/arm_neon_intrinsics.c
  test/CodeGen/arm_neon_intrinsics.c.v8



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits