Re: [PATCH 2/2] vect: Make partial trapping ops use predication [PR96373]

[Richard Sandiford <richard.sandiford@arm.com>]

"Kewen.Lin" <linkw@linux.ibm.com> writes:
> on 2023/2/13 21:57, Richard Sandiford wrote:
>> "Kewen.Lin" <linkw@linux.ibm.com> writes:
>>> Hi Richard,
>>>
>>> on 2023/1/27 19:08, Richard Sandiford via Gcc-patches wrote:
>>>> PR96373 points out that a predicated SVE loop currently converts
>>>> trapping unconditional ops into unpredicated vector ops.  Doing
>>>> the operation on inactive lanes can then raise an exception.
>>>>
>>>> As discussed in the PR trail, we aren't 100% consistent about
>>>> whether we preserve traps or not.  But the direction of travel
>>>> is clearly to improve that rather than live with it.  This patch
>>>> tries to do that for the SVE case.
>>>>
>>>> Doing this regresses gcc.target/aarch64/sve/fabd_1.c.  I've added
>>>> -fno-trapping-math for now and filed PR108571 to track it.
>>>> A similar problem applies to fsubr_1.d.
>>>>
>>>> I think this is likely to regress Power 10, since conditional
>>>> operations are only available for masked loops.  I think we'll
>>>> need to add -fno-trapping-math to any affected testcases,
>>>> but I don't have a Power 10 system to test on.  Kewen, would you
>>>> mind giving this a spin and seeing how bad the fallout is?
>>>>
>>>
>>> Sorry for the late reply, I'm just back from vacation.
>>>
>>> Thank you for fixing this and caring about Power10!
>>>
>>> I tested your proposed patch on one Power10 machine (ppc64le),
>>> it's bootstrapped but some test failures got exposed as below.
>>>
>>> < FAIL: gcc.target/powerpc/p9-vec-length-epil-1.c scan-assembler-times \\\\mlxvl\\\\M 14
>>> < FAIL: gcc.target/powerpc/p9-vec-length-epil-1.c scan-assembler-times \\\\mstxvl\\\\M 7
>>> < FAIL: gcc.target/powerpc/p9-vec-length-epil-2.c scan-assembler-times \\\\mlxvl\\\\M 20
>>> < FAIL: gcc.target/powerpc/p9-vec-length-epil-2.c scan-assembler-times \\\\mstxvl\\\\M 10
>>> < FAIL: gcc.target/powerpc/p9-vec-length-epil-3.c scan-assembler-times \\\\mlxvl\\\\M 14
>>> < FAIL: gcc.target/powerpc/p9-vec-length-epil-3.c scan-assembler-times \\\\mstxvl\\\\M 7
>>> < FAIL: gcc.target/powerpc/p9-vec-length-epil-4.c scan-assembler-times \\\\mlxvl\\\\M 70
>>> < FAIL: gcc.target/powerpc/p9-vec-length-epil-4.c scan-assembler-times \\\\mlxvx?\\\\M 120
>>> < FAIL: gcc.target/powerpc/p9-vec-length-epil-4.c scan-assembler-times \\\\mstxvl\\\\M 70
>>> < FAIL: gcc.target/powerpc/p9-vec-length-epil-4.c scan-assembler-times \\\\mstxvx?\\\\M 70
>>> < FAIL: gcc.target/powerpc/p9-vec-length-epil-5.c scan-assembler-times \\\\mlxvl\\\\M 21
>>> < FAIL: gcc.target/powerpc/p9-vec-length-epil-5.c scan-assembler-times \\\\mstxvl\\\\M 21
>>> < FAIL: gcc.target/powerpc/p9-vec-length-epil-5.c scan-assembler-times \\\\mstxvx?\\\\M 21
>>> < FAIL: gcc.target/powerpc/p9-vec-length-epil-6.c scan-assembler-times \\\\mlxvl\\\\M 10
>>> < FAIL: gcc.target/powerpc/p9-vec-length-epil-6.c scan-assembler-times \\\\mlxvx?\\\\M 42
>>> < FAIL: gcc.target/powerpc/p9-vec-length-epil-6.c scan-assembler-times \\\\mstxvl\\\\M 10
>>> < FAIL: gcc.target/powerpc/p9-vec-length-epil-8.c scan-assembler-times \\\\mlxvl\\\\M 16
>>> < FAIL: gcc.target/powerpc/p9-vec-length-epil-8.c scan-assembler-times \\\\mstxvl\\\\M 7
>>> < FAIL: gcc.target/powerpc/p9-vec-length-full-1.c scan-assembler-not \\\\mlxvx\\\\M
>>> < FAIL: gcc.target/powerpc/p9-vec-length-full-1.c scan-assembler-not \\\\mstxvx\\\\M
>>> < FAIL: gcc.target/powerpc/p9-vec-length-full-1.c scan-assembler-times \\\\mlxvl\\\\M 20
>>> < FAIL: gcc.target/powerpc/p9-vec-length-full-1.c scan-assembler-times \\\\mstxvl\\\\M 10
>>> < FAIL: gcc.target/powerpc/p9-vec-length-full-2.c scan-assembler-not \\\\mlxvx\\\\M
>>> < FAIL: gcc.target/powerpc/p9-vec-length-full-2.c scan-assembler-not \\\\mstxvx\\\\M
>>> < FAIL: gcc.target/powerpc/p9-vec-length-full-2.c scan-assembler-times \\\\mlxvl\\\\M 20
>>> < FAIL: gcc.target/powerpc/p9-vec-length-full-2.c scan-assembler-times \\\\mstxvl\\\\M 10
>>> < FAIL: gcc.target/powerpc/p9-vec-length-full-3.c scan-assembler-times \\\\mlxvl\\\\M 14
>>> < FAIL: gcc.target/powerpc/p9-vec-length-full-3.c scan-assembler-times \\\\mstxvl\\\\M 7
>>> < FAIL: gcc.target/powerpc/p9-vec-length-full-4.c scan-assembler-not \\\\mlxvx\\\\M
>>> < FAIL: gcc.target/powerpc/p9-vec-length-full-4.c scan-assembler-not \\\\mstxv\\\\M
>>> < FAIL: gcc.target/powerpc/p9-vec-length-full-4.c scan-assembler-not \\\\mstxvx\\\\M
>>> < FAIL: gcc.target/powerpc/p9-vec-length-full-4.c scan-assembler-times \\\\mlxvl\\\\M 70
>>> < FAIL: gcc.target/powerpc/p9-vec-length-full-4.c scan-assembler-times \\\\mstxvl\\\\M 70
>>> < FAIL: gcc.target/powerpc/p9-vec-length-full-5.c scan-assembler-not \\\\mlxvx\\\\M
>>> < FAIL: gcc.target/powerpc/p9-vec-length-full-5.c scan-assembler-not \\\\mstxv\\\\M
>>> < FAIL: gcc.target/powerpc/p9-vec-length-full-5.c scan-assembler-not \\\\mstxvx\\\\M
>>> < FAIL: gcc.target/powerpc/p9-vec-length-full-5.c scan-assembler-times \\\\mlxvl\\\\M 21
>>> < FAIL: gcc.target/powerpc/p9-vec-length-full-5.c scan-assembler-times \\\\mstxvl\\\\M 21
>>> < FAIL: gcc.target/powerpc/p9-vec-length-full-6.c scan-assembler-times \\\\mlxvl\\\\M 10
>>> < FAIL: gcc.target/powerpc/p9-vec-length-full-6.c scan-assembler-times \\\\mstxvl\\\\M 10
>>> < FAIL: gcc.target/powerpc/p9-vec-length-full-6.c scan-assembler-times \\\\mstxvx?\\\\M 6
>>> < FAIL: gcc.target/powerpc/p9-vec-length-full-8.c scan-assembler-times \\\\mlxvl\\\\M 30
>>> < FAIL: gcc.target/powerpc/p9-vec-length-full-8.c scan-assembler-times \\\\mstxvl\\\\M 10
>>>
>>> By checking several of them, it's due to that we don't vectorize
>>> some loop having float type involved with partial vector any more.
>>>
>>> As you suggested above, I fixed them with an extra option
>>> "-fno-trapping-math" and verified all of them can pass again.
>>> I also noticed that the original test case in PR96373 fails
>>> on Power10 too, so I added one constructed case pr96373.c
>>> into sub bucket gcc.target/powerpc for testing coverage
>>> on Power.
>>>
>>> One re-spin with the attached adjustment shows there is no
>>> regression failure any more, and the new test case works well
>>> on both ppc64 (P8) and ppc64le (P10) Linux.
>> 
>> Thanks for doing this.  The patch is OK, if you need approval.
>> I'll push mine once it's in.
>
> Thanks for the review!  Pushed in r13-5978-g4f5a1198065dc0.

Thanks, I've now pushed the vectoriser patch.

> btw, do we want this to be backported?  If yes, I'm going to
> backport it to gcc-12 and gcc-11 branches soon (for gcc-10 we
> don't have partial vector support on Power btw).

Yeah, for SVE it'll need to go on all active branches.  I'm going
to be off until 27th Feb so I'll start backporting after that.

Richard