Fix broadcast from scalar patterns (PR target/63594)

On Tue, Dec 09, 2014 at 09:49:17AM +0100, Uros Bizjak wrote:
> >  (define_insn "<mask_codefor><avx512>_vec_dup_gpr<mode><mask_name>"
> > -  [(set (match_operand:VI48_AVX512VL 0 "register_operand" "=v")
> > -       (vec_duplicate:VI48_AVX512VL
> > -         (match_operand:<ssescalarmode> 1 "register_operand" "r")))]
> > -  "TARGET_AVX512F && (<ssescalarmode>mode != DImode || TARGET_64BIT)"
> > -{
> > -  return "vpbroadcast<bcstscalarsuff>\t{%1, %0<mask_operand2>|%0<mask_operand2>, %1}";
> > -}
> > -  [(set_attr "type" "ssemov")
> > -   (set_attr "prefix" "evex")
> > -   (set_attr "mode" "<sseinsnmode>")])
> > -
> > -(define_insn "<mask_codefor><avx512>_vec_dup_mem<mode><mask_name>"
> > -  [(set (match_operand:V48_AVX512VL 0 "register_operand" "=v")
> > -       (vec_duplicate:V48_AVX512VL
> > -         (match_operand:<ssescalarmode> 1 "nonimmediate_operand" "vm")))]
> > +  [(set (match_operand:V48_AVX512VL 0 "register_operand" "=v,v")
> > +       (vec_duplicate:V48_AVX512VL
> > +         (match_operand:<ssescalarmode> 1 "nonimmediate_operand" "vm,r")))]
> >    "TARGET_AVX512F"
> >    "v<sseintprefix>broadcast<bcstscalarsuff>\t{%1, %0<mask_operand2>|%0<mask_operand2>, %1}"
> >    [(set_attr "type" "ssemov")
> >     (set_attr "prefix" "evex")
> > -   (set_attr "mode" "<sseinsnmode>")])
> > +   (set_attr "mode" "<sseinsnmode>")
> > +   (set (attr "enabled")
> > +     (if_then_else (eq_attr "alternative" "1")
> > +       (symbol_ref "GET_MODE_CLASS (<ssescalarmode>mode) == MODE_INT
> > +                    && (<ssescalarmode>mode != DImode || TARGET_64BIT)")
> > +       (const_int 1)))])

I have no idea how to get rid of this enabled attribute, as it disables just
one alternative.  Creating a special isa attribute for it looks less
readable and much more expensive.  This enabled attribute is IMHO of the
good kind, GET_MODE_CLASS (<ssescalarmode>mode) == MODE_INT and
<ssescalarmode>mode != DImode are constants for the particular chosen mode,
TARGET_64BIT doesn't change during compilation of a TU, and so the only
variable is that it disables just one of the alternatives.

> > @@ -16759,7 +16744,10 @@ (define_split
> >    [(set (match_operand:AVX2_VEC_DUP_MODE 0 "register_operand")
> >         (vec_duplicate:AVX2_VEC_DUP_MODE
> >           (match_operand:<ssescalarmode> 1 "register_operand")))]
> > -  "TARGET_AVX2 && reload_completed && GENERAL_REG_P (operands[1])"
> > +  "TARGET_AVX2
> > +   && (!TARGET_AVX512VL
> > +       || (!TARGET_AVX512BW && GET_MODE_SIZE (<ssescalarmode>mode) > 2))
> > +   && reload_completed && GENERAL_REG_P (operands[1])"
> >    [(const_int 0)]
> 
> We would like to avoid convoluted insn enable condition by moving the
> target delated complexity to the mode iterator, even if it requires
> additional single-use mode iterator. In the ideal case, the remaining
> target-dependant condition would represent the baseline target for an
> insn and all other target-related conditions would be inside mode
> iterator.

This splitter condition can be replaced with mode iterator, but it results
in big repetitions there (incremental diff below).  Or do you have something
else in mind?


	Jakub

On Tue, Dec 9, 2014 at 10:17 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Dec 09, 2014 at 09:49:17AM +0100, Uros Bizjak wrote:
>> >  (define_insn "<mask_codefor><avx512>_vec_dup_gpr<mode><mask_name>"
>> > -  [(set (match_operand:VI48_AVX512VL 0 "register_operand" "=v")
>> > -       (vec_duplicate:VI48_AVX512VL
>> > -         (match_operand:<ssescalarmode> 1 "register_operand" "r")))]
>> > -  "TARGET_AVX512F && (<ssescalarmode>mode != DImode || TARGET_64BIT)"
>> > -{
>> > -  return "vpbroadcast<bcstscalarsuff>\t{%1, %0<mask_operand2>|%0<mask_operand2>, %1}";
>> > -}
>> > -  [(set_attr "type" "ssemov")
>> > -   (set_attr "prefix" "evex")
>> > -   (set_attr "mode" "<sseinsnmode>")])
>> > -
>> > -(define_insn "<mask_codefor><avx512>_vec_dup_mem<mode><mask_name>"
>> > -  [(set (match_operand:V48_AVX512VL 0 "register_operand" "=v")
>> > -       (vec_duplicate:V48_AVX512VL
>> > -         (match_operand:<ssescalarmode> 1 "nonimmediate_operand" "vm")))]
>> > +  [(set (match_operand:V48_AVX512VL 0 "register_operand" "=v,v")
>> > +       (vec_duplicate:V48_AVX512VL
>> > +         (match_operand:<ssescalarmode> 1 "nonimmediate_operand" "vm,r")))]
>> >    "TARGET_AVX512F"
>> >    "v<sseintprefix>broadcast<bcstscalarsuff>\t{%1, %0<mask_operand2>|%0<mask_operand2>, %1}"
>> >    [(set_attr "type" "ssemov")
>> >     (set_attr "prefix" "evex")
>> > -   (set_attr "mode" "<sseinsnmode>")])
>> > +   (set_attr "mode" "<sseinsnmode>")
>> > +   (set (attr "enabled")
>> > +     (if_then_else (eq_attr "alternative" "1")
>> > +       (symbol_ref "GET_MODE_CLASS (<ssescalarmode>mode) == MODE_INT
>> > +                    && (<ssescalarmode>mode != DImode || TARGET_64BIT)")
>> > +       (const_int 1)))])
>
> I have no idea how to get rid of this enabled attribute, as it disables just
> one alternative.  Creating a special isa attribute for it looks less
> readable and much more expensive.  This enabled attribute is IMHO of the
> good kind, GET_MODE_CLASS (<ssescalarmode>mode) == MODE_INT and
> <ssescalarmode>mode != DImode are constants for the particular chosen mode,
> TARGET_64BIT doesn't change during compilation of a TU, and so the only
> variable is that it disables just one of the alternatives.

IMO, this one is not problematic.

>> > @@ -16759,7 +16744,10 @@ (define_split
>> >    [(set (match_operand:AVX2_VEC_DUP_MODE 0 "register_operand")
>> >         (vec_duplicate:AVX2_VEC_DUP_MODE
>> >           (match_operand:<ssescalarmode> 1 "register_operand")))]
>> > -  "TARGET_AVX2 && reload_completed && GENERAL_REG_P (operands[1])"
>> > +  "TARGET_AVX2
>> > +   && (!TARGET_AVX512VL
>> > +       || (!TARGET_AVX512BW && GET_MODE_SIZE (<ssescalarmode>mode) > 2))
>> > +   && reload_completed && GENERAL_REG_P (operands[1])"
>> >    [(const_int 0)]
>>
>> We would like to avoid convoluted insn enable condition by moving the
>> target delated complexity to the mode iterator, even if it requires
>> additional single-use mode iterator. In the ideal case, the remaining
>> target-dependant condition would represent the baseline target for an
>> insn and all other target-related conditions would be inside mode
>> iterator.
>
> This splitter condition can be replaced with mode iterator, but it results
> in big repetitions there (incremental diff below).  Or do you have something
> else in mind?
>
> --- gcc/config/i386/sse.md      2014-12-08 13:26:06.505543457 +0100
> +++ gcc/config/i386/sse.md      2014-12-08 13:26:06.505543457 +0100
> @@ -16740,14 +16740,21 @@
>     (set_attr "isa" "avx2,noavx2,avx2,noavx2")
>     (set_attr "mode" "<sseinsnmode>,V8SF,<sseinsnmode>,V8SF")])
>
> +;; Modes handled by AVX2 vec_dup splitter.  Assumes TARGET_AVX2,
> +;; if both -mavx512vl and -mavx512bw are used, disables all modes,
> +;; if just -mavx512vl, disables just V*SI.
> +(define_mode_iterator AVX2_VEC_DUP_MODE_SPLIT
> +  [(V32QI "!TARGET_AVX512VL || !TARGET_AVX512BW")
> +   (V16QI "!TARGET_AVX512VL || !TARGET_AVX512BW")
> +   (V16HI "!TARGET_AVX512VL || !TARGET_AVX512BW")
> +   (V8HI "!TARGET_AVX512VL || !TARGET_AVX512BW")
> +   (V8SI "!TARGET_AVX512VL") (V4SI "!TARGET_AVX512VL")])
> +
>  (define_split
> -  [(set (match_operand:AVX2_VEC_DUP_MODE 0 "register_operand")
> -       (vec_duplicate:AVX2_VEC_DUP_MODE
> +  [(set (match_operand:AVX2_VEC_DUP_MODE_SPLIT 0 "register_operand")
> +       (vec_duplicate:AVX2_VEC_DUP_MODE_SPLIT
>           (match_operand:<ssescalarmode> 1 "register_operand")))]
> -  "TARGET_AVX2
> -   && (!TARGET_AVX512VL
> -       || (!TARGET_AVX512BW && GET_MODE_SIZE (<ssescalarmode>mode) > 2))
> -   && reload_completed && GENERAL_REG_P (operands[1])"
> +  "TARGET_AVX2 && reload_completed && GENERAL_REG_P (operands[1])"
>    [(const_int 0)]
>  {
>    emit_insn (gen_vec_setv4si_0 (gen_lowpart (V4SImode, operands[0]),

Ugh, tough choice.

Let's go with the original, but please add the comment, similar to the
one above. Maybe also use "<ssecalarmode>mode == SImode" instead of
GET_MODE_SIZE, it better matches the comment.

Thanks,
Uros.