Hi!
On Wed, Nov 20, 2019 at 03:31:36PM +0800, Kewen.Lin wrote:
> >> +(define_expand "vector_ge<mode>"
> >> + [(set (match_operand:VEC_F 0 "vlogical_operand")
> >> + (ge:VEC_F (match_operand:VEC_F 1 "vlogical_operand")
> >> + (match_operand:VEC_F 2 "vlogical_operand")))]
> >> "VECTOR_UNIT_ALTIVEC_OR_VSX_P (<MODE>mode)"
> >> + "")
> >
> > This already exists? Line 764 in vector.md?
> >
>
> Yes, that one is only for VEC_F (aka. vector FP), move to here to centralize
> them. Does it make sense?
Ah, I see. Yeah, that makes sense.
> > Does this work for (say) ungt? I would do two switch statements: the
> > first only to do the invert, and then the second to do the swap (with
> > the modified cond!) So:
>
> Yes, it works but it has to call further splitting for "le". It looks
> better to split to the end at once. Thanks a lot for your example!
Yes, exactly. The code is simpler in this case, and more importantly,
it is clearer that the code the splitter outputs is somewhat optimsed.
Output from splitters does not get most basic RTL optimisations anymore,
the splitters run after those!
> > if (cond == UNLE || cond == UNLT || cond == NE
> > || cond == UNGE || cond == UNGT)
> > {
> > cond = reverse_condition_maybe_unordered (cond);
> > need_invert = true;
> > }
> >
> > if (cond == LT || cond == LE)
> > {
> > cond = swap_condition (cond);
> > std::swap (operands[1], operands[2]);
> > }
>
> I added one assert here to guard the codes.
Sure, if you aren't certain no other codes can happen, or you expect
things to ever change, etc.
> > Also, can_create_pseudo in the split condition can fail, in theory
> > anyway. It should be part of the insn condition, instead, and even
> > then it can fail: after the last split pass, but before reload. such
> > an insn can in principle be created, and then it sill be never split.
> >
> > In this case, maybe you should add a scratch register; in the previous
> > case, you can reuse operands[0] for res instead of making a new reg
> > for it, if !can_create_pseudo ().
>
> I've updated the previous case with operands[0] with !can_create_pseudo
> check. But for the later one I met ICE if I added two clobbers with
> scratch into the RTL pattern like:
>
> [(set (match_operand:VEC_F 0 "vfloat_operand")
> (vector_fp_extra_comparison_operator:VEC_F
> (match_operand:VEC_F 1 "vfloat_operand")
> (match_operand:VEC_F 2 "vfloat_operand")))
> (clobber (match_scratch:VEC_F 3))
> (clobber (match_scratch:VEC_F 4))]
>
> Some pattern without clobber as below can't be recognized (early in vregs).
>
> (insn 24 23 25 4 (set (reg:V4SF 142)
> (uneq:V4SF (reg:V4SF 141 [ vect__4.11 ])
> (reg:V4SF 127 [ vect_cst__47 ]))) -1
> (nil))
Yeah. Just doing can_create_pseudo in the insn condition (and in the
split condition, via &&) will work -- there just is this window of
failure you should be aware of, and we need to do something about that,
eventually. It's a very general problem. There are many places in
rs6000 where we already do evil things with this (and some much worse
than this even), so one more won't hurt too much.
It does need can_create_pseudo in the split condition then though.
> > rtx comp1 = gen_rtx_fmt_ee (cond, <MODE>mode, operands[1], operands[2]);
> > rtx res1 = gen_reg_rtx (<MODE>mode);
> > emit_insn (gen_rtx_SET (res1, comp1));
> > rtx comp2 = gen_rtx_fmt_ee (cond, <MODE>mode, operands[2], operands[1]);
> > rtx res2 = gen_reg_rtx (<MODE>mode);
> > emit_insn (gen_rtx_SET (res2, comp2));
> >
> > if (need_invert)
> > {
> > rtx comp3 = gen_rtx_fmt_ee (IOR, <MODE>mode, res1, res2);
> > rtx comp4 = gen_rtx_fmt_e (NOT, <MODE>mode, comp3);
>
> I had to add one more gen_rtx_SET for the IOR here, otherwise it gets the
> error
> on the pattern can't be recognized.
Oh, sorry. The canonical way of writing a NOR is as an ANDCC, so this
should be
rtx not1 = gen_rtx_fmt_e (NOT, <MODE>mode, res1);
rtx not2 = gen_rtx_fmt_e (NOT, <MODE>mode, res2);
rtx comp3 = gen_rtx_fmt_ee (AND, <MODE>mode, not1, not2);
emit_insn (gen_rtx_SET (operands[0], comp3));
> diff --git a/gcc/config/rs6000/vector.md b/gcc/config/rs6000/vector.md
> index b132037..deeab9f 100644
> --- a/gcc/config/rs6000/vector.md
> +++ b/gcc/config/rs6000/vector.md
> @@ -107,6 +107,12 @@
> (smin "smin")
> (smax "smax")])
>
> +;; code iterators and attributes for vector FP comparison operators:
> +(define_code_iterator
> + vector_fp_comparison_operator [lt le ne ungt unge unlt unle])
Can you change this name so it is clear it is just the simple ones?
Segher
