Page MenuHomePhabricator

[AArch64] Implement Clang CLI interface proposal about "-march".
Needs ReviewPublic

Authored by kevin.qin on Jun 30 2014, 3:34 AM.

Details

Reviewers
t.p.northover
Summary

This patch is to cover below changes for AArch64 target.

  1. Revert "Add default feature for CPUs on AArch64 target in Clang" at r210625. Because it gets conflict with the rule that architecture feature should only be decided by "-march".
  1. Get "-mfpu" deprecated.
  1. Implement support of "-march". Usage is: "-march=armv8-a+[no]feature". For instance, "-march=armv8-a+neon+crc+nocrypto". Here "armv8-a" is necessary, and CPU names are not acceptable. Candidate features are fp, neon, crc and crypto.
  1. Implement support of "-mtune". Usage is: "-march=CPU_NAME". For instance, "-march=cortex-a57". This option will ONLY get micro-architecture level feature enabled specifying to target CPU, like "zcm" and "zcz" for cyclone. Any architecture features WON'T be modified.
  1. Change usage of "-mcpu" to "-mcpu=CPU_NAME+[no]feature", which is an alias to "-march={feature of CPU_NAME}+[no]feature" and "-mtune=CPU_NAME" together. An warning is added to discourage use of this option.

Here are some summary may concern.

  1. Neon is enabled by default, and "generic" will be used if no CPU type is specified.
  1. For most scenario, Using "-mtune=CPU" only is recommended as neon is enabled by default and all micro-architecture optimizations are selected, and it would provide great compatibility to run on most of AArch64 devices.
  1. "-march" is designed to be used only if user wants to use crc and crypto instructions, or disable fp/neon. So "-march" will not be frequently used and won't bring too much finger burden.

Regards,
Kevin

Diff Detail

Event Timeline

kevin.qin updated this revision to Diff 10973.Jun 30 2014, 3:34 AM
kevin.qin retitled this revision from to [AArch64] Implement Clang CLI interface proposal about "-march"..
kevin.qin updated this object.
kevin.qin edited the test plan for this revision. (Show Details)
kevin.qin added a reviewer: t.p.northover.
kevin.qin added subscribers: Unknown Object (MLST), Unknown Object (MLST).

One small inline comment.

lib/Driver/Tools.cpp
1521

"Split" would be better, or "Splitted". I'd go for the first :)

kevin.qin updated this revision to Diff 11432.Jul 15 2014, 3:32 AM

Hi all,

There're 3 changes in this patch.

  1. [no]neon is not allowed to pass through -march and -mcpu. An error message is added to suggest using [no]simd instead.
  2. Remove warning message for -mcpu. As it would not be deprecated in short term.
  3. Add more test to guarantee "where conflicting feature modifiers are specified, the right-most feature is used."

To summarize, The updated patch is to follow the same behavior as gcc for -march/-mcpu /-mtune. The behavior is shown in this link:
https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html .

Regards,
Kevin

kevin.qin updated this revision to Diff 11434.Jul 15 2014, 3:39 AM

Minor changes in test file to get check-all pass.

One inline comment.

Also, I'm seeing tests for -mtune and -mcpu, but very few for -march= behavior. Can you make some of those similar to the -mcpu ones to make sure both ways work?

Thanks!

lib/Driver/Tools.cpp
1530

Same comment as last time. Split or Splitted. I'd prefer the first.

kevin.qin updated this revision to Diff 11486.Jul 15 2014, 11:04 PM

Hi,

This patch is to change variable name from "Splited" to "Split".

For the test about -march, I believe the test in patch is enough to show all modifiers that it can accept, as CPU name is not allowed to put through -march. -mcpu and -mtune are the options designed to pass CPU name. See https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html .
I quoted related words from that webpage here:

-march=name
Specify the name of the target architecture, optionally suffixed by one or more feature modifiers. This option has the form -march=arch{+[no]feature}, where the only permissible value for arch is ‘armv8-a’."

Thanks,
Kevin

I think there's one more missing test or set of tests, to cover this sentence from the documentation of the -mcpu option:
"Where this option is used in conjunction with -march or -mtune, those options take precedence over the appropriate part of this option."

Apart from that, this patch looks good to me.

lib/Driver/Tools.cpp
1568

-march -> -mtune in the comment.
TBH, I don't think these comments add much value, the name of the functions are descriptive enough.

kevin.qin updated this revision to Diff 11557.Jul 16 2014, 10:47 PM

Hi,

Parts of codes are refactored to match this behavior.
"Where this option is used in conjunction with -march or -mtune, those options take precedence over the appropriate part of this option."

Regards,
Kevin

kristof.beyls added inline comments.Jul 17 2014, 1:21 AM
lib/Driver/Tools.cpp
1602

"Macro" -> "Micro"?

1616

"Macro" -> "Micro"?

test/Driver/aarch64-cpus.c
20

From an earlier comment you made: " '-mtune' won't automatically change any architecture level feature selection, but will enable some micro architecture feature according to CPU. For example, '-mtune=cyclone' won't enable crypto, but will enable zcm and zcz, and also enable all special pass and scheduler. "

With this patch, it seems mtune would be doing the wrong thing: changing the architecture being targeted (i.e. which instructions can be generated); not just which micro-architecture to optimize for.
That seems wrong to me.
At the moment, it seems the only useful information -mtune could provide to the backend for micro-architecture tuning without changing the architecture targeted are the sub-target-features +zcm and +zcz?

kevin.qin updated this revision to Diff 11632.Jul 17 2014, 11:12 PM

Hi Kristof,

This patch fixed the typo issue.

And for the question about -mtune, if "-mtune=cortex-a53" is used, it will convert to "-target-cpu cortex-a53" in -cc1 option. This option will only select CPU type and won't enable any implied feature. All enabled feature will by passed explicitly by -target-feature in -cc1 option.

Regards,
Kevin

kevin.qin updated this revision to Diff 11633.Jul 17 2014, 11:24 PM

Fix another typo missing in previous patch.

Committed in r213353.

2014-07-18 14:30 GMT+08:00 Kristof Beyls <kristof.beyls@arm.com>:

LGTM.

http://reviews.llvm.org/D4346