[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

For (1) and (2) is there a need to be able to reset the architecture setting no 
matter the previous `march` and `` are?

Currently we're talking about *not* wanting having to use `-march=armv8-a+crc` 
but would you still want a way to reset the architecture no matter what the 
previous options are?

That said, applying the extra extensions to the last `-march` gives you a way 
to bump the base architecture which could be useful. `-march=armv8-a 
-add-extension=+crc -march=armv8.4-a` => armv8.4-a+crc.

For (3) I agree with your concern.

> There are a lot of supported ISA features, so there would be a lot of -m and 
> -mno- options to document. It would become harder to separate out -m options 
> related to architecture selection from -m options that do other things.

A rough count:

  $ clang --help | grep " \-m" | wc -l
  108

I also wouldn't want to get into a situation where we name an extension such 
that `-mextension` looks more like it's a general compiler option. E.g.

  -mnvj   Enable generation of new-value jumps

"nvj" isn't far off what our extensions end up being called.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113779

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

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[Richard Sandiford via Phabricator via cfe-commits]

rsandifo-arm added a comment.

I can see at least three ways of handling this:

1. @paulwalker-arm's suggestion to allow `-march=` without a base architecture 
(or with a dummy base architecture that means “no change”)
2. a single new option `-mfoo=` that applies on top of `-march` and can be used 
to add or remove architecture features
3. individual `-m` and `-mno-` options for each ISA feature.

I think all three achieve the same objective.

I assume for (1) that `-march=` options would apply in-order, so that an 
`-march=` without a base architecture would apply on top of any previous 
`-march=`, whereas an `-march=` with a base architecture would override all 
previous `-march=` options (both old-style and new-style).  This behaviour 
might be an advantage in some situations and a disadvantage in others.  Build 
systems would need to be careful about where in the command line the extra 
features go (but most probably are).

For (2) we could say that all `-mfoo=` options apply (cumulatively, in order) 
to the final `-march` option, even if the `-march` option comes later than some 
of the `-mfoo=` options.  This too could be an advantage in some situations and 
a disadvantage in others (as a reversal of the situation for (1)).

The architecture can already be set by two options rather than one: `-march=` 
and `-mcpu=`.  Currently (at least in GCC), `-march=` trumps the architecture 
effects of `-mcpu=`, even if `-mcpu=` is specified after `-march=`.  I suppose 
the difficulty for (1) is then deciding what should happen if an `-march=` 
without a base architecture is specified alongside an `-mcpu=`.  Does the 
`-march=` option apply to the `-mcpu=` choice of architecture even if the 
`-mcpu=` appears later?  If so, `-march=` options without a base architecture 
would sometimes have a cumulative effect with later (`-mcpu=`) options as well 
as earlier (`-march=` or `-mcpu=`) ones.  This might be non-intuitive and would 
be another way in which `-mcpu=` is not exactly equivalent to the associated 
`-march=` and `-mtune=` options.

So perhaps one advantage of (2) over (1) is that the semantics are easier to 
describe.  `-mfoo=` options apply cumulatively to the command line's final 
choice of base architecture, however that choice is specified.

Personally I'm not very keen on (3).  Reasons:

- (1) and (2) collect the (subtle) semantics about option precedence in a 
single place.  (3) distributes it among many individual options.
- There are a lot of supported ISA features, so there would be a lot of `-m` 
and `-mno-` options to document.  It would become harder to separate out `-m` 
options related to architecture selection from `-m` options that do other 
things.
- With (3) it becomes much harder to check that every supported feature has an 
associated option.  With (1) and (2) this would be guaranteed by construction.
- The `+feature` and `+nofeature` style is also used in things like `#pragma 
GCC target`, which is another mechanism for changing ISA features without 
changing the base architecture.  It feels like the new options are equivalent 
to sticking such pragmas at the start of the translation unit, so it would be 
good for them to use a consistent style.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113779

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

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

In D113779#3496589 , @fhahn wrote:

> In D113779#3207936 , @SjoerdMeijer 
> wrote:
>
>>> If anybody has contacts to GCC that would be very helpful. Unfortunately I 
>>> don't think I will be able to drive this.
>>
>> Ok, I will bring this up internally first with some folks that work on GCC 
>> and see what happens. To be continued...
>
> Hi, did you get an update by any chance?

Sorry for the delay. I have just left a message for our GNU toolchain guys with 
an invitation to add comments here.
I know there are strong opinions that -march should be the only way to set 
extensions, but personally I am open to use case that it might be difficult to 
override -march, so I am not blocking this.
I am still of the opinion that options are already an enormous mess, and 
introducing another way is not making things necessarily better. But if we 
document this, and add -m options for existing extensions, it may not be worse. 
So that will be my request for this patch, that we document this somewhere in 
the Clang docs, and then ideally we see the patches for existing extensions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113779

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

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[Steve Canon via Phabricator via cfe-commits]

scanon added a comment.

Wearing my compiler user hat, I would much rather use additive -mfeature than 
have to specify these as -march+feature, even when using a build system that 
nominally handles this stuff, because I frequently want to be able to compile 
one specific file with "whatever the prevailing options are, but also enable 
this one feature." Most build systems make this possible somehow (track down 
the arch variable, append +feature to it, etc), but it's considerably simpler 
if you can just append -mfeature to the list of flags and call it a day.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113779

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

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[Florian Hahn via Phabricator via cfe-commits]

fhahn updated this revision to Diff 458407.
fhahn added a comment.

Rebase and ping :)

The potential benefit of having -m flags is also mentioned in this recent bug 
report: https://github.com/llvm/llvm-project/issues/57588


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113779

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Arch/AArch64.cpp
  clang/lib/Driver/ToolChains/Arch/ARM.cpp
  clang/test/Preprocessor/aarch64-target-features.c
  clang/test/Preprocessor/arm-target-features.c

Index: clang/test/Preprocessor/arm-target-features.c
===
--- clang/test/Preprocessor/arm-target-features.c
+++ clang/test/Preprocessor/arm-target-features.c
@@ -25,6 +25,7 @@
 // CHECK-V8A-ALLOW-FP-INSTR-V8A-NOT: #define __ARM_FEATURE_DOTPROD
 
 // RUN: %clang -target arm-none-linux-gnueabi -march=armv8.2-a+nofp16fml+fp16 -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-VECTOR-SCALAR %s
+// RUN: %clang -target arm-none-linux-gnueabi -march=armv8.2-a -mnofp16fml -mfp16 -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-VECTOR-SCALAR %s
 // RUN: %clang -target arm-none-linux-gnueabi -march=armv8.2-a+nofp16+fp16fml -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-VECTOR-SCALAR %s
 // RUN: %clang -target arm-none-linux-gnueabi -march=armv8.2-a+fp16+nofp16fml -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-VECTOR-SCALAR %s
 // RUN: %clang -target arm-none-linux-gnueabi -march=armv8-a+fp16fml -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-VECTOR-SCALAR %s
@@ -77,6 +78,7 @@
 // CHECK-FULLFP16-SOFT-NOT: #define __ARM_FEATURE_FP16_VECTOR_ARITHMETIC
 
 // RUN: %clang -target arm-none-linux-gnueabi -march=armv8.2a+dotprod -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-DOTPROD %s
+// RUN: %clang -target arm-none-linux-gnueabi -march=armv8.2a -mdotprod -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-DOTPROD %s
 // CHECK-DOTPROD: #define __ARM_FEATURE_DOTPROD 1
 
 // RUN: %clang -target armv8r-none-linux-gnu -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-V8R %s
Index: clang/test/Preprocessor/aarch64-target-features.c
===
--- clang/test/Preprocessor/aarch64-target-features.c
+++ clang/test/Preprocessor/aarch64-target-features.c
@@ -200,20 +200,29 @@
 // CHECK-SVE2BITPERM: __ARM_FEATURE_SVE2_BITPERM 1
 
 // RUN: %clang -target aarch64-none-linux-gnu -march=armv8.2a+dotprod -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-DOTPROD %s
+// RUN: %clang -target aarch64-none-linux-gnu -march=armv8.2a -mdotprod -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-DOTPROD %s
 // CHECK-DOTPROD: __ARM_FEATURE_DOTPROD 1
 
 // On ARMv8.2-A and above, +fp16fml implies +fp16.
 // On ARMv8.4-A and above, +fp16 implies +fp16fml.
 // RUN: %clang -target aarch64-none-linux-gnueabi -march=armv8.2-a+nofp16fml+fp16 -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-NOFML --check-prefix=CHECK-FULLFP16-VECTOR-SCALAR %s
+// RUN: %clang -target aarch64-none-linux-gnueabi -march=armv8.2-a -mnofp16fml -mfp16 -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-NOFML --check-prefix=CHECK-FULLFP16-VECTOR-SCALAR %s
 // RUN: %clang -target aarch64-none-linux-gnueabi -march=armv8.2-a+nofp16+fp16fml -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-FML --check-prefix=CHECK-FULLFP16-VECTOR-SCALAR %s
+// RUN: %clang -target aarch64-none-linux-gnueabi -march=armv8.2-a -mnofp16 -mfp16fml -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-FML --check-prefix=CHECK-FULLFP16-VECTOR-SCALAR %s
 // RUN: %clang -target aarch64-none-linux-gnueabi -march=armv8.2-a+fp16+nofp16fml -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-NOFML --check-prefix=CHECK-FULLFP16-VECTOR-SCALAR %s
+// RUN: %clang -target aarch64-none-linux-gnueabi -march=armv8.2-a -mfp16 -mnofp16fml -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-NOFML --check-prefix=CHECK-FULLFP16-VECTOR-SCALAR %s
 // RUN: %clang -target aarch64-none-linux-gnueabi -march=armv8-a+fp16fml -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-FML --check-prefix=CHECK-FULLFP16-VECTOR-SCALAR %s
+// RUN: %clang -target aarch64-none-linux-gnueabi -march=armv8-a -mfp16fml -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-FML --check-prefix=CHECK-FULLFP16-VECTOR-SCALAR %s
 // RUN: %clang -target aarch64-none-linux-gnueabi -march=armv8-a+fp16 -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-NOFML 

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[Florian Hahn via Phabricator via cfe-commits]

fhahn added a comment.
Herald added a subscriber: MaskRay.
Herald added a project: All.

In D113779#3207936 , @SjoerdMeijer 
wrote:

>> If anybody has contacts to GCC that would be very helpful. Unfortunately I 
>> don't think I will be able to drive this.
>
> Ok, I will bring this up internally first with some folks that work on GCC 
> and see what happens. To be continued...

Hi, did you get an update by any chance?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113779

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

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

> If anybody has contacts to GCC that would be very helpful. Unfortunately I 
> don't think I will be able to drive this.

Ok, I will bring this up internally first with some folks that work on GCC and 
see what happens. To be continued...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113779

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

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[Florian Hahn via Phabricator via cfe-commits]

fhahn added a comment.

In D113779#3192009 , @SjoerdMeijer 
wrote:

> Ok, fair enough, perhaps adding features is a valid use-case.
>
> I will refrain from commenting on "things are terribly broken".  I agree it 
> is broken, but in a different way than suggested in previous comments. 
> If others also think this makes sense, then here a few follow up remarks from 
> my side:
>
> - First of all, this (really) sets precedent for setting options in a 
> different way. This needs documentation and release noting.
> - If we are going to do this, this should be the first patch in a series to 
> fix this for all features. We can't just do a few of them.

Agreed.

> - There was a suggestion to allow adding features with -march=+feature. Was 
> this dismissed in favour of how things works for x86 and be consistent with 
> that?

This would be the easiest way to implement this, *but* it would require either 
to allow not specify an architecture version with `-march` (which seems a bit 
odd) or perhaps adding a `default` architecture version which just uses the 
default set. For our users, either would work, so I'd be happy to go with what 
seems most compelling to others.

But compatibility with X86 is IMO also valuable for people porting from 
X86->AArch64.

> It would be really good if we keep options consistent/compatible across the 
> GCC and Clang toolchains. Any possibility to check with GCC community if they 
> are open for this?

If anybody has contacts to GCC that would be very helpful. Unfortunately I 
don't think I will be able to drive this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113779

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

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

Ok, fair enough, perhaps adding features is a valid use-case.

I will refrain from commenting on "things are terribly broken".  I agree it is 
broken, but in a different way than suggested in previous comments. 
If others also think this makes sense, then here a few follow up remarks from 
my side:

- First of all, this (really) sets precedent for setting options in a different 
way. This needs documentation and release noting.
- If we are going to do this, this should be the first patch in a series to fix 
this for all features. We can't just do a few of them.
- There was a suggestion to allow adding features with -march=+feature. Was 
this dismissed in favour of how things works for x86 and be consistent with 
that?
- It would be really good if we keep options consistent/compatible across the 
GCC and Clang toolchains. Any possibility to check with GCC community if they 
are open for this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113779

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

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[Florian Hahn via Phabricator via cfe-commits]

fhahn added a comment.

ping :)

Any additional thoughts? Since the original concerns were raised both 
@manojgupta  and myself tried to share a bit of additional background on the 
motivation and to clarify the difference between `-mXXX` and `-march`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113779

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

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[Manoj Gupta via Phabricator via cfe-commits]

manojgupta added a comment.

> More subjective: for most users this whole -march business is abstracted away 
> in build systems, so they won't have to deal with this, that's why this isn't 
> so much of an improvement.



> If we want a better user experience set options, there are probably other 
> things that are more important, like checking legal/illegal architecture 
> combinations.



> So, in summary, we prefer not to go ahead with this. And the precedent that 
> was mentioned, -mcrc, should probably be deprecated.

I'd argue the contrary that the current way of -march=isa+feature is broken. I 
am yet to see a build system that understands or processes the values inside 
march arguments. And this blocks users from choosing custom hw features without 
resorting to terrible hacks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113779

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

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[Florian Hahn via Phabricator via cfe-commits]

fhahn added a comment.

In D113779#3130853 , @paulwalker-arm 
wrote:

> Rather than adding connivence options after the fact what about allowing 
> `-march=` to be specified multiple times? The first must be the usual format 
> with later ones required to start with `+`.  The defined parsing behaviour 
> would be as if there was a single `-march` instance positioned at the first 
> occurrence but containing the value of all instances when combined from left 
> to right.  For example `-march=armv8.4-a .. -march=+nofp16` or perhaps 
> `+=` syntax like  `-march=armv8.4-a .. -march+=nofp16+nosve` is more 
> intuitive?

I think that would be a convenient option which wouldn't require us to add a 
lot of new options, which would be quite cumbersome. But to address the main 
issues (providing a purely additive way to specify features) I think we would 
need to allow `-march=+feature`, without any preceding `-march=armvXXX`.

In D113779#3131569 , @manojgupta 
wrote:

> Yes, the current approach of "-march=+feature" is terrible and does not 
> work with developers who want flexibility of features. This being pitched as 
> a feature imo is akin to promoting a design bug as a feature. 
> Any additive or subtractive alternative is welcome.

I wouldn't go so far as to call the current `-march` handling terrible, but I 
think there are valid use cases that cannot be addressed with it, as per the 
current discussion.  As you said, providing some way specify features 
additively without also committing to a specific architecture version would be 
desirable for our users IMO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113779

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

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[Manoj Gupta via Phabricator via cfe-commits]

manojgupta added a comment.

Yes, the current approach of "-march=+feature" is terrible and does not 
work with developers who want flexibility of features. This being pitched as a 
feature imo is akin to promoting a design bug as a feature. 
Any additive or subtractive alternative is welcome.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113779

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

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[Paul Walker via Phabricator via cfe-commits]

paulwalker-arm added a comment.

Rather than adding connivence options after the fact what about allowing 
`-march=` to be specified multiple times? The first must be the usual format 
with later ones required to start with `+`.  The defined parsing behaviour 
would be as if there was a single `-march` instance positioned at the first 
occurrence but containing the value of all instances when combined from left to 
right.  For example `-march=armv8.4-a .. march=+nofp16` or perhaps `+=` 
syntax like  `-march=armv8.4-a .. march+=nofp16+nosve` is more intuitive?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113779

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

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[Florian Hahn via Phabricator via cfe-commits]

fhahn added a comment.

In D113779#3130744 , @DavidSpickett 
wrote:

> There was a similar proposal for crypto https://reviews.llvm.org/D60472.
>
> Quoting @manojgupta for the pitch for that:
>
>> The motivation for this change is to make "crypto" setting an additive 
>> option e.g. like "-mavx" used in many media packages. Some packages in 
>> Chrome want to enable crypto conditionally for a few files to allow crypto 
>> feature to be used based on runtime cpu detection. They set 
>> "-march=armv8+crypto" flag but it gets overridden by the global 
>> "-march=armv8a" flag set by the build system in Chrome OS because the target 
>> cpu does not support crypto causing compile-time errors.
>> Ability to specify "-mcrypto" standalone makes it an additive option and 
>> ensures that it it is not lost. i.e. this will help in decoupling "-mcrypto" 
>> from "-march" so that they could be set independently. The current additive 
>> alternate is '-Xclang -target-feature -Xclang "+crypto" ' which is ugly.
>
> Concerns were raised there.
>
> Seems like we're balancing adding a bunch of (clang only for at least the 
> short term) options versus making the build system(s) cope with having to 
> track what the last `-march` was.

Thanks for sharing the related patch/discussion.

> versus making the build system(s) cope with having to track what the last 
> `-march` was.

Please see my earlier response. There can be use cases where the build system 
may not know what the last `-march` was, because the user may not have 
specified an explicit `-march` (e.g. they want to use the default picked by the 
compiler). Is there a way for the build system to conveniently determine what 
the default architecture version of the compiler is?

Also pushing this to the build system vendors means we need to convince X 
build-system vendors to adjust their implementations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113779

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

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

(I should note that crytpo isn't the best example because it means different 
things to different base architectures, that part doesn't apply here)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113779

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

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added subscribers: manojgupta, DavidSpickett.
DavidSpickett added a comment.

There was a similar proposal for crypto https://reviews.llvm.org/D60472.

Quoting @manojgupta for the pitch for that:

> The motivation for this change is to make "crypto" setting an additive option 
> e.g. like "-mavx" used in many media packages. Some packages in Chrome want 
> to enable crypto conditionally for a few files to allow crypto feature to be 
> used based on runtime cpu detection. They set "-march=armv8+crypto" flag but 
> it gets overridden by the global "-march=armv8a" flag set by the build system 
> in Chrome OS because the target cpu does not support crypto causing 
> compile-time errors.
> Ability to specify "-mcrypto" standalone makes it an additive option and 
> ensures that it it is not lost. i.e. this will help in decoupling "-mcrypto" 
> from "-march" so that they could be set independently. The current additive 
> alternate is '-Xclang -target-feature -Xclang "+crypto" ' which is ugly.

Concerns were raised there.

Seems like we're balancing adding a bunch of (clang only for at least the short 
term) options versus making the build system(s) cope with having to track what 
the last `-march` was.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113779

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

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[Florian Hahn via Phabricator via cfe-commits]

fhahn added a comment.

In D113779#3130701 , @SjoerdMeijer 
wrote:

> This introduces another way of setting (optional) architecture extensions and 
> having two ways to do the same is nearly always a bad thing, which is how one 
> of my colleagues phrased it.

I think there's a subtle difference to using the `-mXXX` options vs `-march`. 
`-mXXX` adds a feature *without* changing the default architecture version. 
Unless I am missing something, it's not possible to selectively add target 
features using `-march` without also forcing the architecture version to some 
particular version.

AFAICT the only alternative to `-mXXX` options would be using `-Xclang 
-target-feature -Xclang +dotprod`, which is very verbose and not documented.

The `-mXXX` flags would allow users to conveniently write forward-compatible 
compiler invocations that also ensure a minimum set of features is available. 
Using `-mXXX` flags is very common on X86 and providing a similar interface for 
AArch64 will likely help users transitioning.

> - how do these new `-m` flags and `-march` interact?

The `-mXXX` flags add the target feature to the list of target features, the 
same as additional features provided to `-march`. It should be equivalent to 
specifying extra flags via `-march` in terms of checking for invalid 
combinations and conflict resolution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113779

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

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added a comment.

This introduces another way of setting (optional) architecture extensions and 
having two ways to do the same is nearly always a bad thing, which is how one 
of my colleagues phrased it.

This was already a complex area and thus I don't think introducing another is 
going to really simplify things. This is the main problem of this proposal. 
Other things are:

- ideally we keep these flags consistent with GCC, but that seems to be a no-go.
- how do these new `-m` flags and `-march` interact?

More subjective: for most users this whole `-march` business is abstracted away 
in build systems, so they won't have to deal with this, that's why this isn't 
so much of an improvement.

If we want a better user experience set options, there are probably other 
things that are more important, like checking legal/illegal architecture 
combinations.

So, in summary, we prefer not to go ahead with this. And the precedent that was 
mentioned, `-mcrc`, should probably be deprecated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113779

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

[PATCH] D113779: [Clang] Add mfp16, mfp16fml and mdotprod flags for ARM target features.

[Florian Hahn via Phabricator via cfe-commits]

fhahn created this revision.
fhahn added reviewers: aemerson, ab, dmgreen, SjoerdMeijer, scanon.
Herald added subscribers: dang, kristof.beyls.
fhahn requested review of this revision.
Herald added a project: clang.

This patch adds support for `-m[no]fpf16`, ` `-m[no]fpf16fml` and
`-m[no]dotprod` feature flags for ARM.

The flags behave similar to specifying an -march flag with
+/-{fp16,fp16fml,dotprod}, but is more convenient for users that do not
want to provide a specific architecture version. There are similar flags
alread, like `-mcrc`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113779

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Arch/AArch64.cpp
  clang/lib/Driver/ToolChains/Arch/ARM.cpp
  clang/test/Preprocessor/aarch64-target-features.c
  clang/test/Preprocessor/arm-target-features.c

Index: clang/test/Preprocessor/arm-target-features.c
===
--- clang/test/Preprocessor/arm-target-features.c
+++ clang/test/Preprocessor/arm-target-features.c
@@ -25,6 +25,7 @@
 // CHECK-V8A-ALLOW-FP-INSTR-V8A-NOT: #define __ARM_FEATURE_DOTPROD
 
 // RUN: %clang -target arm-none-linux-gnueabi -march=armv8.2-a+nofp16fml+fp16 -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-VECTOR-SCALAR %s
+// RUN: %clang -target arm-none-linux-gnueabi -march=armv8.2-a -mnofp16fml -mfp16 -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-VECTOR-SCALAR %s
 // RUN: %clang -target arm-none-linux-gnueabi -march=armv8.2-a+nofp16+fp16fml -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-VECTOR-SCALAR %s
 // RUN: %clang -target arm-none-linux-gnueabi -march=armv8.2-a+fp16+nofp16fml -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-VECTOR-SCALAR %s
 // RUN: %clang -target arm-none-linux-gnueabi -march=armv8-a+fp16fml -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-VECTOR-SCALAR %s
@@ -77,6 +78,7 @@
 // CHECK-FULLFP16-SOFT-NOT: #define __ARM_FEATURE_FP16_VECTOR_ARITHMETIC
 
 // RUN: %clang -target arm-none-linux-gnueabi -march=armv8.2a+dotprod -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-DOTPROD %s
+// RUN: %clang -target arm-none-linux-gnueabi -march=armv8.2a -mdotprod -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-DOTPROD %s
 // CHECK-DOTPROD: #define __ARM_FEATURE_DOTPROD 1
 
 // RUN: %clang -target armv8r-none-linux-gnu -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-V8R %s
Index: clang/test/Preprocessor/aarch64-target-features.c
===
--- clang/test/Preprocessor/aarch64-target-features.c
+++ clang/test/Preprocessor/aarch64-target-features.c
@@ -199,20 +199,29 @@
 // CHECK-SVE2BITPERM: __ARM_FEATURE_SVE2_BITPERM 1
 
 // RUN: %clang -target aarch64-none-linux-gnu -march=armv8.2a+dotprod -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-DOTPROD %s
+// RUN: %clang -target aarch64-none-linux-gnu -march=armv8.2a -mdotprod -x c -E -dM %s -o - | FileCheck --check-prefix=CHECK-DOTPROD %s
 // CHECK-DOTPROD: __ARM_FEATURE_DOTPROD 1
 
 // On ARMv8.2-A and above, +fp16fml implies +fp16.
 // On ARMv8.4-A and above, +fp16 implies +fp16fml.
 // RUN: %clang -target aarch64-none-linux-gnueabi -march=armv8.2-a+nofp16fml+fp16 -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-NOFML --check-prefix=CHECK-FULLFP16-VECTOR-SCALAR %s
+// RUN: %clang -target aarch64-none-linux-gnueabi -march=armv8.2-a -mnofp16fml -mfp16 -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-NOFML --check-prefix=CHECK-FULLFP16-VECTOR-SCALAR %s
 // RUN: %clang -target aarch64-none-linux-gnueabi -march=armv8.2-a+nofp16+fp16fml -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-FML --check-prefix=CHECK-FULLFP16-VECTOR-SCALAR %s
+// RUN: %clang -target aarch64-none-linux-gnueabi -march=armv8.2-a -mnofp16 -mfp16fml -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-FML --check-prefix=CHECK-FULLFP16-VECTOR-SCALAR %s
 // RUN: %clang -target aarch64-none-linux-gnueabi -march=armv8.2-a+fp16+nofp16fml -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-NOFML --check-prefix=CHECK-FULLFP16-VECTOR-SCALAR %s
+// RUN: %clang -target aarch64-none-linux-gnueabi -march=armv8.2-a -mfp16 -mnofp16fml -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-NOFML --check-prefix=CHECK-FULLFP16-VECTOR-SCALAR %s
 // RUN: %clang -target aarch64-none-linux-gnueabi -march=armv8-a+fp16fml -x c -E -dM %s -o - | FileCheck -match-full-lines --check-prefix=CHECK-FULLFP16-FML --check-prefix=CHECK-FULLFP16-VECTOR-SCALAR %s
+// RUN: %clang -target aarch64-none-linux-gnueabi -march=armv8-a -mfp16fml -x c -E -dM %s -o - | FileCheck -match-full-lines