| Message ID | 1508182699-10436-7-git-send-email-martin@martin.st |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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;