On Wed, Jul 9, 2014 at 11:45 AM, Eric Christopher <[email protected]> wrote: > On Tue, Jul 8, 2014 at 11:51 PM, Kevin Qin <[email protected]> wrote: >> 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. > > 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 >>> > [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
