On Wed, Jul 9, 2014 at 6:10 AM, Kristof Beyls <[email protected]> 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: [email protected] > [mailto:[email protected]] On Behalf Of Kevin Qin > Sent: 09 July 2014 07:51 > To: Eric Christopher > Cc: Clang Commits; [email protected]; LLVM Commits; > [email protected] > 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 <[email protected]>: > >>> > 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 >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> > > > > > > -- > > Best Regards, > > > > Kevin Qin _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
