[GASPP,6/6] Work around an armasm64 bug in the scale operand to fcvtzs/scvtf

Message ID 1508182699-10436-7-git-send-email-martin@martin.st
State Superseded
Headers show

Commit Message

Martin Storsjö Oct. 16, 2017, 7:38 p.m.
The operand shouldn't be stored as is, but stored as 64-scale, in
the opcode, but armasm64 misses to do this.

This might be a big enough bug to report and try to get fixed, but
that requires removing this workaround at that point.
---
 gas-preprocessor.pl | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Janne Grunau Oct. 18, 2017, 7:02 a.m. | #1
On 2017-10-16 22:38:19 +0300, Martin Storsjö wrote:
> The operand shouldn't be stored as is, but stored as 64-scale, in
> the opcode, but armasm64 misses to do this.
> 
> This might be a big enough bug to report and try to get fixed, but
> that requires removing this workaround at that point.

Please report this as bug. I'd propose a environment variable or version 
check for this fixup.

> ---
>  gas-preprocessor.pl | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/gas-preprocessor.pl b/gas-preprocessor.pl
> index d9eaf1d..182b684 100755
> --- a/gas-preprocessor.pl
> +++ b/gas-preprocessor.pl
> @@ -1032,6 +1032,16 @@ sub handle_serialized_line {
>                  $line =~ s/$instr$suffix/${instr}u$suffix/;
>                }
>              }
> +
> +            # Instructions like fcvtzs and scvtf store the scale value
> +            # inverted in the opcode (stored as 64 - scale), but armasm64
> +            # stores it as-is. Thus convert from "fcvtzs w0, s0, #8"
> +            # into "fcvtzs w0, s0, #56".
> +            if ($line =~ /(?:fcvtzs|scvtf)\s+(\w+)\s*,\s*(\w+)\s*,\s*#(\d+)/) {
> +                my $scale = $3;
> +                my $inverted_scale = 64 - $3;
> +                $line =~ s/#$scale/#$inverted_scale/;
> +            }
>          }
>          # armasm is unable to parse &0x - add spacing
>          $line =~ s/&0x/& 0x/g;

The fixup itself is ok

Janne
Martin Storsjö Oct. 18, 2017, 7:10 a.m. | #2
On Wed, 18 Oct 2017, Janne Grunau wrote:

> On 2017-10-16 22:38:19 +0300, Martin Storsjö wrote:
>> The operand shouldn't be stored as is, but stored as 64-scale, in
>> the opcode, but armasm64 misses to do this.
>> 
>> This might be a big enough bug to report and try to get fixed, but
>> that requires removing this workaround at that point.
>
> Please report this as bug.

I've reported it at 
https://connect.microsoft.com/VisualStudio/feedback/details/3142655/armasm64-encodes-incorrect-scale-for-fcvtzs-and-scvtf 
(seems to require sign-in to see) but apparently they are switching to 
some other system so I have to re-report it elsewhere.

> I'd propose a environment variable or version check for this fixup.

Yeah, I thought about that. At this point within gas-preprocessor, we 
don't really know the version of armasm (and executing it to find out 
would be pretty expensive). One possible solution is to enclose the 
hook-up of the affected functions in x264 with something like this:

#if !defined(_MSC_FULL_VER) || _MSC_FULL_VER >= FIXED_VERSION

That assumes that the assembler is updated in sync with the compiler, 
which I guess is reasonable.

Alternatively we'd use something like the existing GASPP_FIX_XCODE5 env 
var, requiring setting GASPP_FIX_ARMASM_FP_SCALE or something such. If the 
bug report progress shows that it might not ever be fixed, we could remove 
the need for the env variable of course.

// Martin

Patch

diff --git a/gas-preprocessor.pl b/gas-preprocessor.pl
index d9eaf1d..182b684 100755
--- a/gas-preprocessor.pl
+++ b/gas-preprocessor.pl
@@ -1032,6 +1032,16 @@  sub handle_serialized_line {
                 $line =~ s/$instr$suffix/${instr}u$suffix/;
               }
             }
+
+            # Instructions like fcvtzs and scvtf store the scale value
+            # inverted in the opcode (stored as 64 - scale), but armasm64
+            # stores it as-is. Thus convert from "fcvtzs w0, s0, #8"
+            # into "fcvtzs w0, s0, #56".
+            if ($line =~ /(?:fcvtzs|scvtf)\s+(\w+)\s*,\s*(\w+)\s*,\s*#(\d+)/) {
+                my $scale = $3;
+                my $inverted_scale = 64 - $3;
+                $line =~ s/#$scale/#$inverted_scale/;
+            }
         }
         # armasm is unable to parse &0x - add spacing
         $line =~ s/&0x/& 0x/g;