Hi Richard,
Recapping on this earlier conversation:
http://lists.linaro.org/pipermail/linaro-toolchain/2010-July/000030.html http://lists.linaro.org/pipermail/linaro-toolchain/2010-July/000035.html
Is it worth another attempt to make a case to upstream for supporting passing -mimplicit-it=thumb by default to gas?
According to my understanding of this issue, my argument would go as follows:
* gcc currently estimates the size of asm blocks, rather than determining the size accurately. * gcc cannot guarantee the right answer for asm block size when asm blocks contain directives etc., however use of directives in asm blocks is widespread * gcc cannot guarantee the right answer for asm block size in Thumb-2. gcc conservatively overestimates the size by assuming that each statement of the asm block expands to 4 bytes. * All of Ubuntu lucid and maverick has been built with -mimplicit-it=thumb passwd by default, with no known build or runtime failures arising from this (so size issues aside, we have confidence that the resulting code generation is sound) * -mimplicit-it=thumb -mthumb makes the asm block size estimation unsafe: the asm block can exceed the estimated size even in the absence of directives, which may lead to fixup range errors during assembly. * Following the principles already established for Thumb-2 in general the estimation can be made safe (or, as safe as the established Thumb-2 behaviour) by raising the assumed maximum statement expansion size for asm blocks to 6 bytes, since -mimplicit-it will add as most a single (16-bit) IT instruction to each statement. * The vast majority of all asm blocks are small (< 20 instructions, say), so the overall overestimate in sizes will generally be modest for any given compilation unit. * -mimplicit-it is already _required_ by the Linux kernel and possible other projects.
...so...
* With -mimplicit-it=thumb and a 6-byte asm block statement expansion size estimate, we have toolchain behaviour which is as reliable, and as correct, as it is in upstream at present. * Layout of data in the compiler output will be more optimal in some cases, and less optimal in other cases, compared with the the current Thumb-2 behaviour, due to differing asm block size estimates. The exact behaviour will depend on the distribution of conditional instructions within asm blocks. * Taken over a whole compilation unit, the total code size overestimate (and therefore the impact on object layout) will normally be modest, due to the small typical size of asm blocks. * Behaviour for -marm will not be impacted at all.
If gcc currently estimated asm block code size accurately, then I could understand upstream's objection; but as it stands it seems to me we wouldn't be making anything worse in practice with the proposed change; and there is no compatibility impact (other than positive impact).
Of course, I may have some wrong assumptions here, or there may be some background I'm not aware of...
Comments?
Cheers ---Dave
Dnia piątek, 12 listopada 2010 o 18:33:03 Dave Martin napisał(a):
- -mimplicit-it is already required by the Linux kernel and
possible other projects.
Qt and KDE4 require -mimplicit-it=thumb too. It is disabled in Ubuntu gcc-4.5 and as a result it does not build on armel without setting it in debian/rules (qt4 has it done, kde waits for decision of toolchain maintainers).
Regards,
On 13.11.2010 08:20, Marcin Juszkiewicz wrote:
Dnia piątek, 12 listopada 2010 o 18:33:03 Dave Martin napisał(a):
- -mimplicit-it is already required by the Linux kernel and
possible other projects.
Qt and KDE4 require -mimplicit-it=thumb too. It is disabled in Ubuntu gcc-4.5 and as a result it does not build on armel without setting it in debian/rules (qt4 has it done, kde waits for decision of toolchain maintainers).
is it the same failure as in qt4? If yes, this should be fixed in the qt4 headers. In the past, Dave Martin did an analysis of assembler instructions in packages. Is this still a goal for Linaro to address these issues, and if not, who will address these?
Dave pointed out that (unrelated) bugs like #490371 currently are not tracked.
Matthias
On Sat, Nov 13, 2010 at 7:20 AM, Marcin Juszkiewicz marcin.juszkiewicz@linaro.org wrote:
Dnia piątek, 12 listopada 2010 o 18:33:03 Dave Martin napisał(a):
* -mimplicit-it is already required by the Linux kernel and possible other projects.
Qt and KDE4 require -mimplicit-it=thumb too. It is disabled in Ubuntu gcc-4.5 and as a result it does not build on armel without setting it in debian/rules (qt4 has it done, kde waits for decision of toolchain maintainers).
The difference is that in Linux, -mimplicit-it is sort of officially required (actually, I believe this toolchain feature got added as a consequence of implementing CONFIG_THUMB2_KERNEL)
In Qt, we require -mimplicit-it by accident - the maintainers do not explicitly support building for Thumb-2. This will be the case for most projects we encounter.
Cheers ---Dave
In general the product should move forward and drop work-arounds like -mimplicit-it. We (the greater ARM community) should fix these package problems as they are found. Here's a bunch of quick-fire statements:
* Qt is currently broken on ARM multiprocessor systems * Qt provides a QAtomic class which provides atomic functions on integers and pointers * You can call different functions depending on your ordering constraints * GCC's __sync primitives are ordered only * The ARMv6 QAtomic implementation only provides ordered versions * AVR32 currently uses the sync primitives in the same way
So there's a precedent there for __sync primitives and they'd be equivalent to what is already there. If RVDS also supports __sync_* (Richard?) then we can also delete the RVDS specific code.
The kernel adds -mimplicit-it via its own build rules. What else needs -mimplicit-it? Matthias, are you running a rebuild at the moment? How many extra packages have failed?
I had a play with the Maverick GCC-4.5 and binutils. This code:
void foo() { asm("it eq\n\t" "teqeq r2,#1" ); }
compiles fine with -marm, -mthumb, and -march=armv4t -marm. It seems the assembler accepts IT instructions in both unified and compatibility modes.
-- Michael
On Sat, Nov 13, 2010 at 6:33 AM, Dave Martin dave.martin@linaro.org wrote:
Hi Richard,
Recapping on this earlier conversation:
http://lists.linaro.org/pipermail/linaro-toolchain/2010-July/000030.html http://lists.linaro.org/pipermail/linaro-toolchain/2010-July/000035.html
Is it worth another attempt to make a case to upstream for supporting passing -mimplicit-it=thumb by default to gas?
According to my understanding of this issue, my argument would go as follows:
* gcc currently estimates the size of asm blocks, rather than determining the size accurately. * gcc cannot guarantee the right answer for asm block size when asm blocks contain directives etc., however use of directives in asm blocks is widespread * gcc cannot guarantee the right answer for asm block size in Thumb-2. gcc conservatively overestimates the size by assuming that each statement of the asm block expands to 4 bytes. * All of Ubuntu lucid and maverick has been built with -mimplicit-it=thumb passwd by default, with no known build or runtime failures arising from this (so size issues aside, we have confidence that the resulting code generation is sound) * -mimplicit-it=thumb -mthumb makes the asm block size estimation unsafe: the asm block can exceed the estimated size even in the absence of directives, which may lead to fixup range errors during assembly. * Following the principles already established for Thumb-2 in general the estimation can be made safe (or, as safe as the established Thumb-2 behaviour) by raising the assumed maximum statement expansion size for asm blocks to 6 bytes, since -mimplicit-it will add as most a single (16-bit) IT instruction to each statement. * The vast majority of all asm blocks are small (< 20 instructions, say), so the overall overestimate in sizes will generally be modest for any given compilation unit. * -mimplicit-it is already _required_ by the Linux kernel and possible other projects.
...so...
* With -mimplicit-it=thumb and a 6-byte asm block statement expansion size estimate, we have toolchain behaviour which is as reliable, and as correct, as it is in upstream at present. * Layout of data in the compiler output will be more optimal in some cases, and less optimal in other cases, compared with the the current Thumb-2 behaviour, due to differing asm block size estimates. The exact behaviour will depend on the distribution of conditional instructions within asm blocks. * Taken over a whole compilation unit, the total code size overestimate (and therefore the impact on object layout) will normally be modest, due to the small typical size of asm blocks. * Behaviour for -marm will not be impacted at all.
If gcc currently estimated asm block code size accurately, then I could understand upstream's objection; but as it stands it seems to me we wouldn't be making anything worse in practice with the proposed change; and there is no compatibility impact (other than positive impact).
Of course, I may have some wrong assumptions here, or there may be some background I'm not aware of...
Comments?
Cheers ---Dave
linaro-toolchain mailing list linaro-toolchain@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-toolchain
On Wed, Nov 17, 2010 at 2:53 AM, Michael Hope michael.hope@linaro.org wrote:
In general the product should move forward and drop work-arounds like -mimplicit-it. We (the greater ARM community) should fix these package problems as they are found. Here's a bunch of quick-fire statements:
* Qt is currently broken on ARM multiprocessor systems * Qt provides a QAtomic class which provides atomic functions on integers and pointers * You can call different functions depending on your ordering constraints * GCC's __sync primitives are ordered only * The ARMv6 QAtomic implementation only provides ordered versions * AVR32 currently uses the sync primitives in the same way
So there's a precedent there for __sync primitives and they'd be equivalent to what is already there. If RVDS also supports __sync_* (Richard?) then we can also delete the RVDS specific code.
The kernel adds -mimplicit-it via its own build rules. What else needs -mimplicit-it? Matthias, are you running a rebuild at the moment? How many extra packages have failed?
I had a play with the Maverick GCC-4.5 and binutils. This code:
void foo() { asm("it eq\n\t" "teqeq r2,#1" ); }
compiles fine with -marm, -mthumb, and -march=armv4t -marm. It seems the assembler accepts IT instructions in both unified and compatibility modes.
Interesting... it this is indeed true then it could provide a way forward. I don't know which binutils versions will support this -- Richard may be able to comment.
Note that -mimplicit-it is a sort of anti-workaround: I believe the design intent of the unified assembler syntax was that programmers should not normally write IT explicitly (since that obviously makes code harder to port and maintain ... as we're discovering). So the native behaviour of the RVCT assembler is the like the implicit-it behaviour, and gas (and GCC inline asm) are incompatible with this behaviour by default.
Of course, there's no technical reason why the behaviour has to be the same across different toolchains, but since the alternative is to patch all the inline assembler in the world ... and out-of-line assembler too if we want to migrate that to thumb ... and since the kernel at least is unlikely to do this, it does at least seem worth trying to make the case to upstream.
Of course, it's possible this has all been tried before and will be a foregone conclusion... so if people still disagree then that's fair enough.
If all gas that we care about accept IT instructions in separated syntax mode, then this does at least make it feasible to put the ITs in manually without resorting to preprocessor hacks. But it may still be a lot of work.
Cheers ---Dave
On Wed, Nov 17, 2010 at 11:22 AM, Dave Martin dave.martin@linaro.org wrote:
On Wed, Nov 17, 2010 at 2:53 AM, Michael Hope michael.hope@linaro.org wrote:
In general the product should move forward and drop work-arounds like -mimplicit-it. We (the greater ARM community) should fix these package problems as they are found. Here's a bunch of quick-fire
[...]
Having discussed this further with Richard, it sounds like there are enough issues blocking -mimplicit-it upstream that we should not expect it to be supported by default upstream in the foreseeable future:
* -mimplicit-it disables some important sanity-checking on the compiler output (by not checking compiler-generated ITs, or the absence thereof). We could in principle make the assembler only inject ITs in between #APP and #NO_APP, but the assembler doesn't support this yet; nor does any arm gcc I've seen systematically generate these directives for inline asms; so implementing this would probably result in a flag day when everyone has to move to up-to-date gcc and gas. Upstreams are unlikely to go for that.
* with -mimplicit-it, the compiler must be pretty conservative about inline asm block size (assuming 6 byte per statement) - that's feasible, but very suboptimal and is likely to result in the need for yet another compiler option to turn it on; again, this is unlikely to become the default upstream.
* add-hoc workarounds can be used, such as wrapping GCC to compile in multiple passes so that the correct inline asm size for each block can be determined. But such approaches are likely to be too cumbersome to get merged in any project.
So I've now come round to the view that we _should_ probably bite the bullet and fix the inline asm directly. So:
* We need to verify which binutils permit (and ignore) the IT instructions in non-unified (ARM) syntax. I've observed that 2.19.1 definitely supports this; I don't know about earlier versions -- this is probably something the toolchain group should investigate. * We should be proactive about making these changes upstream. Writing some standard wording to explain the reason for the change and the likely impact would probably be a good idea.
Cheers ---Dave
On Mon, 22 Nov 2010, Dave Martin wrote:
So I've now come round to the view that we _should_ probably bite the bullet and fix the inline asm directly. So:
- We need to verify which binutils permit (and ignore) the IT
instructions in non-unified (ARM) syntax. I've observed that 2.19.1 definitely supports this; I don't know about earlier versions -- this is probably something the toolchain group should investigate.
- We should be proactive about making these changes upstream.
Writing some standard wording to explain the reason for the change and the likely impact would probably be a good idea.
I hope there is at least a validation of the IT instructions by the assembler with regards to the condition codes on the following instructions (and vice versa) to make sure they are all coherent, and even so for ARM mode compilation.
Nicolas
Hi,
On Mon, Nov 22, 2010 at 2:39 PM, Nicolas Pitre nicolas.pitre@linaro.org wrote:
[...]
I hope there is at least a validation of the IT instructions by the assembler with regards to the condition codes on the following instructions (and vice versa) to make sure they are all coherent, and even so for ARM mode compilation.
In unified syntax, yes; in traditional syntax, no.
When driving gas via GCC, this means that the checking is done only when building for Thumb-2 (since traditional syntax is always used for ARM code generated by GCC, and this probably isn't going to change for now). In traditional syntax, at least some binutils versions totally ignore IT instructions.
However, for code generated by GCC itself (i.e., not inline asm), GCC is supposed to generate the correct IT instructions when generating Thumb-2 code. -mimplicit-it may therefore cause some code generation errors to go unnoticed, particularly where the compiler misses out an IT instruction which it was supposed to insert, but the assembler silently inserts the missing instruction.
Cheers ---Dave
On Mon, Nov 22, 2010 at 11:35 PM, Dave Martin dave.martin@linaro.org wrote:
On Wed, Nov 17, 2010 at 11:22 AM, Dave Martin dave.martin@linaro.org wrote:
On Wed, Nov 17, 2010 at 2:53 AM, Michael Hope michael.hope@linaro.org wrote:
In general the product should move forward and drop work-arounds like -mimplicit-it. We (the greater ARM community) should fix these package problems as they are found. Here's a bunch of quick-fire
Just to finish this thread off, the following decisions were made: * -mimplicit-it will not be passed to the assembler by default * Linaro as a whole and Linaro Foundations in particular will fix any problems upstream as found
-- Michael
Hi,
On Tue, Dec 14, 2010 at 3:24 AM, Michael Hope michael.hope@linaro.org wrote:
On Mon, Nov 22, 2010 at 11:35 PM, Dave Martin dave.martin@linaro.org wrote:
On Wed, Nov 17, 2010 at 11:22 AM, Dave Martin dave.martin@linaro.org wrote:
On Wed, Nov 17, 2010 at 2:53 AM, Michael Hope michael.hope@linaro.org wrote:
In general the product should move forward and drop work-arounds like -mimplicit-it. We (the greater ARM community) should fix these package problems as they are found. Here's a bunch of quick-fire
Just to finish this thread off, the following decisions were made: * -mimplicit-it will not be passed to the assembler by default * Linaro as a whole and Linaro Foundations in particular will fix any problems upstream as found
Just to add to this, we need to work with the Ubuntu guys to ensure that all problems caused by this compiler change are tracked.
I've seen a few bugs going past where the Ubuntu maintainers quickly "fix" the affected package by building with -marm. This is OK for getting the package building in the short term, but we need to be careful to ensure these problems aren't forgotten about.
Secondly, the people maintaining the affected upstream projects may not be builing for Thumb-2 regularly-- this means that packages may break again due to upstream maintenance. In rare cases, maintenance changes which cause incorrect execution in Thumb-2 might not cause build failures. We need to be vigilant about this, and accompany changes pushed upstream with comments advising how to avoid this.
I expect the most practical approach is to write up advice on the wiki or somewhere, and refer developers to it, especially linaro and ubuntu-arm guys. I can have a go at drafting that, but it may not happen until January...
Cheers ---Dave
linaro-toolchain@lists.linaro.org
-
Dave Martin -
Marcin Juszkiewicz -
Matthias Klose -
Michael Hope -
Nicolas Pitre