<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
<style type="text/css" style="display:none;"><!-- P {margin-top:0;margin-bottom:0;} --></style>
</head>
<body dir="ltr">
<div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;font-family:Calibri,Helvetica,sans-serif;" dir="ltr">
<p style="margin-top:0;margin-bottom:0">A patch for initial refactoring is now up for review:</p>
<p style="margin-top:0;margin-bottom:0"><a href="https://reviews.llvm.org/D53980" class="OWAAutoLink">https://reviews.llvm.org/D53980</a></p>
<p style="margin-top:0;margin-bottom:0"><br>
</p>
<p style="margin-top:0;margin-bottom:0">It moves the ARM and AArch64 TargetParser namespaces into their own files. There's no change to the x86/AMDGPU side of things but those who work on that may be interested in the restructuring.</p>
<p style="margin-top:0;margin-bottom:0"><br>
</p>
<p style="margin-top:0;margin-bottom:0">I'm not sure who that would be specifically but feel free to take a look and leave any comments you might have.<br>
</p>
<p style="margin-top:0;margin-bottom:0"><br>
</p>
<p style="margin-top:0;margin-bottom:0">Thanks,</p>
<p style="margin-top:0;margin-bottom:0">David Spickett.<br>
</p>
</div>
<hr style="display:inline-block;width:98%" tabindex="-1">
<div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" style="font-size:11pt" color="#000000"><b>From:</b> cfe-dev <cfe-dev-bounces@lists.llvm.org> on behalf of David Spickett via cfe-dev <cfe-dev@lists.llvm.org><br>
<b>Sent:</b> 12 October 2018 12:01:16<br>
<b>To:</b> Bryan Chan; Renato Golin<br>
<b>Cc:</b> nd; Clang Dev<br>
<b>Subject:</b> Re: [cfe-dev] [llvm-dev] [RFC] New Clang target selection options for ARM/AArch64</font>
<div> </div>
</div>
<style type="text/css" style="display:none">
<!--
p
{margin-top:0;
margin-bottom:0}
-->
</style>
<div dir="ltr">
<div id="x_divtagdefaultwrapper" dir="ltr" style="font-size:12pt; color:#000000; font-family:Calibri,Helvetica,sans-serif">
<p style="margin-top:0; margin-bottom:0">Hi Bryan,<br>
</p>
<p style="margin-top:0; margin-bottom:0"><br>
</p>
<p style="margin-top:0; margin-bottom:0">Looks like arm will enable it, but AArch64 doesn't. I haven't been able to find a reason for this.</p>
<p style="margin-top:0; margin-bottom:0"><br>
</p>
<p style="margin-top:0; margin-bottom:0"></p>
<div>$ ./clang --target=aarch64-arm-none-eabi -march=armv8.4-a -dM -E -x c /dev/null | grep DOTPROD<br>
$ ./clang --target=arm-arm-none-eabi -march=armv8.4-a -dM -E -x c /dev/null | grep DOTPROD<br>
#define __ARM_FEATURE_DOTPROD 1</div>
<p></p>
<p style="margin-top:0; margin-bottom:0"><br>
</p>
<p style="margin-top:0; margin-bottom:0">That define being the one that gates the intrinsics in the header.</p>
<p style="margin-top:0; margin-bottom:0"><br>
</p>
<p style="margin-top:0; margin-bottom:0">This is certainly something we want to address with these proposed changes.
<br>
</p>
<p style="margin-top:0; margin-bottom:0"><br>
</p>
<p style="margin-top:0; margin-bottom:0">I have a feeling doing a fix now may require info we don't have. Since dot product is fairly straightforward but other 'default' extensions may have more complex dependencies. If you have ideas already we can discuss
further, perhaps on a bug report?</p>
<p style="margin-top:0; margin-bottom:0"><br>
</p>
<p style="margin-top:0; margin-bottom:0">Thanks,</p>
<p style="margin-top:0; margin-bottom:0">David Spickett.<br>
</p>
<p style="margin-top:0; margin-bottom:0"><br>
</p>
<hr tabindex="-1" style="display:inline-block; width:98%">
<div id="x_divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" color="#000000" style="font-size:11pt"><b>From:</b> Bryan Chan <bryan.chan@huawei.com><br>
<b>Sent:</b> 11 October 2018 23:48<br>
<b>To:</b> David Spickett; Renato Golin<br>
<b>Cc:</b> nd; Clang Dev<br>
<b>Subject:</b> RE: [llvm-dev] [RFC] New Clang target selection options for ARM/AArch64</font>
<div> </div>
</div>
<meta content="text/html; charset=us-ascii">
<meta name="x_Generator" content="Microsoft Word 15 (filtered medium)">
<div lang="EN-US">
<div class="x_x_WordSection1">
<p class="x_x_MsoNormal"><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:#1F497D">Hi David,</span></p>
<p class="x_x_MsoNormal"><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:#1F497D"> </span></p>
<p class="x_x_MsoNormal"><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:#1F497D">Yes, the assembly example works for me on AArch64 as well. The problematic use case for me was this:</span></p>
<p class="x_x_MsoNormal"><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:#1F497D"> </span></p>
<p class="x_x_MsoNormal" style="text-autospace:none"><span style="font-size:9.0pt; font-family:Consolas">$ cat vdot-c.c</span></p>
<p class="x_x_MsoNormal" style="text-autospace:none"><span style="font-size:9.0pt; font-family:Consolas">#include <arm_neon.h></span></p>
<p class="x_x_MsoNormal" style="text-autospace:none"><span style="font-size:9.0pt; font-family:Consolas"> </span></p>
<p class="x_x_MsoNormal" style="text-autospace:none"><span style="font-size:9.0pt; font-family:Consolas">uint32x2_t foo (uint32x2_t r, uint8x8_t x, uint8x8_t y)</span></p>
<p class="x_x_MsoNormal" style="text-autospace:none"><span style="font-size:9.0pt; font-family:Consolas">{</span></p>
<p class="x_x_MsoNormal" style="text-autospace:none"><span style="font-size:9.0pt; font-family:Consolas"> return vdot_u32 (r, x, y);</span></p>
<p class="x_x_MsoNormal" style="text-autospace:none"><span style="font-size:9.0pt; font-family:Consolas">}</span></p>
<p class="x_x_MsoNormal" style="text-autospace:none"><span style="font-size:9.0pt; font-family:Consolas"> </span></p>
<p class="x_x_MsoNormal" style="text-autospace:none"><span style="font-size:9.0pt; font-family:Consolas">$ ./bin/clang --target=aarch64-unknown-linux-gnu -march=armv8.4-a+dotprod -o /dev/null -c vdot-c.c</span></p>
<p class="x_x_MsoNormal"><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:#1F497D"> </span></p>
<p class="x_x_MsoNormal" style="text-autospace:none"><span style="font-size:9.0pt; font-family:Consolas">$ ./bin/clang --target=aarch64-unknown-linux-gnu -march=armv8.4-a -o /dev/null -c vdot-c.c</span></p>
<p class="x_x_MsoNormal" style="text-autospace:none"><b><span style="font-size:9.0pt; font-family:Consolas">vdot-c.c:5:10:
</span></b><span style="font-size:9.0pt; font-family:Consolas; color:#6C71C4">warning:
</span><b><span style="font-size:9.0pt; font-family:Consolas">implicit declaration of function 'vdot_u32' is invalid in C99 [-Wimplicit-function-declaration]</span></b></p>
<p class="x_x_MsoNormal" style="text-autospace:none"><span style="font-size:9.0pt; font-family:Consolas"> return vdot_u32 (r, x, y);</span></p>
<p class="x_x_MsoNormal" style="text-autospace:none"><span style="font-size:9.0pt; font-family:Consolas; color:#586E75"> ^</span></p>
<p class="x_x_MsoNormal" style="text-autospace:none"><b><span style="font-size:9.0pt; font-family:Consolas">vdot-c.c:5:10:
</span></b><span style="font-size:9.0pt; font-family:Consolas; color:#CB4B16">error:
</span><b><span style="font-size:9.0pt; font-family:Consolas">returning 'int' from a function with incompatible result type 'uint32x2_t' (vector of 2 'uint32_t' values)</span></b></p>
<p class="x_x_MsoNormal" style="text-autospace:none"><span style="font-size:9.0pt; font-family:Consolas"> return vdot_u32 (r, x, y);</span></p>
<p class="x_x_MsoNormal" style="text-autospace:none"><span style="font-size:9.0pt; font-family:Consolas; color:#586E75"> ^~~~~~~~~~~~~~~~~~</span></p>
<p class="x_x_MsoNormal" style="text-autospace:none"><span style="font-size:9.0pt; font-family:Consolas">1 warning and 1 error generated.</span></p>
<p class="x_x_MsoNormal" style="text-autospace:none"><span style="font-size:9.0pt; font-family:Consolas"> </span></p>
<p class="x_x_MsoNormal"><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:#1F497D">It seems wrong to me that clang doesn't call llvm::AArch64::getDefaultExtensions when handling -march=, but there might be a good reason for not doing so.</span></p>
<p class="x_x_MsoNormal"><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:#1F497D"> </span></p>
<p class="x_x_MsoNormal"><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:#1F497D">Thanks,</span></p>
<div>
<p class="x_x_MsoNormal"><span style="font-size:10.0pt; font-family:"Calibri",sans-serif; color:#1F497D">--<br>
Bryan Chan</span></p>
</div>
<p class="x_x_MsoNormal"><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:#1F497D"> </span></p>
<div>
<div style="border:none; border-top:solid #E1E1E1 1.0pt; padding:3.0pt 0in 0in 0in">
<p class="x_x_MsoNormal"><b><span style="font-size:11.0pt; font-family:"Calibri",sans-serif">From:</span></b><span style="font-size:11.0pt; font-family:"Calibri",sans-serif"> David Spickett [mailto:David.Spickett@arm.com]
<br>
<b>Sent:</b> Thursday, October 11, 2018 9:40 AM<br>
<b>To:</b> Bryan Chan <bryan.chan@huawei.com>; Renato Golin <renato.golin@linaro.org><br>
<b>Cc:</b> nd <nd@arm.com>; Clang Dev <cfe-dev@lists.llvm.org><br>
<b>Subject:</b> Re: [llvm-dev] [RFC] New Clang target selection options for ARM/AArch64</span></p>
</div>
</div>
<p class="x_x_MsoNormal"> </p>
<div id="x_x_divtagdefaultwrapper">
<p><span style="font-family:"Calibri",sans-serif; color:black">Hi Bryan,</span></p>
<p><span style="font-family:"Calibri",sans-serif; color:black"> </span></p>
<p><span style="font-family:"Calibri",sans-serif; color:black">Can you give some more detail on what you're seeing? I tried an assembly example and that works:</span></p>
<p><span style="font-family:"Calibri",sans-serif; color:black"> </span></p>
<div>
<div>
<p class="x_x_MsoNormal"><span style="font-family:"Calibri",sans-serif; color:black">$ cat /tmp/test.s<br>
vudot.u8 d0, d1, d2<br>
vsdot.s8 d0, d1, d2<br>
$ ./clang --target=arm-arm-none-eabi -march=armv8.4-a -c /tmp/test.s -o /tmp/test.o</span></p>
</div>
</div>
<div>
<p class="x_x_MsoNormal"><span style="font-family:"Calibri",sans-serif; color:black"> </span></p>
</div>
<div>
<p class="x_x_MsoNormal"><span style="font-family:"Calibri",sans-serif; color:black">Maybe you're referring to something higher level than that, or a combination of mcpu/march?</span></p>
</div>
<div>
<p class="x_x_MsoNormal"><span style="font-family:"Calibri",sans-serif; color:black"> </span></p>
</div>
<div>
<p class="x_x_MsoNormal"><span style="font-family:"Calibri",sans-serif; color:black">Thanks,</span></p>
</div>
<div>
<p class="x_x_MsoNormal"><span style="font-family:"Calibri",sans-serif; color:black">David Spickett.</span></p>
</div>
<p class="x_x_MsoNormal"><span style="font-family:"Calibri",sans-serif; color:black"> </span></p>
<div class="x_x_MsoNormal" align="center" style="text-align:center"><span style="font-family:"Calibri",sans-serif; color:black">
<hr width="98%" size="2" align="center">
</span></div>
<div id="x_x_divRplyFwdMsg">
<p class="x_x_MsoNormal"><b><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:black">From:</span></b><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:black"> Bryan Chan (Canada Research Centre) <</span><a href="mailto:bryan.chan@huawei.com"><span style="font-size:11.0pt; font-family:"Calibri",sans-serif">bryan.chan@huawei.com</span></a><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:black">><br>
<b>Sent:</b> 09 October 2018 18:41<br>
<b>To:</b> David Spickett; Renato Golin<br>
<b>Cc:</b> nd; Clang Dev<br>
<b>Subject:</b> RE: [llvm-dev] [RFC] New Clang target selection options for ARM/AArch64</span><span style="font-family:"Calibri",sans-serif; color:black">
</span></p>
<div>
<p class="x_x_MsoNormal"><span style="font-family:"Calibri",sans-serif; color:black"> </span></p>
</div>
</div>
<div>
<div>
<p class="x_x_xmsonormal"><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:#1F497D">Hi David et al.,</span><span style="font-family:"Calibri",sans-serif; color:black"></span></p>
<p class="x_x_xmsonormal"><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:#1F497D"> </span><span style="font-family:"Calibri",sans-serif; color:black"></span></p>
<p class="x_x_xmsonormal"><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:#1F497D">Great job on the proposal. I have recently run into the problem of mandatory features, and was actually contemplating a possible fix. It seems that currently
mandatory features in the various ARMv8.x architectures are not enabled in cfe by default, which is surprising and inconsistent with GCC's behavior. For example, -march=armv8.4-a does not turn on the dot-product extension. The cause is that we are ignoring
the ArchBaseExtensions bits in TargetParser.cpp unless -mcpu is specified. Was there a reason for handling -march like this?</span><span style="font-family:"Calibri",sans-serif; color:black"></span></p>
<p class="x_x_xmsonormal"><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:#1F497D"> </span><span style="font-family:"Calibri",sans-serif; color:black"></span></p>
<div>
<p class="x_x_xmsonormal" style="margin-bottom:12.0pt"><span style="font-size:10.0pt; font-family:"Calibri",sans-serif; color:#1F497D">--<br>
Bryan</span><span style="font-family:"Calibri",sans-serif; color:black"></span></p>
</div>
<p class="x_x_xmsonormal"><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:#1F497D"> </span><span style="font-family:"Calibri",sans-serif; color:black"></span></p>
<div>
<div style="border:none; border-top:solid #E1E1E1 1.0pt; padding:3.0pt 0in 0in 0in">
<p class="x_x_xmsonormal"><b><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:black">From:</span></b><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:black"> llvm-dev [</span><a href="mailto:llvm-dev-bounces@lists.llvm.org" style=""><span style="font-size:11.0pt; font-family:"Calibri",sans-serif">mailto:llvm-dev-bounces@lists.llvm.org</span></a><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:black">]
<b>On Behalf Of </b>David Spickett via llvm-dev<br>
<b>Sent:</b> Tuesday, September 25, 2018 10:54 AM<br>
<b>To:</b> Renato Golin <</span><a href="mailto:renato.golin@linaro.org"><span style="font-size:11.0pt; font-family:"Calibri",sans-serif">renato.golin@linaro.org</span></a><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:black">><br>
<b>Cc:</b> LLVM Dev <</span><a href="mailto:llvm-dev@lists.llvm.org"><span style="font-size:11.0pt; font-family:"Calibri",sans-serif">llvm-dev@lists.llvm.org</span></a><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:black">>; nd <</span><a href="mailto:nd@arm.com"><span style="font-size:11.0pt; font-family:"Calibri",sans-serif">nd@arm.com</span></a><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:black">>;
Clang Dev <</span><a href="mailto:cfe-dev@lists.llvm.org"><span style="font-size:11.0pt; font-family:"Calibri",sans-serif">cfe-dev@lists.llvm.org</span></a><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:black">><br>
<b>Subject:</b> Re: [llvm-dev] [RFC] New Clang target selection options for ARM/AArch64</span><span style="font-family:"Calibri",sans-serif; color:black"></span></p>
</div>
</div>
<p class="x_x_xmsonormal"><span style="font-family:"Calibri",sans-serif; color:black"> </span></p>
<div id="x_x_x_divtagdefaultwrapper">
<div>
<p class="x_x_xmsonormal"><span style="font-family:"Calibri",sans-serif; color:black">Hi Eli, Renato,<br>
<br>
Thanks for your feedback, there's a lot more to some of these things than I knew. I've addressed your points below.<br>
<br>
The overall summary is:<br>
- Start with converting the TargetParser to tableGen, with no user facing changes<br>
- Add warnings based on that, behind -Wall. Starting with command lines, since directives have
<br>
larger implications that need investigation<br>
<br>
Thanks,<br>
David Spickett.<br>
<br>
<br>
mandatory features<br>
==================<br>
<br>
>> Could you go into a bit more detail about mandatory features? I'm pretty sure people are using the extension functionality to turn off features which are technically mandatory according to the reference manual, like floating-point in armv8a.<br>
<br>
>> I'd be more comfortable if these weren't enabled by default, but were<br>
present in -Wall.<br>
<br>
It seems like a large portion of the architecturally invalid combinations have a technical reasoning. So I'd amend that point from:<br>
<br>
"- Emit a warning when a mandatory feature of the base architecture is enabled with '+extension', or disabled with '+noextension'. (and ignore the option)"<br>
<br>
to <br>
<br>
" - Emit a warning when a mandatory feature of the base architecture is enabled with '+extension', or disabled with '+noextension'."<br>
<br>
So the option doesn't change behaviour and as Renato suggested we don't make them errors by default. So it's visible if you want to check these things but it's not going to break existing code. Anyone who wants an error can always upgrade it if required.<br>
<br>
Use of Tablegen<br>
===============<br>
<br>
>> Maybe you could put it into some existing library, like libLLVMTarget.<br>
<br>
This would be a little easier but with what Renato said...<br>
<br>
>> Option 1 makes everyone pay the cost and can be a lot harder to make<br>
it flexible and "zero-cost".<br>
<br>
I think we want to stick with the second way. This (from what I understand) also helps with the goal of reusing existing tablegen.<br>
<br>
>> One additional goal we had in the past, when we first wrote the<br>
TargetParser was to use the *existing* target description table-gen<br>
files to generate the parser tables.<br>
<br>
I should have been more clear, "unify the list of extensions that command lines and asm directives use" is sort of the same thing just muddled. As discussed above I think we can achieve this, I'm sure I'll hit the same issues you did Renato but hopefully they
aren't showstoppers.<br>
<br>
"target" attribute (Eli)<br>
==================<br>
<br>
>> One thing this doesn't mention is clang's "target" attribute for functions; have you considered that at all?<br>
<br>
I hadn't, thanks for pointing that out.<br>
<br>
As far as I can tell, we only support cpu names via the target attribute:<br>
__attribute__((target("arch=cortex-a75")))<br>
Whereas this doesn't work:<br>
__attribute__((target("arch=armv8-a+crc")))<br>
<br>
We don't plan to add this as part of this work, but of course you could specify invalid combinations with a CPU and some combination of other directives and options. These would be warnings following the ones already mentioned.<br>
<br>
I need to do some more investigation to work out exactly what invalid combinations you could produce. So this will be a latter part of the work if at all.<br>
<br>
"Negative" backend features (Eli)<br>
===========================<br>
<br>
>> This seems mostly orthogonal? At least I mean, I guess it might make the translation from TargetParser features to LLVM features slightly easier, but it seems like there could be some unexpected implications, so I don't want to tie it to this change.<br>
<br>
Agreed, it would certainly be a separate patch. It might not be needed so I will work on the tablegen conversion without changing any of this and see how it goes.<br>
<br>
>> they're the only negative features that are relevant for TargetParser?<br>
<br>
Yes, the rest are for internal use aka not enabled by a specific option. For a particular CPU for example.<br>
<br>
'auto' FPU value (Renato)<br>
================<br>
<br>
>> I'd have assumed -mfpu is already "auto" by default. Or is this to<br>
>> just override a previous option?<br>
>><br>
>> ex: clang -mcpu cortex-a8 -mfpu vfp4 -mfpu auto -> defaults back to VFP3.<br>
<br>
I don't see any reference to this in the code or the docs, and clang something similair:<br>
./clang --target=arm-arm-none-eabi -mcpu=cortex-a8 -mfpu=vfp4 -mfpu=auto -c /tmp/test.c<br>
clang-8: error: the clang compiler does not support '-mfpu=auto'<br>
<br>
Maybe I'm missing something.<br>
<br>
ACLE macros (Renato)<br>
===========<br>
<br>
>> The base arch is armv8.4-a, the crypto extension turns on AES/SHA2/SHA3/SM4. The nosha2 disables SHA2/>SHA3 (since SHA3 is dependant on SHA2). Each of these features has an ACLE feature test macro, so Clang >needs to know that nosha2 also disables SHA3.<br>
<br>
>Is this complex logic done by GCC's front-end as well?<br>
<br>
I don't think so, you might be right there. We will look into exactly what GCC has implemented before making any moves here.<br>
<br>
Errors (Renato)<br>
======<br>
<br>
>> Errors:<br>
>> - unknown extension in an assembly directive (currently fails silently)<br>
>> IIRC, this is by design.<br>
<br>
If that's the case then we'll keep the behaviour. Again with warnings under -Wall.
<br>
<br>
My impression of it came more from me trying to work out what was a valid option at all. However if we can improve the documentation and consistency between directives and command line options that won't be an issue.<br>
<br>
>> Define "incompatible". Older Arm cores could have new features that<br>
wasn't even define in its own standard because manufacturers upgraded<br>
the extra but not the core.<br>
<br>
Good point, I suppose "incompatible" in the way I wrote it means "not listed as an off the shelf config". Which you're right, doesn't cover everything. So yes, agreed on defaulting to warnings behind -Wall.<br>
<br>
>> - mandatory feature of the base arch is enabled with '+' (option is redundant so is ignored)<br>
<br>
Agreed, also as discussed above it should not ignore the option. (unless it is actually a nop in that situation or completely impossible)<br>
<br>
>> .arch_extension was implemented because GCC does it. I'm not sure what<br>
you mean by that, but I'm not happy with removing it, as it will break<br>
scores of assembly files out there.<br>
<br>
I put it there to put the choice between being GCC compatible and being consistent within Clang itself. I've quickly realised that the former is more important.<br>
<br>
>> This makes sense, but will likely require changes in a lot of existing<br>
low-level assembly files, which choose a generic .cpu and vary<br>
.fpu/.arch_extension to implement independent functionality (like<br>
unwinders).<br>
<br>
Again I didn't know about that use case. It's definitely a later goal and I think there needs to be more investigation before we could make any changes.<br>
<br>
>> I strongly recommend that you do not change *any* user-facing<br>
behaviour until the underlying parser changes are done and released<br>
upstream.<br>
<br>
After what I've read here I'm fully on board with that.</span></p>
</div>
<p class="x_x_xmsonormal"><span style="font-family:"Calibri",sans-serif; color:black"> </span></p>
</div>
<div class="x_x_MsoNormal" align="center" style="text-align:center"><span style="font-family:"Calibri",sans-serif; color:black">
<hr width="98%" size="2" align="center">
</span></div>
<div id="x_x_x_divRplyFwdMsg">
<p class="x_x_xmsonormal"><b><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:black">From:</span></b><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:black"> Renato Golin <</span><a href="mailto:renato.golin@linaro.org"><span style="font-size:11.0pt; font-family:"Calibri",sans-serif">renato.golin@linaro.org</span></a><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:black">><br>
<b>Sent:</b> 24 September 2018 21:51:01<br>
<b>To:</b> David Spickett<br>
<b>Cc:</b> Clang Dev; LLVM Dev; nd<br>
<b>Subject:</b> Re: [llvm-dev] [RFC] New Clang target selection options for ARM/AArch64</span><span style="font-family:"Calibri",sans-serif; color:black">
</span></p>
<div>
<p class="x_x_xmsonormal"><span style="font-family:"Calibri",sans-serif; color:black"> </span></p>
</div>
</div>
<div>
<div>
<p class="x_x_xmsonormal"><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:black">On Fri, 21 Sep 2018 at 11:06, David Spickett via llvm-dev<br>
<</span><a href="mailto:llvm-dev@lists.llvm.org"><span style="font-size:11.0pt; font-family:"Calibri",sans-serif">llvm-dev@lists.llvm.org</span></a><span style="font-size:11.0pt; font-family:"Calibri",sans-serif; color:black">> wrote:<br>
> Below is a document detailing changes we'd like to make to Clang/LLVM to improve the usability of the target options for ARM and AArch64.<br>
<br>
Hi David,<br>
<br>
This is *awesome*. Thanks for such a detailed analysis!<br>
<br>
<br>
> In this RFC we propose changes to ARM and AArch64 target selection. With the top level goals to:<br>
> - validate that given options make sense within architectural restrictions<br>
> - make option discovery and documentation easier<br>
> - unify the list of extensions that command lines and asm directives use<br>
> - bring the options closer to GCC's where appropriate<br>
<br>
One additional goal we had in the past, when we first wrote the<br>
TargetParser was to use the *existing* target description table-gen<br>
files to generate the parser tables.<br>
<br>
This means new changes to cores, sub-arches, and fixes to existing<br>
ones will *automatically* be translated to command line and assembly<br>
parsing.<br>
<br>
<br>
<br>
> Proposed solution<br>
> ------------------<br>
><br>
> ARM and AArch64:<br>
> - Make the TargetParser the single source for extension names, removing the AsmParser tables.<br>
> - Reject unknown extension names with a diagnostic that includes a list of valid extensions for that architecture/CPU.<br>
> - Reject invalid combinations of architecture/CPU and extensions with an error diagnostic.<br>
> - Add independent subtarget features for each extension so that v8.x+1-a extensions can be used individually with earlier v8.x-a architectures where allowed.<br>
<br>
SGTM.<br>
<br>
> - Emit a warning when a mandatory feature of the base architecture is enabled with '+extension', or disabled with '+noextension'. (and ignore the option)<br>
> - Errors caused by the solution above should be able to be downgraded to warnings with the usual -W* options. This applies only to cases where there is a reasonable interpretation of the options chosen.<br>
<br>
I'd be more comfortable if these weren't enabled by default, but were<br>
present in -Wall.<br>
<br>
Writing generic and precise build systems is a nightmare, which is the<br>
biggest reason why compilers generally ignore nonsense options<br>
silently.<br>
<br>
<br>
> ARM:<br>
> - Allow all possible ARM extensions in the '.arch_extension' directive, without the '+' syntax<br>
> (allow them to be recognised, they could still be rejected for compatibility).<br>
> - Reject invalid mfpu and march/mcpu combinations with an error diagnostic.<br>
> - Reject invalid arch/cpu and extension combinations with an error diagnostic.<br>
<br>
SGTM.<br>
<br>
> - Add an 'auto' value for -mfpu and make it the default. Meaning that the FPU is implied by mcpu/march. If mfpu is not auto, it should override other options and a warning should be emitted.<br>
<br>
I'd have assumed -mfpu is already "auto" by default. Or is this to<br>
just override a previous option?<br>
<br>
ex: clang -mcpu cortex-a8 -mfpu vfp4 -mfpu auto -> defaults back to VFP3.<br>
<br>
<br>
<br>
> Optional features<br>
> -----------------<br>
><br>
> AArch64:<br>
> - add the '.arch_extension' directive, with the same behaviour as ARM (no '+', one extension per directive). This brings Clang in line with GCC which has this directive for both architectures. Clang does however allow you to achieve the same thing by using
'+' with '.arch'.<br>
><br>
> ARM:<br>
> - Allow '+' in '.arch' and '.cpu'. GCC does not allow this, but it would make ARM/AArch64 more consistent within Clang.<br>
<br>
I see no reason to be inconsistent with GNU tools here. We can have<br>
more, but we should not have less or different behaviour.<br>
<br>
<br>
> Use of Table-gen<br>
> ================<br>
><br>
> We think the benefits outweigh the disadvantages in this case.<br>
<br>
Agreed!<br>
<br>
<br>
> To do this, we would need to move TargetParser to break the cyclic dependency of LLVMSupport -> llvm-tblgen -> LLVMSupport. There are 2 options for this:<br>
> 1. create a new LLVMTargetParser library that contains all parsers for architectures that use it.<br>
> 2. put the TargetParser for each backend in the library group for that backend. This requires one of:<br>
> * Relaxing the requirement that target parsers must be built even if the backend is not.<br>
> * Modifying the CMake scripts to build the target parsers even if the backend is not being built.<br>
><br>
> Option 1 is simpler but option 2 would allow us to make use of the existing tablegen files in the backends so it is preferred.<br>
<br>
Option 1 makes everyone pay the cost and can be a lot harder to make<br>
it flexible and "zero-cost". This is the reason why it was changed<br>
from a class-based model to a static function / table model.<br>
<br>
I had a go at option 2 years ago and it works. You need to fiddle a<br>
bit with the CMake file in lib/Targets (to prepare the inc files even<br>
if targets aren't being built, because Clang needs to use it for all<br>
supported targets regardless).<br>
<br>
It wasn't upstreamed because the hard part is to re-use the existing<br>
table-gen files for a new back-end, which would generate the tables.<br>
Not so much writing the new back-end, but making sure the data we need<br>
isn't redundant or contradictory (which it was both) across all<br>
table-gen files. We also had to add new options to the targets (define<br>
new classes, etc) which were solely used by the parser, so were harder<br>
to justify on its own and needed a much more extensive validation than<br>
we had bandwidth for.<br>
<br>
<br>
> Consider this AArch64 march:<br>
> -march=armv8.4-a+crypto+nosha2<br>
><br>
> The base arch is armv8.4-a, the crypto extension turns on AES/SHA2/SHA3/SM4. The nosha2 disables SHA2/SHA3 (since SHA3 is dependant on SHA2). Each of these features has an ACLE feature test macro, so Clang needs to know that nosha2 also disables SHA3.<br>
<br>
Is this complex logic done by GCC's front-end as well?<br>
<br>
It would be pretty cool to have it smart like that, but we also have<br>
to be careful to have a rock solid model before improving on GCC's<br>
(potentially broken) functionality, and hopefully someone talking to<br>
them on the side.<br>
<br>
The amount of noise that comes every time we change the command line<br>
options interpretation is non-trivial. :)<br>
<br>
<br>
> Errors:<br>
> - unknown extension in an assembly directive (currently fails silently)<br>
<br>
IIRC, this is by design.<br>
<br>
Imagine a macro that defines .cpu in an asm file to multiple things,<br>
and the rest of the file has .fpu all over the place, with support for<br>
all .cpu options, but with the guarantee that those functions will<br>
only be compiled/executed if the .cpu is correct.<br>
<br>
This may sound weird, but some libraries (ex. unwind) actually depend<br>
on weird behaviour like that.<br>
<br>
<br>
> - extension incompatible with base arch, message shows the base arch it requires.<br>
> - extension requires another which is disabled later, message shows which one is required.<br>
> - extension requires another which is not enabled, message shows requirements.<br>
> - ARM mfpu option is not 'auto' and is incompatible with the base arch, message shows list of valid FPUs.<br>
<br>
Define "incompatible". Older Arm cores could have new features that<br>
wasn't even define in its own standard because manufacturers upgraded<br>
the extra but not the core.<br>
<br>
I'm happy to have errors for things that are impossible, like "ARMv5<br>
AArch64" or enabling and disabling intersecting groups that cannot be<br>
represented in the compiler.<br>
<br>
I'm happy to have warnings, possibly only under -Wall, for nonsense<br>
options like "ARMv5 VFP4" or "ARMv8A IWMMX".<br>
<br>
<br>
> Warnings:<br>
> - ARM mfpu option is not auto and another option implies a different FPU than the mfpu value. The mfpu value will be used, and the message will show what was overridden.<br>
<br>
This is nice.<br>
<br>
> - mandatory feature of the base arch is enabled with '+' (option is redundant so is ignored)<br>
<br>
Maybe under -Wall?<br>
<br>
> - mandatory feature of a base arch is disabled with '+no<feature>' (option makes no sense so the extension remains enabled)<br>
<br>
Arm is a flexible architecture, and build systems are crazy. This will<br>
likely confuse a lot of builds in the wild.<br>
<br>
I'd avoid it unless in -Wall.<br>
<br>
<br>
> .arch_extension Directive<br>
> =========================<br>
><br>
> We can handle this in a few of ways:<br>
> - Remove .arch_extension in favour of .arch. This conflicts with the option above to add it to AArch64 to bring us in line with GCC, and will break a lot of code written for older versions of Clang.<br>
<br>
.arch_extension was implemented because GCC does it. I'm not sure what<br>
you mean by that, but I'm not happy with removing it, as it will break<br>
scores of assembly files out there.<br>
<br>
> - Track the current base target, as implied by the command line or the last .arch/.cpu directive. This makes the directives as similar to the command lines as they can be without breaking backwards compatibility.<br>
<br>
This makes sense, but will likely require changes in a lot of existing<br>
low-level assembly files, which choose a generic .cpu and vary<br>
.fpu/.arch_extension to implement independent functionality (like<br>
unwinders).<br>
<br>
If you read the GNU manuals, the assembly directives is more to allow<br>
the assembler to relax checks than enforce them more.<br>
<br>
I personally like strong checks, but the problems we have with inline<br>
assembly will come crashing in assembly files if we start tightening<br>
the checks there, too.<br>
<br>
It's a worthy long goal, but it's a loooong goal and you don't want<br>
your current TargetParser work to depend on that.<br>
<br>
<br>
> $ ./clang --target=arm-arm-none-eabi -march=armv7-m -mfpu=neon-fp16 -c /tmp/test.c -o /tmp/test.o<br>
> (should be invalid but is allowed)<br>
><br>
> $ ./arm-eabi-gcc -march=armv7-m -mfpu=neon-fp16 -c /tmp/test.c -o /tmp/test.o<br>
> (same example given for Clang above, should be invalid)<br>
<br>
If both are allowed, I'd recommend you not to change it in this<br>
current pass. Let's get the parser fixed before changing overall<br>
behaviour.<br>
<br>
<br>
> Dependencies within extensions are not checked. For example crypto requires simd, but it can be disabled in the same march option.<br>
><br>
> $ ./clang --target=aarch64-arm-none-eabi -march=armv8-a+crypto+nosimd -c /tmp/test.c -o /tmp/test.o<br>
><br>
> Extensions are rejected if not recognised but not checked for compatibility. Hence the Clang crypto/simd example above is allowed with GCC too.<br>
><br>
> $ ./aarch64-elf-gcc -march=armv8-a+crypto+nosimd -c /tmp/test.c -o /tmp/test.o<br>
> (should not be allowed)<br>
<br>
This is unlikely to change, let alone in the time frame of your work.<br>
<br>
I strongly recommend that you do not change *any* user-facing<br>
behaviour until the underlying parser changes are done and released<br>
upstream.<br>
<br>
-- <br>
cheers,<br>
--renato</span><span style="font-family:"Calibri",sans-serif; color:black"></span></p>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</body>
</html>