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

[Eric Christopher]

On Wed, Jul 9, 2014 at 11:45 AM, Eric Christopher <echristo at gmail.com> wrote:
> On Tue, Jul 8, 2014 at 11:51 PM, Kevin Qin <kevinqindev at gmail.com> wrote:
>> 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.
>
> I can agree with this. I also would like -march=cortex-a57 to work as
> part of this patch. There's no reason to have it be different between
> -march and -mtune.
>

To be clear, this change and the tests that Kristof mentioned (in
addition to the other tests) are all I see as necessary for this patch
to go in and can go in without further review.

Thanks for all of your work and dedication on this Kevin.

-eric

> Thanks.
>
> -eric
>
>>>
>>>
>>> >>
>>> >>
>>> >>
>>> >> > 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