On Tue, Dec 9, 2014 at 10:17 AM, Jakub Jelinek <ja...@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.
