> On 11 Oct 2024, at 12:08, Richard Sandiford <richard.sandif...@arm.com> wrote:
>
> External email: Use caution opening links or attachments
>
>
> Tamar Christina <tamar.christ...@arm.com> writes:
>>> -----Original Message-----
>>> From: Richard Biener <rguent...@suse.de>
>>> Sent: Friday, October 11, 2024 7:52 AM
>>> To: Richard Sandiford <richard.sandif...@arm.com>
>>> Cc: Jennifer Schmitz <jschm...@nvidia.com>; gcc-patches@gcc.gnu.org; Richard
>>> Earnshaw <richard.earns...@arm.com>; Kyrylo Tkachov
>>> <ktkac...@nvidia.com>; Tamar Christina <tamar.christ...@arm.com>
>>> Subject: Re: [PATCH][PR113816] AArch64: Use SIMD+GPR for logical vector
>>> reductions
>>>
>>> On Thu, 10 Oct 2024, Richard Sandiford wrote:
>>>
>>>> Jennifer Schmitz <jschm...@nvidia.com> writes:
>>>>> This patch implements the optabs reduc_and_scal_<mode>,
>>>>> reduc_ior_scal_<mode>, and reduc_xor_scal_<mode> for ASIMD modes V8QI,
>>>>> V16QI, V4HI, and V8HI for TARGET_SIMD to improve codegen for bitwise
>>> logical
>>>>> vector reduction operations.
>>>>> Previously, either only vector registers or only general purpose
>>>>> registers (GPR)
>>>>> were used. Now, vector registers are used for the reduction from 128 to 64
>>> bits;
>>>>> 64-bit GPR are used for the reduction from 64 to 32 bits; and 32-bit GPR
>>>>> are
>>> used
>>>>> for the rest of the reduction steps.
>>>>>
>>>>> For example, the test case (V8HI)
>>>>> int16_t foo (int16_t *a)
>>>>> {
>>>>> int16_t b = -1;
>>>>> for (int i = 0; i < 8; ++i)
>>>>> b &= a[i];
>>>>> return b;
>>>>> }
>>>>>
>>>>> was previously compiled to (-O2):
>>>>> foo:
>>>>> ldr q0, [x0]
>>>>> movi v30.4s, 0
>>>>> ext v29.16b, v0.16b, v30.16b, #8
>>>>> and v29.16b, v29.16b, v0.16b
>>>>> ext v31.16b, v29.16b, v30.16b, #4
>>>>> and v31.16b, v31.16b, v29.16b
>>>>> ext v30.16b, v31.16b, v30.16b, #2
>>>>> and v30.16b, v30.16b, v31.16b
>>>>> umov w0, v30.h[0]
>>>>> ret
>>>>>
>>>>> With patch, it is compiled to:
>>>>> foo:
>>>>> ldr q31, [x0]
>>>>> ext v30.16b, v31.16b, v31.16b, #8
>>>>> and v31.8b, v30.8b, v31.8b
>>>>> fmov x0, d31
>>>>> and x0, x0, x0, lsr 32
>>>>> and w0, w0, w0, lsr 16
>>>>> ret
>>>>>
>>>>> For modes V4SI and V2DI, the pattern was not implemented, because the
>>>>> current codegen (using only base instructions) is already efficient.
>>>>>
>>>>> Note that the PR initially suggested to use SVE reduction ops. However,
>>>>> they have higher latency than the proposed sequence, which is why using
>>>>> neon and base instructions is preferable.
>>>>>
>>>>> Test cases were added for 8/16-bit integers for all implemented modes and
>>>>> all
>>>>> three operations to check the produced assembly.
>>>>>
>>>>> We also added [istarget aarch64*-*-*] to the selector vect_logical_reduc,
>>>>> because for aarch64 vector types, either the logical reduction optabs are
>>>>> implemented or the codegen for reduction operations is good as it is.
>>>>> This was motivated by failure of a scan-tree-dump directive in the test
>>>>> cases
>>>>> gcc.dg/vect/vect-reduc-or_1.c and gcc.dg/vect/vect-reduc-or_2.c.
>>>>>
>>>>> The patch was bootstrapped and regtested on aarch64-linux-gnu, no
>>> regression.
>>>>> OK for mainline?
>>>>>
>>>>> Signed-off-by: Jennifer Schmitz <jschm...@nvidia.com>
>>>>>
>>>>> gcc/
>>>>> PR target/113816
>>>>> * config/aarch64/aarch64-simd.md (reduc_<optab>_scal_<mode>):
>>>>> Implement for logical bitwise operations for VDQV_E.
>>>>>
>>>>> gcc/testsuite/
>>>>> PR target/113816
>>>>> * lib/target-supports.exp (vect_logical_reduc): Add aarch64*.
>>>>> * gcc.target/aarch64/simd/logical_reduc.c: New test.
>>>>> * gcc.target/aarch64/vect-reduc-or_1.c: Adjust expected outcome.
>>>>> ---
>>>>> gcc/config/aarch64/aarch64-simd.md | 55 +++++
>>>>> .../gcc.target/aarch64/simd/logical_reduc.c | 208 ++++++++++++++++++
>>>>> .../gcc.target/aarch64/vect-reduc-or_1.c | 2 +-
>>>>> gcc/testsuite/lib/target-supports.exp | 4 +-
>>>>> 4 files changed, 267 insertions(+), 2 deletions(-)
>>>>> create mode 100644 gcc/testsuite/gcc.target/aarch64/simd/logical_reduc.c
>>>>>
>>>>> diff --git a/gcc/config/aarch64/aarch64-simd.md
>>> b/gcc/config/aarch64/aarch64-simd.md
>>>>> index 23c03a96371..00286b8b020 100644
>>>>> --- a/gcc/config/aarch64/aarch64-simd.md
>>>>> +++ b/gcc/config/aarch64/aarch64-simd.md
>>>>> @@ -3608,6 +3608,61 @@
>>>>> }
>>>>> )
>>>>>
>>>>> +;; Emit a sequence for bitwise logical reductions over vectors for V8QI,
>>>>> V16QI,
>>>>> +;; V4HI, and V8HI modes. The reduction is achieved by iteratively
>>>>> operating
>>>>> +;; on the two halves of the input.
>>>>> +;; If the input has 128 bits, the first operation is performed in vector
>>>>> +;; registers. From 64 bits down, the reduction steps are performed in
>>>>> general
>>>>> +;; purpose registers.
>>>>> +;; For example, for V8HI and operation AND, the intended sequence is:
>>>>> +;; EXT v1.16b, v0.16b, v0.16b, #8
>>>>> +;; AND v0.8b, v1.8b, v0.8b
>>>>> +;; FMOV x0, d0
>>>>> +;; AND x0, x0, x0, 32
>>>>> +;; AND w0, w0, w0, 16
>>>>> +;;
>>>>> +;; For V8QI and operation AND, the sequence is:
>>>>> +;; AND x0, x0, x0, lsr 32
>>>>> +;; AND w0, w0, w0, lsr, 16
>>>>> +;; AND w0, w0, w0, lsr, 8
>>>>> +
>>>>> +(define_expand "reduc_<optab>_scal_<mode>"
>>>>> + [(match_operand:<VEL> 0 "register_operand")
>>>>> + (LOGICAL:VDQV_E (match_operand:VDQV_E 1 "register_operand"))]
>>>>> + "TARGET_SIMD"
>>>>> + {
>>>>> + rtx dst = operands[1];
>>>>> + rtx tdi = gen_reg_rtx (DImode);
>>>>> + rtx tsi = lowpart_subreg (SImode, tdi, DImode);
>>>>> + rtx op1_lo;
>>>>> + if (known_eq (GET_MODE_SIZE (<MODE>mode), 16))
>>>>> + {
>>>>> + rtx t0 = gen_reg_rtx (<MODE>mode);
>>>>> + rtx t1 = gen_reg_rtx (DImode);
>>>>> + rtx t2 = gen_reg_rtx (DImode);
>>>>> + rtx idx = GEN_INT (8 / GET_MODE_UNIT_SIZE (<MODE>mode));
>>>>> + emit_insn (gen_aarch64_ext<mode> (t0, dst, dst, idx));
>>>>> + op1_lo = lowpart_subreg (V2DImode, dst, <MODE>mode);
>>>>> + rtx t0_lo = lowpart_subreg (V2DImode, t0, <MODE>mode);
>>>>> + emit_insn (gen_aarch64_get_lanev2di (t1, op1_lo, GEN_INT (0)));
>>>>> + emit_insn (gen_aarch64_get_lanev2di (t2, t0_lo, GEN_INT (0)));
>>>>> + emit_insn (gen_<optab>di3 (t1, t1, t2));
>>>>> + emit_move_insn (tdi, t1);
>>>>> + }
>>>>> + else
>>>>> + {
>>>>> + op1_lo = lowpart_subreg (DImode, dst, <MODE>mode);
>>>>> + emit_move_insn (tdi, op1_lo);
>>>>> + }
>>>>> + emit_insn (gen_<optab>_lshrdi3 (tdi, tdi, GEN_INT (32), tdi));
>>>>> + emit_insn (gen_<optab>_lshrsi3 (tsi, tsi, GEN_INT (16), tsi));
>>>>> + if (known_eq (GET_MODE_UNIT_BITSIZE (<MODE>mode), 8))
>>>>> + emit_insn (gen_<optab>_lshrsi3 (tsi, tsi, GEN_INT (8), tsi));
>>>>> + emit_move_insn (operands[0], lowpart_subreg (<VEL>mode, tsi,
>>>>> SImode));
>>>>> + DONE;
>>>>> + }
>>>>> +)
>>>>
>>>> Sorry to be awkward, but I've got mixed feelings about this.
>>>> The sequence produced looks good. But we're not defining this
>>>> optab because the target has a special instruction for implementing
>>>> the operation. We're just implementing it to tweak the loop emitted
>>>> by vect_create_epilog_for_reduction. Maybe we should change that loop
>>>> instead, perhaps with a target hook?
>>>
>>> So does it currently not work because there's no v8qi, v4qi or v2qi
>>> vector modes? I'll note there already _is_
>>> targetm.vectorize.split_reduction (mode), but it expects a vector
>>> mode as result and it doesn't get the operation in.
>>>
>>
>> The point of the reduction is that they shouldn't be done on the vector side.
>> So even if we have v4qi and v2qi the reduction shouldn't be done there as
>> the latency and throughput for these sub 64-bit operations on the vector
>> side is much higher and so should be done on the integer side. i.e. these
>> should be done on DI and SI.
>>
>> This is why I disagree with Richard that this can be done generically for all
>> targets. The decomposition takes advantage of Arm's fused shifted logical
>> operations on GPR. This makes them really cheap to do on integer.
>
> Wouldn't it still be a win without that though? Having the fused operation
> gives us a ~4x improvement in latency on a typical core (EXT + vector AND
> -> fused scalar AND). But the unfused operation would still be a ~2x
> improvement.
>
>> Richard brought up AArch32 as another example. I don't agree there either
>> Because AArch32's register file overlap means you don't need shifts.
>> The first step reduction can just be done by accessing the registers
>> as even/odd pairs on a smaller mode.
>>
>> To get this codegen generically the targets must supports modes tie-able
>> between the vector and smaller vector size. i.e. BIT_FIELD_REF on the
>> vector mode to a smaller mode must be cheap or free.
>>
>> The decision for when you do the transfer from SIMD to GPR is highly
>> target specific, and the how as well.
>>
>>> I would expect that we can improve on this existing machinery
>>> instead of inventing a new one.
>>>
>>
>> But we're not. We're just implementing the optab to the machinery
>> that already exists.
>
> I don't think this kind of expansion is what optabs are there for though.
> I suppose it was in the pre-gimple days, when all optimisation was done
> on RTL. (Although of course that also predates vectorisation.) But now,
> I think the purpose of optabs is to advertise what the target can do via
> some form of direct h/w support.
>
> The original patch (using ANDV) was an ideal use of optabs. But I think
> the request to use a tweaked version of the current open-coded reduction
> scheme, although a good suggestion, also changes the way that we should
> go about it.
>
>> I think we're just over engineering a new contributor's patch. Even if
>> we think there is some merit to do it generically, I don't see why we
>> shouldn't take this patch today.
>
> Sorry if it comes across that way.
Hi everyone,
Thanks for the discussion around my patch. I will certainly be happy to look at
alternative implementation locations for the optimization, also on the gimple
level.
Could you offer some more guidance which direction to pursue, whether to extend
targetm.vectorize.split_reduction (mode) or to make changes to
vect_create_epilog_for_reduction? I’m not familiar enough with these parts of
the code yet to decide which one is more suitable.
Thanks and have a nice weekend,
Jennifer
>
> Thanks,
> Richard
smime.p7s
Description: S/MIME cryptographic signature
