Created attachment 108391 [details] ARMv7 NEON test program The attached test program gives the wrong output when run with the memcheck tool. The attached patch makes the issue disappear. The issue depends on how valgrind is built. With the right combination of compiler and compiler flags (e.g. using -mfpu=neon makes a difference here with GCC 7.2.0), the helper functions in memcheck/mc_main.c end up using the same (caller-save) VFP registers as used by VEX. According to http://infocenter.arm.com/help/topic/com.arm.doc.ihi0042f/IHI0042F_aapcs.pdf (§5.1.2.1 VFP register usage conventions): VFP registers d16-d31 (q8-q15), if present, do not need to preserved (by the callee).
Created attachment 108392 [details] [PATCH] VEX/ARM: mark caller-save VFP registes as trashed by calls
The comment in getRRegUniverse_ARM() should be more explicit about which registers are caller save and callee save. And if this is indeed the case, then the patch needs to take care of supplying different set of Q registers for register allocator use. Callee saved registers are preferred over caller saved ones.
(In reply to Ivo Raisr from comment #2) > The comment in getRRegUniverse_ARM() should be more explicit about which > registers are caller save and callee save. > > And if this is indeed the case, then the patch needs to take care of > supplying different set of Q registers for register allocator use. Callee > saved registers are preferred over caller saved ones. The callee-save registers are used up for S and D registers (which alias with the Q registers). Can we first get this fixed and then look at eventually improving the register selection? (Or do you have a suggestion for a better set of registers?)
Created attachment 108396 [details] [PATCH] VEX/ARM: mark caller-save VFP registes as trashed by calls
Your fix touches register allocation. It is crucial that both ARMInstr_Call() and getRRegUniverse_ARM() are kept in sync (hinted in getRRegUniverse_ARM as well) and that register allocator is presented with the workable set of registers. By marking all Q ones as caller saved (trashed for call), register allocator would need to spill them all before the call. This creates a performance penalty and bloats the generated r-code. Would you try running Memcheck on a program which uses 128-bit VFP (q) registers with '--stats=yes' and note the difference on 'ratio' reported? How prevalent are programs utilizing 128-bit VFP registers compared to 64-bit ones? In other words, are compilers (gcc) likely to utilize those registers a lot? A clue for your answer might lie in the document you cited first: "Registers s16-s31 (d8-d15, q4-q7) must be preserved across subroutine calls; registers s0-s15 (d0-d7, q0-q3) do not need to be preserved (and can be used for passing arguments or returning results in standard procedure-call variants). Registers d16-d31 (q8-q15), if present, do not need to be preserved."
Q registers should be mostly relevant for NEON (often intrinsics or assembly routines with limited control flow). I ran memcheck with and without the change on a fairly NEON-intensive component (mostly DSP and neural networks), and see little performance difference, if any, (Cortex A57 running 32-bit code, which may seem like a weird choice, but it was convenient). I have not looked at the stats, but the output is as follows.
Created attachment 108397 [details] Stats before
Created attachment 108398 [details] Stats after
(In reply to Sindre Aamås from comment #6) > I have not looked at the stats, but the output is as follows. As suspected, there is a code bloat in the generated code because of the additional spilling before helper calls. Ratio before: 15.9 Ratio after: 16.0
The reason why I ran across this is likely that our toolchain has -mfpu=neon as default, which may be uncommon, which would explain why this has not been reported previously.
We do have a number of test programs under none/tests/arm which are supposed to test VFP. Please could you have a look if your test program would belong into one of these; otherwise add it there. Then post the results of running the regression test suite before and after your changes are applied. I will then take your patch in.