Bug 489338 - arm64: Instruction fcvtas should round 322.5 to 323, but result is 322.
Summary: arm64: Instruction fcvtas should round 322.5 to 323, but result is 322.
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: vex (other bugs)
Version First Reported In: 3.20.0
Platform: Debian unstable Linux
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-06-27 21:16 UTC by Bernhard Übelacker
Modified: 2024-07-01 05:55 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
Minimal reproducer: fp-valgrind-test.c (718 bytes, text/x-csrc)
2024-06-27 21:16 UTC, Bernhard Übelacker
Details
0001-arm64-Fix-fcvtas-instruction.patch (14.22 KB, patch)
2024-06-27 21:24 UTC, Bernhard Übelacker
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Bernhard Übelacker 2024-06-27 21:16:29 UTC
Created attachment 171101 [details]
Minimal reproducer: fp-valgrind-test.c

Hello, I was investigating a debian bug, where the continuous integration failed
because it receives a different value when running with valgrind. [1071656]
I could track it down to a fcvtas instruction.

  => 0xaaaaaaaa4948 <ConvertToRational+104>:     fcvtas  w5, d8
(With $d8=322.5)

With plain GDB:
  (gdb) print $w5
  $2 = 323

With valgrind+GDB:
  (gdb) print $w5
  $2 = 322

Attached is a minimal reproducer.

[1071656] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1071656
Comment 1 Bernhard Übelacker 2024-06-27 21:24:20 UTC
Created attachment 171102 [details]
0001-arm64-Fix-fcvtas-instruction.patch

I tried to look into it and I think the issue is the `fcvtas` gets stored in the intermediate representation with Irrm_NEAREST.
When this gets translated back to the native instruction i guess it results in a `fcvtns`.

Attached patch tries to preserve this difference by using Irrm_NEAREST_TIE_AWAY_0,
at least the reproducer shows with it no longer a difference.
Comment 2 Paul Floyd 2024-06-28 08:00:27 UTC
Looks good. One problem with the regtests is that a lot of the numeric tests just use random number generators. That means that they don't cover rounding with ties very well.

This also needs doing for the vector version, as noted here

         case 3: ch = 'a'; irrm = Irrm_NEAREST; break; /* kludge? */

I'll see if I can also knock up a test that covers all 8 combinations of float/double and signed/unsigned scalar.
Comment 3 Paul Floyd 2024-07-01 05:55:14 UTC
Pushed with a small change to use True and False rather than 1 and 0.

I'll add a couple of tests as well.

commit de4c79ffbcd2d5e89495cee8feadf77d5f3a6ef2 (HEAD -> master, origin/master, origin/HEAD)
Author: Bernhard <C3><9C>belacker <bernhardu@mailbox.org>
Date:   Thu Jun 27 22:51:09 2024 +0200

    arm64: Fix fcvtas instruction.