Bug 508661 - ppc64le: Testcase none/tests/ppc64/test_isa_3_1_VRT assserts
Summary: ppc64le: Testcase none/tests/ppc64/test_isa_3_1_VRT assserts
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: vex (other bugs)
Version First Reported In: unspecified
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-08-23 21:53 UTC by Florian Krohm
Modified: 2025-10-18 16:34 UTC (History)
2 users (show)

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


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Florian Krohm 2025-08-23 21:53:26 UTC
With current git ffc66d9dcdc2

Testcase none/tests/ppc64/test_isa_3_1_VRT asserts in function dis_vx_quadword_arith 
because of this:

putVReg( vT_addr, binop( Iop_ModS128, mkexpr( ......

where binop produces an IRExpr of type Ity_I128 but PutVReg expects that
expression to have type Ity_V128.

The code has been like this since affeb57ccd 2021-01-22. Has this ever worked?

Here's the reproducer:

__attribute__((altivec(vector__))) unsigned long long vrt, vra, vrb, vrc;

static void test_vdivesq (void) {
  __asm__ __volatile__ ("vdivesq %0, %1, %2"
    : "=v" (vrt) : "v" (vra), "v" (vrb) );
}

int main()
{
  test_vdivesq();
  return 0;
}

On a POWER10 machine (or newer):
gcc -c test.c
valgrind --tool=none ./a.out

vex: priv/guest_ppc_toIR.c:1717 (putVReg): Assertion `typeOfIRExpr(irsb->tyenv, e) == Ity_V128' failed.
vex storage: T total 86151576 bytes allocated
vex storage: P total 192 bytes allocated

valgrind: the 'impossible' happened:
   LibVEX called failure_exit().

host stacktrace:
==918931==    at 0x58038798: show_sched_status_wrk (m_libcassert.c:426)
==918931==    by 0x5803896F: report_and_quit (m_libcassert.c:497)
==918931==    by 0x58038BB7: vgPlain_core_panic_at (m_libcassert.c:572)
==918931==    by 0x58038BEB: vgPlain_core_panic (m_libcassert.c:582)
==918931==    by 0x58055BC7: failure_exit (m_translate.c:761)
==918931==    by 0x5815D47B: vex_assert_fail (main_util.c:245)
==918931==    by 0x5818C69F: putVReg (guest_ppc_toIR.c:1717)
==918931==    by 0x581EC5A7: dis_vx_quadword_arith.constprop.0 (guest_ppc_toIR.c:29043)
==918931==    by 0x582251AB: disInstr_PPC (guest_ppc_toIR.c:37816)
==918931==    by 0x58182B3B: disassemble_basic_block_till_stop (guest_generic_bb_to_IR.c:956)
==918931==    by 0x58184E4B: bb_to_IR (guest_generic_bb_to_IR.c:1451)
==918931==    by 0x5815AFDB: LibVEX_FrontEnd (main_main.c:617)
==918931==    by 0x5815BA3F: LibVEX_Translate (main_main.c:1293)
==918931==    by 0x5805965B: vgPlain_translate (m_translate.c:1835)
==918931==    by 0x5801AA23: vgPlain_scheduler (scheduler.c:1144)
==918931==    by 0x580ADC07: run_a_thread_NORETURN (syswrap-linux.c:102)
Comment 1 Florian Krohm 2025-08-24 10:20:30 UTC
vdiveuq, vdivsq, vdivuq, vmodsq, vmoduq  also fail in the same spot.
Comment 2 Carl Love 2025-10-15 19:15:02 UTC
I was able to rerproduce the error.  I tracked the failure down to the commit:

From 644d68e9501dd5679194dd5c8e0d3ce24764a1d8 Mon Sep 17 00:00:00 2001
From: Florian Krohm <flo2030@eich-krohm.de>
Date: Wed, 9 Jul 2025 20:15:46 +0000
Subject: [PATCH] Fix operand / result types of Iop_DivU128[E], Iop_ModU128 and
 their signed counterparts

In libvex_ir.h these IROps are described to operate on Ity_I128 operands and produce a like typed result. This 
contradicts the specification in ir_defs.c
(function typeOfprimop) which claims Ity_V128 for operands and result.

Above IROps are used exclusively by ppc for the following opcodes:
Iop_DivU128  --> vdivuq  Vector Divide Unsigned Quadword
Iop_DivS128  --> vdivsq  Vector Divide Signed Quadword
Iop_DivU128E --> vdiveuq Vector Divide Extended Unsigned Quadword
Iop_DivS128E --> vdivesq Vector Divide Extended Signed Quadword
Iop_ModU128  --> vmoduq  Vector Modulo Unsigned Quadword
Iop_ModS128  --> vmodsq  Vector Modulo Signed Quadword

Reading the ISA document, it is clear, that those opcodes perform an
integer division / modulo operation. Technically, they work on vector
registers, presumably because vector registers are the only resource
wide enough to store a quadword. Perhaps that is where the confusion
comes from.
So Ity_I128 it is.
---
 VEX/priv/ir_defs.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/VEX/priv/ir_defs.c b/VEX/priv/ir_defs.c
index 9e7fbf920..ba61758d7 100644
--- a/VEX/priv/ir_defs.c
+++ b/VEX/priv/ir_defs.c
@@ -3694,10 +3694,12 @@ void typeOfPrimop ( IROp op,
       case Iop_MulI128by10E:
       case Iop_MulI128by10ECarry:
       case Iop_PwExtUSMulQAdd8x16:
-      case Iop_DivU128: case Iop_DivS128:
+         BINARY(Ity_V128,Ity_V128, Ity_V128);
+
+      case Iop_DivU128:  case Iop_DivS128:
       case Iop_DivU128E: case Iop_DivS128E:
       case Iop_ModU128:  case Iop_ModS128:
-         BINARY(Ity_V128,Ity_V128, Ity_V128);
+         BINARY(Ity_I128,Ity_I128, Ity_I128);
 
       case Iop_2xMultU64Add128CarryOut:
       case Iop_Perm8x16x2:
-- 
2.50.1

Yes the instructions do integer divisions of 128-bit values that are stored in the V128 registers.  The Iops need to operate on the v128 type not on the I128 type due to the register type where they are stored.  The underlying PPC support for the various Iops use the corresponding PPC instruction that takes the values from the V128 register and treats the contents as an integer.

The patch needs to be reversed to fix the issue.

Not sure why the change did not show up in the nightly regression runs.  Will look into that.
Comment 3 Mark Wielaard 2025-10-17 17:48:22 UTC
Was this resolved by:

commit 3c3262fc702e829fc13192d1ca1de1e2de8987b3
Author: Florian Krohm <flo2030@eich-krohm.de>
Date:   Thu Oct 16 14:23:01 2025 +0000

    Revert 644d68e9501dd5679194dd5c8e0d3ce24764a1d8
    
    That change broke none/tests/ppc64/test_isa_3_1_VRT. Not good.
    Add a comment as to why a vector type is needed.

Or do we need some followup change?
Comment 4 Florian Krohm 2025-10-18 16:34:52 UTC
Fixed in 3c3262fc702e829fc13192d1ca1de1e2de8987b3 which reverts the guilty 644d68e9501dd5679194dd5c8e0d3ce24764a1d8.

No NEWS entry because valgrind effectively did not change.