Re: [ARM] VFP11 denorm erratum workaround in linker

[Nick Clifton <nickc at redhat dot com>]

Hi Julian,

> We've worked around this erratum in the linker

Hmm, I was going to argue against this, but when I reviewed my thoughts 
I realised that they came down to the fact that I would rather a large 
patch like this, with its potential for introducing new bugs, be put 
into gcc rather than the linker :-)

For the record though, let me state that I do not like patches like 
this, when they are liable to introduce new bugs, and they are being 
used work around somebody else's problem that they cannot/will not fix. 
  I appreciate that working around hardware bugs is a necessary evil, I 
just do not like it very much.

> GCC at present only outputs scalar code for VFP units, so the default 
> (pre-ARMv7 architectures) is to fix scalar code. The workaround can be 
> disabled or selected for vector operations using the linker options:

Why would you want to only fix scalar code, but not fix vector code ? 
Just because gcc does not currently produce any, there is no guarantee 
that there will not be any in the final link.  Surely the safe default 
should be to fix both ?

>   --vfp11-denorm-fix={none,scalar,vector}

Shouldn't that be:

   --vfp11-denorm-fix={none,scalar,vector,both}

Or does "vector" mode imply "scalar" as well ?

I am also worried about the future.  Suppose a new revision of the VFP 
unit comes out (in less than v7 architecture) which does not have this 
problem.  Being able to disable this workaround on the command line is 
all very well, but what if the user forgets, or does not know about it ? 
 I would not be at all surprised to receive bug reports in the future 
saying "why is your stupid linker creating all these branches to some 
.vfp11_veneer section?".  To my mind hardware workarounds should be 
disabled by default and only enabled by users who know that they are 
working with broken hardware.  (This also emphasises the fact that the 
software is working around broken hardware, so do not blame the software 
for any reduction in performance).

> The workaround works by scanning input code sections for 
> potentially-troublesome code sequences, and replacing them by branches 
> to a specially-constructed veneer (similar to the way ARM<->Thumb 
> interworking stubs are created at link-time). The veneer contains the 
> original instruction plus a branch back to the original subsequent 
> instruction, which is sufficient to avoid the erratum in either scalar 
> or vector mode.

In a really large program, could these branches go out of range ?

> Tested with cross to arm-none-eabi, and also with the GCC testsuite 
> (C/C++/libstdc++) with -mfpu=vfp/-mfloat-abi=softfp.

Did you also run the gas/ld/binutils testsuites ?


> + /* Information about a VFP11 erratum veneer, or a branch to such a veneer.
> +    The type can be:
> +      'a': branch to ARM veneer
> +      't': branch to Thumb veneer
> +      'A': ARM veneer
> +      'T': Thumb veneer.  */

> +   char type;

I do not see much point in 'type' being a char.  An enum would be better 
(and self-documenting).  I doubt that you would save any space by using 
the char as the structure is likely to be padded out to an int boundary 
anyway.

> + /* Return a VFP register number.  

*sigh* All this code is one of the reasons why I would much rather have 
this fix in the compiler.  The compiler knows which instructions are 
being generated and it knows the difference between code and data, so a 
lot of the work that you are doing here can be avoided.  (Hand coded 
assembler aside.  But if you hand code assembler you can hand code 
workarounds).


> + /* Look for potentially-troublesome code sequences which might trigger the
> +    VFP11 denormal/antidependency erratum.  See, e.g., the ARM1136 errata sheet
> +    (available from ARM) 

It would be very nice if you could include a URL for the errata sheet 
here.  Assuming that ARM make the sheet available for direct download.

>  In short, the erratum
> +    can trigger if an instruction uses denorm operands (bouncing to support
> +    code), and a subsequent instruction overwrites one of those operands.
> +    In some circumstances, the second instruction can be issued before the
> +    first is handled, leading to incorrect results.
> +    
> +    We can work around the erratum by replacing (at link time) the first
> +    instruction with a branch to a veneer containing the original instruction
> +    and a branch back to the following instruction.  The erratum is avoided if:
> +    
> +      * In scalar mode, there is one unrelated instruction between the bouncing
> +        instruction and the instruction which overwrites its operands.
> + 
> +      * In vector mode, if there are two unrelated instructions between the
> +        first and second instructions.
> +    
> +    The erratum is only relevant in full IEEE compatibility mode, but we can't
> +    tell if that is needed at this point by ourselves. If you know you don't
> +    need the workaround, pass --vfp11-denorm-fix=none to the linker.  */


This information really needs to be in the documentation.  In fact you 
need to add documentation of the new switch to ld.texinfo and a mention 
of it to ld/NEWS as well.

One other thing - I think that it would be a good idea to include a 
linker testsuite entry to check this new feature.

Cheers
  Nick