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

[Eric Christopher]

On Wed, Jul 9, 2014 at 6:10 AM, Kristof Beyls <kristof.beyls at arm.com> wrote:
> My apologies for joining this review late.
>
>
>
> Generally speaking, I think that gcc has a sane set of command line
>
> options to target AArch64, see
> https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html,
>
> so I don't see much reason to deviate from those.

With a narrow focus of only ARM processors I can see how you'd think that. :\

It's quite upsetting that ARM decided to deviate again from the
established command line options when doing the AArch64 port.


> I think that the patch as is is worth committing, apart from the
>
> following issues that I think need to be fixed first:
>
> * At the moment, the patch implements "neon" as an -march and -mcpu
>
>   feature modifier. I think this needs to be renamed to "simd", i.e.
>
>   the same name as gcc uses. This is more in line with the architecture
>
>   specification too, which calls this "Advanced SIMD", not "Neon".
>
>   It might be worthwhile, if possible, for the error message to
>
>   suggest using "simd" instead of "neon" as the feature modifier
>
>   when a user did use "+(no)neon"?
>

Seems like a good change.

> * I think it'd be worthwhile to add tests to check that the following
>
>   documented gcc behaviour is also clangs behaviour after committing
>
>   this patch:
>
>   "Where conflicting feature modifiers are specified, the
>
>    right-most feature is used."
>

Agreed.

> * On deprecating -mcpu: I think that should be left to a follow-on
>
>   patch and probably needs some further discussing, as I'm not
>
>   fully convinced it's the right thing to do. If we did deprecate
>
>   -mcpu, I do think that we would have to start accepting cpu
>
>   names to the -march flag, as a shorthand for "the architecture
>
>   variant as implemented by that CPU". For example, -march cortex-a57
>
>   would be equivalent to specifying -march armv8-a+fp+simd+crypto+crc.
>
>   We would also lose command line compatibility with gcc, which
>
>   would make it harder for people using gcc to start trying out clang.
>

Let's just have -march=cyclone/cortex-a57 work right from the start
and we won't have to worry about it.

Thanks.

-eric

>
>
> Thanks,
>
>
>
> Kristof
>
>
>
> From: cfe-commits-bounces at cs.uiuc.edu
> [mailto:cfe-commits-bounces at cs.uiuc.edu] On Behalf Of Kevin Qin
> Sent: 09 July 2014 07:51
> To: Eric Christopher
> Cc: Clang Commits; kanheim at a-bix.com; LLVM Commits;
> reviews+D4346+public+5fab7236f933ff44 at reviews.llvm.org
> Subject: Re: [PATCH] [AArch64] Implement Clang CLI interface proposal about
> "-march".
>
>
>
> HI Eric,
>
>
>
> Thanks for your feedback. Below is my comments.
>
>
>
> 2014-07-09 2:25 GMT+08:00 Eric Christopher <echristo at gmail.com>:
>
>>> > 4. 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.
>>>
>>> That's not what -mtune is. According to GCC's manual: "Tune to
>>> cpu-type everything applicable about the generated code, except for
>>> the ABI and the set of available instructions."
>>>
>>> The difference between -mcup and -mtune is that the former selects ABI
>>> and ISAs supported by the CPU, while the former doesn't. This is
>>> particularly important if you want to run the code on a newer CPU but
>>> doesn't want to break older ones, so you can't use instructions that
>>> the old ones don't have, but you can optimise for the pipeline and
>>> branch decisions of the newer CPU, as long as it just slows down the
>>> older ones.
>>
>> I didn't explain it clearly. Your point is totally what I did in this
>> patch.
>> I emphasize " ONLY get micro-architecture level feature enabled" is want
>> to
>> say ISA won't be changed by this option. This option is to select target
>> CPU
>> to optimize for, including enabling micro-architecture level feature,
>> choosing MI scheduler and triggering any optimizations specific for
>> target.
>>>
>>>
>>>
>>> > 5. 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.
>>>
>>> I find this one redundant with -march and don't think we should add
>>> deprecated features. -mcpu is the flag you want for the behaviour
>>> you've done -mtune above. AFAIK, we don't have the infrastructure to
>>> implement -mtune yet. Also, the driver is a bit bonkers when going
>>> from CPU to Arch from a different arch than the host without using
>>> -target (which is the point with -march, I guess).
>>>
>>> I don't think -mcpu should be used on its own, only in conjunction
>>> with -target or -march.
>>
>> In my patch, the difference between "-mcpu" and "-mtune" is that, "-mcpu"
>> will enable all ISAs which target CPU supports, while "-mtune" won't do
>> this. And "-mcpu" can accept extra feature modifiers to make a change, but
>> "-mtune" accepts CPU name only. So "-mcpu" is an shortcut of "-march" and
>> "-tune". Keeping this option alive in clang is because it's still alive in
>> gcc, and may still be used in many projects.  An warning is added to
>> discourage use of this option.
>
> This is fine, and I encourage the warning. Also, -march should
> probably default to -mtune of the same architecture. I didn't read to
> verify, but just making sure this is the case.
>
> Currently, there's only one architecture available,  so -march will always
> default to "armv8-a+neon". We can do further when there's more and more
> architectures on AArch64 target.
>
>
>>>
>>>
>>>
>>>
>>> > 1. Neon is enabled by default, and "generic" will be used if no CPU
>>> > type
>>> > is specified.
>>>
>>> Makes sense to me.
>>>
>>>
>>> > 2. 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.
>>>
>>> That'd be -mcpu, and we still need -march or -target.
>>
>> "-target" is still necessary at moment while "-march" can be omitted
>> sometimes, because the settings of default feature can work well for most
>> scenarios and provide good code migration. All I want to do is to get
>> "-mcpu" supporter happy to use "-mtune" instead. They don't need to
>> complain
>> typing too much as splitting "-mcpu" into "-march" and "-mtune" because
>> they
>> can use "-mtune" only. For a standard sets of compiling flags, pair use of
>> "-march" and "-mtune" is strongly recommended.
>
> This seems to be a good idea. Can you give examples of behavior you're
> expecting to see just to verify?
>
>
>
> Single use of "-target aarch64-linux-gnu" equals "-target aarch64-linux-gnu
> -march=armv8-a+neon mtune=generic", which can provide correct codes but not
> fully optimized.
>
>
>
> "-target aarch64-linux-gnu -mtune=cortex-a57" euqals "-target
> aarch64-linux-gnu -march=armv8-a+neon mtune=cortex-a57" ,which can work
> quite well in most scenarios. NEON is enabled for vectorization and MI
> scheduler is selected to optimize codes for cortex-a57. And it provides good
> compatibility which allows binary running on most AArch64 devices as it
> doesn't rely on any crc or crypto support. New starters of AArch64 can
> easily start their project from these flags, and it is good enough for
> experiment purpose for experienced developer.
>
>
>
> If user wants to control more features, such as enable crc and crypto, or
> disable neon or fp, then they need to use "-target=aarch64-linux-gnu
> -march=armv8-a+[no]feature -mtune=cortex-a57". It's standard sets of flags I
> recommend to use, which explicitly select the architecture feature though
> command line.  Even if a project only require NEON, it's recommend to add
> "-march=armv8-a+neon" in command line. Because the default behavior of
> -march is unreliable, which may get change in future.
>
>
>
> To summarize, missing of "-march" can work well at moment, but should only
> be used for short term experiment. Pair using "-march" and "-mtune" is
> recommended.
>
>
>>>
>>>
>>>
>>> > 3. "-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.
>>>
>>> I thought the idea was to encourage -march... at least on new targets...
>>
>> Yes, we always encourage people to specifying architecture features via
>> "-march". Letting "-march" and "-mtune" replace "-mcpu" and "-mfpu" is
>> what
>> we want to do.
>
> Very much so.
>
> Thanks!
>
> -eric
>
>
>>>
>>>
>>> --renato
>>
>>
>>
>>
>> --
>> Best Regards,
>>
>> Kevin Qin
>>
>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
>
>
>
>
> --
>
> Best Regards,
>
>
>
> Kevin Qin