guest_s390_toIR.c has: case 0xb9e1: /* POPCNT */ goto unimplemented; And indeed: $ cat popcnt.c int main (int argc, const char **argv) { return __builtin_popcountl ((long)argc); } $ gcc --version gcc (GCC) 4.8.3 20140911 (Red Hat 4.8.3-9) $ gcc popcnt.c $ valgrind ./a.out vex s390->IR: unimplemented insn: B9E1 0011 ==2443== valgrind: Unrecognised instruction at address 0x80000564. ==2443== at 0x80000564: main (in /home/mwielaar/a.out) ==2443== Your program just tried to execute an instruction that Valgrind ==2443== did not recognise. There are two possible reasons for this. ==2443== 1. Your program has a bug and erroneously jumped to a non-code ==2443== location. If you are running Memcheck and you just saw a ==2443== warning about a bad jump, it's probably your program's fault. ==2443== 2. The instruction is legitimate but Valgrind doesn't handle it, ==2443== i.e. it's Valgrind's fault. If you think this is the case or ==2443== you are not sure, please let us know and we'll try to fix it. ==2443== Either way, Valgrind will now raise a SIGILL signal which will ==2443== probably kill your program. ==2443== ==2443== Process terminating with default action of signal 4 (SIGILL) ==2443== Illegal opcode at address 0x80000564 ==2443== at 0x80000564: main (in /home/mwielaar/a.out) Dump of assembler code for function main: 0x0000000080000540 <+0>: stmg %r11,%r15,88(%r15) 0x0000000080000546 <+6>: lay %r15,-176(%r15) 0x000000008000054c <+12>: lgr %r11,%r15 0x0000000080000550 <+16>: lgr %r1,%r2 0x0000000080000554 <+20>: stg %r3,160(%r11) 0x000000008000055a <+26>: st %r1,172(%r11) 0x000000008000055e <+30>: lgf %r1,172(%r11) => 0x0000000080000564 <+36>: popcnt %r1,%r1 0x0000000080000568 <+40>: sllg %r2,%r1,32 0x000000008000056e <+46>: agr %r1,%r2 0x0000000080000572 <+50>: sllg %r2,%r1,16 0x0000000080000578 <+56>: agr %r1,%r2 0x000000008000057c <+60>: sllg %r2,%r1,8 0x0000000080000582 <+66>: agr %r1,%r2 0x0000000080000586 <+70>: srlg %r1,%r1,56 0x000000008000058c <+76>: lgfr %r1,%r1 0x0000000080000590 <+80>: lgr %r2,%r1 0x0000000080000594 <+84>: lg %r4,288(%r11) 0x000000008000059a <+90>: lmg %r11,%r15,264(%r11) 0x00000000800005a0 <+96>: br %r4 End of assembler dump. Reproducible: Always
Created attachment 97264 [details] Implement popcnt for s390x Suggested fix for this Bug.
Thanks. Works for me on both the original program (libgccjit) and the simple reproducer. BTW. I believe the patch is missing an update to none/tests/s390x/Makefile.am to make sure the new test is build and the vgtest file is added. Index: none/tests/s390x/Makefile.am =================================================================== --- none/tests/s390x/Makefile.am (revision 15402) +++ none/tests/s390x/Makefile.am (working copy) @@ -11,7 +11,7 @@ ex_sig ex_clone cu14 cu14_1 cu41 fpconv ecag fpext fpext_warn \ rounding-1 rounding-2 rounding-3 rounding-4 rounding-5 bfp-1 \ bfp-2 bfp-3 bfp-4 srnm srnmb comp-1 comp-2 exrl tmll tm stmg \ - ex clst mvc test_fork test_sig rounding-6 rxsbg\ + ex clst mvc test_fork test_sig rounding-6 rxsbg popcnt \ spechelper-alr spechelper-algr \ spechelper-slr spechelper-slgr \ spechelper-cr spechelper-clr \
(In reply to Andreas Arnez from comment #1) > Created attachment 97264 [details] > Implement popcnt for s390x > > Suggested fix for this Bug. Thanks for the patch. As Mark said: none/tests/s390x/Makefile.am needs adjustment. Otherwise the popcnt test does not get built during "make check". In opcodes.h you define POPCNT. Good! Why not use it in popcnt.c? Can you add a check_popcnt call in popcnt.c where all input bits are 1? We also need configury to check whether the machine has the POPCNT insn. And only if the machine supports that opcode we should build the test. Other than that the patch looks good.
(In reply to Florian Krohm from comment #3) > Thanks for the patch. > As Mark said: none/tests/s390x/Makefile.am needs adjustment. Otherwise the > popcnt test does not get built during "make check". Right, I missed to include that change. Thanks to Mark for pointing out. > In opcodes.h you define POPCNT. Good! Why not use it in popcnt.c? Just a matter of taste. I prefer leaving the choice of operand registers to the compiler, like you would do in production code. > Can you add a check_popcnt call in popcnt.c where all input bits are 1? Sure. Any particular reason for testing this specific value? > We also need configury to check whether the machine has the POPCNT insn. And > only if the machine supports that opcode we should build the test. Not sure about that one. The patch always emulates the instruction, independent of the host capabilities. Do you think that should change? Wouldn't that be inconsistent with setting facility bit 45? > Other than that the patch looks good. Thanks for reviewing!
Created attachment 97268 [details] Implement popcnt for s390x (v2) Updated patch: - add popcnt to Makefile.am; - test all-one value; - enable s390x_features to check for facility bit 45.
(In reply to Andreas Arnez from comment #4) > > In opcodes.h you define POPCNT. Good! Why not use it in popcnt.c? > Just a matter of taste. I prefer leaving the choice of operand registers to > the compiler, like you would do in production code. I've added opcodes.h mainly to deal with certain issues in old binutils versions. I don't recall exactly what those problems were. > Can you add a check_popcnt call in popcnt.c where all input bits are 1? > Sure. Any particular reason for testing this specific value? I like to test boundary values. > > We also need configury to check whether the machine has the POPCNT insn. And > > only if the machine supports that opcode we should build the test. > Not sure about that one. The patch always emulates the instruction, > independent of the host capabilities. Right! Not sure what I was thinking. Thanks for the respin. I've added the patch as r15792 and VEX r3210