<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"><head><meta http-equiv=Content-Type content="text/html; charset=utf-8"><meta name=Generator content="Microsoft Word 14 (filtered medium)"><style><!--
/* Font Definitions */
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Tahoma;
panose-1:2 11 6 4 3 5 4 4 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman","serif";}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
span.EmailStyle17
{mso-style-type:personal-reply;
font-family:"Calibri","sans-serif";
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri","sans-serif";
mso-fareast-language:EN-US;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:72.0pt 72.0pt 72.0pt 72.0pt;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]--></head><body lang=EN-GB link=blue vlink=purple><div class=WordSection1><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>My apologies for joining this review late.<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Generally speaking, I think that gcc has a sane set of command line<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>options to target AArch64, see <a href="https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html">https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html</a>,<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>so I don't see much reason to deviate from those.<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>I think that the patch as is is worth committing, apart from the<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>following issues that I think need to be fixed first:<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>* At the moment, the patch implements "neon" as an -march and -mcpu<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'> feature modifier. I think this needs to be renamed to "simd", i.e.<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'> the same name as gcc uses. This is more in line with the architecture<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'> specification too, which calls this "Advanced SIMD", not "Neon".<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'> It might be worthwhile, if possible, for the error message to<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'> suggest using "simd" instead of "neon" as the feature modifier<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'> when a user did use "+(no)neon"?<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>* I think it'd be worthwhile to add tests to check that the following<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'> documented gcc behaviour is also clangs behaviour after committing<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'> this patch:<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'> "Where conflicting feature modifiers are specified, the<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'> right-most feature is used."<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>* On deprecating -mcpu: I think that should be left to a follow-on<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'> patch and probably needs some further discussing, as I'm not<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'> fully convinced it's the right thing to do. If we did deprecate<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'> -mcpu, I do think that we would have to start accepting cpu<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'> names to the -march flag, as a shorthand for "the architecture<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'> variant as implemented by that CPU". For example, -march cortex-a57<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'> would be equivalent to specifying -march armv8-a+fp+simd+crypto+crc.<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'> We would also lose command line compatibility with gcc, which<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'> would make it harder for people using gcc to start trying out clang.<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Thanks,<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Kristof<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><div style='border:none;border-left:solid blue 1.5pt;padding:0cm 0cm 0cm 4.0pt'><div><div style='border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0cm 0cm 0cm'><p class=MsoNormal><b><span lang=EN-US style='font-size:10.0pt;font-family:"Tahoma","sans-serif"'>From:</span></b><span lang=EN-US style='font-size:10.0pt;font-family:"Tahoma","sans-serif"'> cfe-commits-bounces@cs.uiuc.edu [mailto:cfe-commits-bounces@cs.uiuc.edu] <b>On Behalf Of </b>Kevin Qin<br><b>Sent:</b> 09 July 2014 07:51<br><b>To:</b> Eric Christopher<br><b>Cc:</b> Clang Commits; kanheim@a-bix.com; LLVM Commits; reviews+D4346+public+5fab7236f933ff44@reviews.llvm.org<br><b>Subject:</b> Re: [PATCH] [AArch64] Implement Clang CLI interface proposal about "-march".<o:p></o:p></span></p></div></div><p class=MsoNormal><o:p> </o:p></p><div><p class=MsoNormal>HI Eric,<o:p></o:p></p><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>Thanks for your feedback. Below is my comments.<o:p></o:p></p><div><p class=MsoNormal style='margin-bottom:12.0pt'><o:p> </o:p></p><div><p class=MsoNormal>2014-07-09 2:25 GMT+08:00 Eric Christopher <<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>>:<o:p></o:p></p><div><div><p class=MsoNormal style='margin-bottom:12.0pt'>>> > 4. Implement support of "-mtune". Usage is: "-march=CPU_NAME". For<br>>> > instance, "-march=cortex-a57". This option will ONLY get micro-architecture<br>>> > level feature enabled specifying to target CPU, like "zcm" and "zcz" for<br>>> > cyclone. Any architecture features WON'T be modified.<br>>><br>>> That's not what -mtune is. According to GCC's manual: "Tune to<br>>> cpu-type everything applicable about the generated code, except for<br>>> the ABI and the set of available instructions."<br>>><br>>> The difference between -mcup and -mtune is that the former selects ABI<br>>> and ISAs supported by the CPU, while the former doesn't. This is<br>>> particularly important if you want to run the code on a newer CPU but<br>>> doesn't want to break older ones, so you can't use instructions that<br>>> the old ones don't have, but you can optimise for the pipeline and<br>>> branch decisions of the newer CPU, as long as it just slows down the<br>>> older ones.<br>><br>> I didn't explain it clearly. Your point is totally what I did in this patch.<br>> I emphasize " ONLY get micro-architecture level feature enabled" is want to<br>> say ISA won't be changed by this option. This option is to select target CPU<br>> to optimize for, including enabling micro-architecture level feature,<br>> choosing MI scheduler and triggering any optimizations specific for target.<br>>><br>>><br>>><br>>> > 5. Change usage of "-mcpu" to "-mcpu=CPU_NAME+[no]feature", which is an<br>>> > alias to "-march={feature of CPU_NAME}+[no]feature" and "-mtune=CPU_NAME"<br>>> > together. An warning is added to discourage use of this option.<br>>><br>>> I find this one redundant with -march and don't think we should add<br>>> deprecated features. -mcpu is the flag you want for the behaviour<br>>> you've done -mtune above. AFAIK, we don't have the infrastructure to<br>>> implement -mtune yet. Also, the driver is a bit bonkers when going<br>>> from CPU to Arch from a different arch than the host without using<br>>> -target (which is the point with -march, I guess).<br>>><br>>> I don't think -mcpu should be used on its own, only in conjunction<br>>> with -target or -march.<br>><br>> In my patch, the difference between "-mcpu" and "-mtune" is that, "-mcpu"<br>> will enable all ISAs which target CPU supports, while "-mtune" won't do<br>> this. And "-mcpu" can accept extra feature modifiers to make a change, but<br>> "-mtune" accepts CPU name only. So "-mcpu" is an shortcut of "-march" and<br>> "-tune". Keeping this option alive in clang is because it's still alive in<br>> gcc, and may still be used in many projects. An warning is added to<br>> discourage use of this option.<o:p></o:p></p></div></div><p class=MsoNormal>This is fine, and I encourage the warning. Also, -march should<br>probably default to -mtune of the same architecture. I didn't read to<br>verify, but just making sure this is the case.<o:p></o:p></p><div><p class=MsoNormal>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.<o:p></o:p></p></div><blockquote style='border:none;border-left:solid #CCCCCC 1.0pt;padding:0cm 0cm 0cm 6.0pt;margin-left:4.8pt;margin-right:0cm'><div><p class=MsoNormal style='margin-bottom:12.0pt'><br>>><br>>><br>>><br>>><br>>> > 1. Neon is enabled by default, and "generic" will be used if no CPU type<br>>> > is specified.<br>>><br>>> Makes sense to me.<br>>><br>>><br>>> > 2. For most scenario, Using "-mtune=CPU" only is recommended as neon is<br>>> > enabled by default and all micro-architecture optimizations are selected,<br>>> > and it would provide great compatibility to run on most of AArch64 devices.<br>>><br>>> That'd be -mcpu, and we still need -march or -target.<br>><br>> "-target" is still necessary at moment while "-march" can be omitted<br>> sometimes, because the settings of default feature can work well for most<br>> scenarios and provide good code migration. All I want to do is to get<br>> "-mcpu" supporter happy to use "-mtune" instead. They don't need to complain<br>> typing too much as splitting "-mcpu" into "-march" and "-mtune" because they<br>> can use "-mtune" only. For a standard sets of compiling flags, pair use of<br>> "-march" and "-mtune" is strongly recommended.<o:p></o:p></p></div><p class=MsoNormal>This seems to be a good idea. Can you give examples of behavior you're<br>expecting to see just to verify?<o:p></o:p></p></blockquote><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>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.<o:p></o:p></p></div><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>"-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.<o:p></o:p></p></div><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>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 <span style='font-size:10.5pt;font-family:"Arial","sans-serif";color:#323232'>explicitly select the </span>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.<o:p></o:p></p></div><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal><span style='font-size:10.5pt;font-family:"Arial","sans-serif";color:#333333'>To </span><span style='font-family:"Arial","sans-serif";color:#333333'>summarize</span>, missing of "-march" can work well at moment, but should only be used for short term experiment. Pair using "-march" and "-mtune" is recommended.<o:p></o:p></p></div><blockquote style='border:none;border-left:solid #CCCCCC 1.0pt;padding:0cm 0cm 0cm 6.0pt;margin-left:4.8pt;margin-right:0cm'><div><p class=MsoNormal style='margin-bottom:12.0pt'><br>>><br>>><br>>><br>>> > 3. "-march" is designed to be used only if user wants to use crc and<br>>> > crypto instructions, or disable fp/neon. So "-march" will not be frequently<br>>> > used and won't bring too much finger burden.<br>>><br>>> I thought the idea was to encourage -march... at least on new targets...<br>><br>> Yes, we always encourage people to specifying architecture features via<br>> "-march". Letting "-march" and "-mtune" replace "-mcpu" and "-mfpu" is what<br>> we want to do.<o:p></o:p></p></div><p class=MsoNormal>Very much so.<br><br>Thanks!<br><span style='color:#888888'><br>-eric</span><o:p></o:p></p><div><p class=MsoNormal><br>>><br>>><br>>> --renato<br>><br>><br>><br>><br>> --<br>> Best Regards,<br>><br>> Kevin Qin<br>><o:p></o:p></p></div><div><div><p class=MsoNormal>> _______________________________________________<br>> cfe-commits mailing list<br>> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>><o:p></o:p></p></div></div></blockquote></div><p class=MsoNormal><br><br clear=all><o:p></o:p></p><div><p class=MsoNormal><o:p> </o:p></p></div><p class=MsoNormal>-- <o:p></o:p></p><div><p class=MsoNormal>Best Regards,<o:p></o:p></p><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>Kevin Qin<o:p></o:p></p></div></div></div></div></div></div></div></body></html>