Re: [avr-gcc-list] Avr-libc-user-manual: "Problems with reordering code"

[David Brown]

You could file this as a bug on the website:
<https://savannah.nongnu.org/bugs/?group=avr-libc>

As far as I understand it, the documentation (both on the website and
the Atmel documentation) is generated directly from the library code and
comments - so this would be a change to the library source.

Feel free to quote any parts of my mails on the subject when filing the bug.

mvh.,

David


On 09/02/17 13:11, Marcin Godlewski wrote:
> Dear All,
> 
> The site
> http://www.nongnu.org/avr-libc/user-manual/optimization.html#optim_code_reorder/optimization_1optim_code_reorder.html
> still contains buggy description of memory barriers in avr-gcc. As
> this site is popular among avr users I think it's really worth
> fixing. What is more the same inaccurate article is available on
> Atmel doc site:
> http://www.atmel.com/webdoc/AVRLibcReferenceManual/optimization_1optim_code_reorder.html
> . Is there anybody subscribed to this mailing list who can contact
> the authors/maintainers of the site in order to discuss correction of
> the content?
> 
> Marcin Godlewski
> 
> W dniu 2016-12-10 23:25:17 użytkownik Marcin Godlewski
> <address@hidden> napisał:
>> W dniu 2016-12-09 10:11:55 użytkownik David Brown
>> <address@hidden> napisał:
>>> On 08/12/16 21:46, Georg-Johann Lay wrote:
>>>> Marcin Godlewski schrieb:
>>>>> Dear all,
>>>>> 
>>>>> Thanks for the reply to David. However I'm not trying to find
>>>>> a solution for the described issue. What I'm trying to say in
>>>>> this e-mail is that this part of Atmel documentation: 
>>>>> http://www.atmel.com/webdoc/AVRLibcReferenceManual/optimization_1optim_code_reorder.html
>>>>>
>>>>> 
is innacurate and should be corrected. The conclusion says:
>>>>> 
>>>>> memory barriers ensure proper ordering of volatile accesses
>>>>> 
>>>>> memory barriers don't ensure statements with no volatile
>>>>> accesses to be reordered across the barrier while it should
>>>>> say:
>>>>> 
>>>>> memory barriers ensure proper ordering of global variables
>>>>> accesses
>>>>> 
>>>>> memory barriers don't ensure local variables accesses to be 
>>>>> reordered across the barrier
>>>> 
>>>> At least the "local" vs. "global" is not completely correct.
>>>> After all it's about memory accesses, and it doesn't matter if
>>>> the memory is local (e.g. local static) or if you are
>>>> dereferencing a pointer (which might point to a local auto or
>>>> to an object on heap).
>>>> 
>>>> The code example you quoted above is actually due to a subtle 
>>>> implementation detail of division, modulo and some other
>>>> arithmetic of GCC's avr backend (the division is _not_ a call
>>>> actually).
>>>> 
>>>> IIRC the solution for me back then was -fno-tree-ter as any
>>>> messing with inline asm doesn't hit the point.
>>> 
>>> Yes, that is the solution you proposed when we discussed it a
>>> good while back (on the avrlibc list, I think).  I disagree with
>>> you somewhat here (as I did then, though I can't remember if we
>>> discussed the details).
>>> 
>>> Changing an optimisation option like this means that code that
>>> looks right, will run as expected - and that is a good thing.
>>> But it also means that the code will /only/ be correct if
>>> particular optimisation flags are set in particular ways.  That
>>> is a very fragile situation, and should always be avoided.  To be
>>> safe, this optimisation would have to be completely disabled in
>>> the avr-gcc port.  I don't know how useful this particular
>>> optimisation is in terms of generating more efficient code,
>>> though from the gcc manual it appears very useful and is enabled 
>>> at -O1.  Clearly that determines the "cost" of this solution to
>>> the re-ordering problem.
>>> 
>>> 
>>> The use of the assembly dependency (or a nicely named macro with
>>> the same effect) fixes the problem in this situation.  It does so
>>> regardless of optimisation options - the compiler is required to
>>> have calculated the result of the division before disabling
>>> interrupts, and cannot re-order the operations.  It does so
>>> without adding any extra assembly code or hindering any
>>> optimisations - it merely forces an order on operations that are
>>> to be done anyway.
>>> 
>>> It has the clear disadvantage of needing extra code in the
>>> user's source.  Like memory barriers, it is a way of giving the
>>> compiler extra information that cannot be expressed in normal C,
>>> and which the compiler cannot (at the moment) figure out for
>>> itself.
>>> 
>>> You say that the assembly dependency does not "hit the point".  I
>>> think you are correct there - it is treating the symptom, not the
>>> disease.  It is not telling the compiler that an operation should
>>> not be re-ordered, or that division is a costly operation.  It
>>> simply tells the compiler that we need the results of that
>>> computation /here/.  But it is a very effective and efficient
>>> cure for this sort of problem.  Unless and until there is some
>>> /safe/ fix in the compiler to avoid this (and I don't count "put
>>> this compiler option in your command line" as safe), I really do
>>> think it is the best we have.
>>> 
>>> 
>>> Note, however, that the "forceDependency" macro only solves half
>>> the problem.  Consider :
>>> 
>>> unsigned int test2b(void) { unsigned int val;
>>> 
>>> cli(); val = ivar; sei(); val = 65535 / val; return val; }
>>> 
>>> In this case, the compiler could move the division backwards
>>> above the sei(), giving a similar problem.  (It did not make the
>>> move in my brief tests - but it /could/ do.)  I don't know if the
>>> -fno-tree-ter flag stops that too, but the forceDependency()
>>> macro is not enough.  The forgetCompilerKnowledge macro is the
>>> answer:
>>> 
>>> unsigned int test2b(void) { unsigned int val;
>>> 
>>> cli(); val = ivar; sei(); asm volatile ("" : "+g" (val)); val =
>>> 65535 / val; return val; }
>>> 
>>> This tells the compiler that it needs to stabilise the value of
>>> "val", and it can't assume anything about "val" after this point
>>> in the code, because it /might/ be read and /might/ change in the
>>> assembly code. Again, nothing is actually generated in the
>>> assembly and we are only forcing an ordering on the code.
>>> 
>>> 
>>> Nothing would please me better here here than to have the
>>> compiler understand that users would not want such re-ordering
>>> around cli() and sei(), so that the problem simply goes away.
>>> But it should not require particular choices of compiler flags,
>>> nor should it require disabling useful optimisations and thus
>>> generating poorer code elsewhere.
>>> 
>>> It is also worth noting that though this situation occurs
>>> because division does not work like a normal function call,
>>> despite it using a library call for implementation, there is
>>> nothing fundamental to stop the compiler moving a call to foo()
>>> back or forth across a cli() or sei() as long as the compiler is
>>> sure that no memory is accessed, no volatiles are accessed, and
>>> there are no other externally visible effects in foo().  If the
>>> definition of foo() is available when compiling the code, then
>>> the compiler could well know this to be the case.  If we replace
>>> "val = 65535U / val;" with "val = foo(val);", where we have :
>>> 
>>> unsigned int foo(unsigned int v) { return (v * v) - v; }
>>> 
>>> in the same compilation unit, some or all of the calculation from
>>> foo() will be inlined and mixed with the cli().  Again,
>>> -fno-tree-ter fixes this - at the cost of killing such mixing and
>>> optimisation in cases where it is useful.  And again, the inline
>>> assembly fixes it at the cost of knowing that you have to add
>>> this line of source code.
>>> 
>>> As gcc gets ever smarter with its inlining, function cloning,
>>> link-time optimisations, etc., then this will become more and
>>> more of an issue.
>>> 
>>> 
>>> 
>>> Maybe the answer is that gcc needs an "execution barrier" that
>>> is stronger than a memory barrier, because it will also block
>>> calculations - it would act as a barrier to all local variables.
>>> I cannot think of any way to make such a barrier with inline
>>> assembly or the C11 fences - I think it would take a new
>>> __builtin for gcc.  Such a feature would have use on all embedded
>>> targets, not just AVR.
>>> 
>>> mvh.,
>>> 
>>> David
>>> 
>>> 
>> 
>> I totally agree with you - a feature like "execution barrier" would
>> be very useful. C11 made good job standardizing multi-threading
>> features but unfortunately the features not always fits firmware
>> development. Controlling what exactly goes into critical section is
>> a fundamental problem, so I would even go further - why don't you
>> propose the "execution barrier" as a new feature for the future C
>> language standard?
>> 
>>> 
>>>> 
>>>> Johann
>>>> 
>>>>> I don't know whether this group is the right place to post it
>>>>> however I do not know any better place. Hope someone here can
>>>>> trigger the change of the documentation and I also hope to be
>>>>> corrected if I am wrong.
>>>>> 
>>>>> Thanks and regards, Marcin
>>> 
>>> 
>> 
>> 
>> 
>> 
> 
> 
>