Tamar Christina <tamar.christ...@arm.com> writes:
>> -----Original Message-----
>> From: Richard Sandiford <richard.sandif...@arm.com>
>> Sent: Wednesday, March 5, 2025 11:27 AM
>> To: Tamar Christina <tamar.christ...@arm.com>
>> Cc: gcc-patches@gcc.gnu.org; nd <n...@arm.com>; Richard Earnshaw
>> <richard.earns...@arm.com>; ktkac...@gcc.gnu.org
>> Subject: Re: [1/3 PATCH]AArch64: add support for partial modes to last
>> extractions [PR118464]
>> 
>> Tamar Christina <tamar.christ...@arm.com> writes:
>> >> > diff --git a/gcc/config/aarch64/aarch64-sve.md
>> b/gcc/config/aarch64/aarch64-
>> >> sve.md
>> >> > index
>> >>
>> e975286a01904bec0b283b7ba4afde6f0fd60bf1..6c0be3c1a51449274720175b
>> >> 5e6e7d7535928de6 100644
>> >> > --- a/gcc/config/aarch64/aarch64-sve.md
>> >> > +++ b/gcc/config/aarch64/aarch64-sve.md
>> >> > @@ -3107,7 +3107,7 @@ (define_insn "@extract_<last_op>_<mode>"
>> >> >    [(set (match_operand:<VEL> 0 "register_operand")
>> >> >         (unspec:<VEL>
>> >> >           [(match_operand:<VPRED> 1 "register_operand")
>> >> > -          (match_operand:SVE_FULL 2 "register_operand")]
>> >> > +          (match_operand:SVE_ALL 2 "register_operand")]
>> >> >           LAST))]
>> >> >    "TARGET_SVE"
>> >> >    {@ [ cons: =0 , 1   , 2  ]
>> >>
>> >> It looks like this will use (say):
>> >>
>> >>   lasta b<n>, pg, z<m>.b
>> >>
>> >> for VNx4QI, is that right?  I don't think that's safe, since the .b form
>> >> treats all bits of the pg input as significant, whereas only one in every
>> >> four bits of pg is defined for VNx4BI (the predicate associated with 
>> >> VNx4QI).
>> >>
>> >> I think converting these patterns to partial vectors means operating
>> >> on containers rather than elements.  E.g. the VNx4QI case should use
>> >> .s rather than .b.  That should just be a case of changing vwcore to
>> >> vccore and Vetype to Vctype, but I haven't looked too deeply.
>> >
>> > Ah I see, so for partial types, the values are not expected to be packed 
>> > in the
>> lower
>> > part of the vector, but instead are "padded"?
>> 
>> Right.
>> 
>> > That explains some of the other patterns
>> > I was confused about.
>> >
>> > Any ideas how to test these? It's hard to control what modes the vectorizer
>> picks..
>> 
>> Yeah, agreed.  I think it'd be difficult to trigger it reliably from the
>> vectoriser given its current limited use of the ifns.
>> 
>> A gimple frontend test might work though, with a predicate/mask
>> generated from (say) 16-bit elements, then bitcast to a predicate/mask
>> for 32-bit elements and used as an input to an explicit ifn on 32-bit
>> elements.  If the 16-bit predicate contains 0, 1 for some even-aligned
>> pair, after the last 1, 0 aligned pair, then the code would likely have
>> picked the wrong element.
>
> *insert chaos emoji* I realized the testcases in the testsuite early break
> vect tests needed this since that's how I noticed it anyway.. so it's enough
> to run that with SVE enabled (as there are tests depending on partial vectors
> anyway)
>
> So...
>
> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
>
> Ok for master?
>
> Thanks,
> Tamar
>
> gcc/ChangeLog:
>
>
>       PR tree-optimization/118464
>       PR tree-optimization/116855
>       * config/aarch64/aarch64-sve.md (@extract_<last_op>_<mode>,
>       @fold_extract_<last_op>_<mode>,
>       @aarch64_fold_extract_vector_<last_op>_<mode>): Change SVE_FULL to
>       SVE_ALL/
>
> -- inline copy of patch --
>
> diff --git a/gcc/config/aarch64/aarch64-sve.md 
> b/gcc/config/aarch64/aarch64-sve.md
> index 
> a93bc463a909ea28460cc7877275fce16e05f7e6..205eeec2e35544de848e0dbb48e3f5ae59391a88
>  100644
> --- a/gcc/config/aarch64/aarch64-sve.md
> +++ b/gcc/config/aarch64/aarch64-sve.md
> @@ -3107,12 +3107,12 @@ (define_insn "@extract_<last_op>_<mode>"
>    [(set (match_operand:<VEL> 0 "register_operand")
>       (unspec:<VEL>
>         [(match_operand:<VPRED> 1 "register_operand")
> -        (match_operand:SVE_FULL 2 "register_operand")]
> +        (match_operand:SVE_ALL 2 "register_operand")]
>         LAST))]
>    "TARGET_SVE"
>    {@ [ cons: =0 , 1   , 2  ]
> -     [ ?r       , Upl , w  ] last<ab>\t%<vwcore>0, %1, %2.<Vetype>
> -     [ w        , Upl , w  ] last<ab>\t%<Vetype>0, %1, %2.<Vetype>
> +     [ ?r       , Upl , w  ] last<ab>\t%<vwcore>0, %1, %2.<Vctype>

This looks good for the Vetype->Vctype part, but it still needs the
vwcore->vccore change too.  If we don't do that, we could end up pairing
a w destination with a .d source, which would trigger an assembler error.

Thanks,
Richard


> +     [ w        , Upl , w  ] last<ab>\t%<Vctype>0, %1, %2.<Vctype>
>    }
>  )
>  
> @@ -8899,26 +8899,26 @@ (define_insn "@fold_extract_<last_op>_<mode>"
>       (unspec:<VEL>
>         [(match_operand:<VEL> 1 "register_operand")
>          (match_operand:<VPRED> 2 "register_operand")
> -        (match_operand:SVE_FULL 3 "register_operand")]
> +        (match_operand:SVE_ALL 3 "register_operand")]
>         CLAST))]
>    "TARGET_SVE"
>    {@ [ cons: =0 , 1 , 2   , 3  ]
> -     [ ?r       , 0 , Upl , w  ] clast<ab>\t%<vwcore>0, %2, %<vwcore>0, 
> %3.<Vetype>
> -     [ w        , 0 , Upl , w  ] clast<ab>\t%<Vetype>0, %2, %<Vetype>0, 
> %3.<Vetype>
> +     [ ?r       , 0 , Upl , w  ] clast<ab>\t%<vwcore>0, %2, %<vwcore>0, 
> %3.<Vctype>
> +     [ w        , 0 , Upl , w  ] clast<ab>\t%<Vctype>0, %2, %<Vctype>0, 
> %3.<Vctype>
>    }
>  )
>  
>  (define_insn "@aarch64_fold_extract_vector_<last_op>_<mode>"
> -  [(set (match_operand:SVE_FULL 0 "register_operand")
> -     (unspec:SVE_FULL
> -       [(match_operand:SVE_FULL 1 "register_operand")
> +  [(set (match_operand:SVE_ALL 0 "register_operand")
> +     (unspec:SVE_ALL
> +       [(match_operand:SVE_ALL 1 "register_operand")
>          (match_operand:<VPRED> 2 "register_operand")
> -        (match_operand:SVE_FULL 3 "register_operand")]
> +        (match_operand:SVE_ALL 3 "register_operand")]
>         CLAST))]
>    "TARGET_SVE"
>    {@ [ cons: =0 , 1 , 2   , 3  ]
> -     [ w        , 0 , Upl , w  ] clast<ab>\t%0.<Vetype>, %2, %0.<Vetype>, 
> %3.<Vetype>
> -     [ ?&w      , w , Upl , w  ] movprfx\t%0, %1\;clast<ab>\t%0.<Vetype>, 
> %2, %0.<Vetype>, %3.<Vetype>
> +     [ w        , 0 , Upl , w  ] clast<ab>\t%0.<Vctype>, %2, %0.<Vctype>, 
> %3.<Vctype>
> +     [ ?&w      , w , Upl , w  ] movprfx\t%0, %1\;clast<ab>\t%0.<Vctype>, 
> %2, %0.<Vctype>, %3.<Vctype>
>    }
>  )

Reply via email to