Message ID | 20141209091723.GO1667@tucnak.redhat.com |
---|---|
State | New |
Headers | show |
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.
--- 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]),