[PATCH 1/2] [target 87767] Refactor AVX512 broadcast patterns with speical memory constraint.

[Hongtao Liu]

On Tue, Oct 20, 2020 at 10:57 PM Vladimir Makarov <vmakarov@redhat.com> wrote:
>
>
> On 2020-10-20 1:33 a.m., Hongtao Liu wrote:
> > On Mon, Oct 19, 2020 at 11:38 PM Vladimir Makarov <vmakarov@redhat.com> wrote:
> >>
> >> On 2020-10-11 8:58 p.m., Hongtao Liu wrote:
> >>> Hi:
> >>>     This is done in 2 steps:
> >>>     1. Extend special memory constraint to handle non MEM_P cases, i.e.
> >>> (vec_duplicate:V4SF (mem:SF (addr)))
> >>>     2. Refactor implementation of *_bcst{_1,_2,_3} patterns. Add new
> >>> predicate bcst_mem_operand and corresponding constraint "Br" to merge
> >>> "$(pattern)_bcst{_1,_2,_3}" into "$(pattern)", also delete those
> >>> separate "*_bcst{_1,_2,_3}" patterns.
> >>>
> >>>     Bootstrap is ok, regression test on i386 backend is ok.
> >>>
> >>> gcc/ChangeLog:
> >>>
> >>>           PR target/87767
> >>>           * ira-costs.c (record_operand_costs): Extract memory operand
> >>>           from recog_data.operand[i] for record_address_regs.
> >>>           (record_reg_classes): Extract memory operand from OP for
> >>>           conditional judgement MEM_P.
> >>>           * ira.c (ira_setup_alts): Ditto.
> >>>           * lra-constraints.c (extract_mem_from_operand): New function.
> >>>           (satisfies_memory_constraint_p): Extract memory operand from
> >>>           OP for decompose_mem_address, return false when there's no
> >>>           memory operand inside OP.
> >>>           (process_alt_operands): Remove MEM_P (op) since it would be
> >>>           judged in satisfies_memory_constraint_p.
> >>>           * recog.c (asm_operand_ok): Extract memory operand from OP for
> >>>           judgement of memory_operand (OP, VOIDmode).
> >>>           (constrain_operands): Don't unwrapper unary operator when
> >>>           there's memory operand inside.
> >>>           * rtl.h (extract_mem_from_operand): New decl.
> >>
> >> Thank you for working on the PR.  In general patch is ok for me. The
> >> only thing is
> >>
> >> +/* For special_memory_operand, it could be false for MEM_P (op),
> >> +   i.e. bcst_mem_operand in i386 backend.
> >> +   Extract and return real memory operand or op.  */
> >> +rtx
> >> +extract_mem_from_operand (rtx op)
> >> +{
> >> +  if (MEM_P (op))
> >> +    return op;
> >> +  /* Only allow one memory_operand inside special memory operand.  */
> >>
> >> The comment contradicts to the below code which returns the first memory operand (not the only one).
> >>
> > Yes.
> >
> >> +  subrtx_var_iterator::array_type array;
> >> +  FOR_EACH_SUBRTX_VAR (iter, array, op, ALL)
> >> +    {
> >> +      rtx x = *iter;
> >> +      if (MEM_P (x))
> >> +       return x;
> >> +    }
> >> +
> >> +  return op;
> >> +}
> >> +
> >>
> >> I think the code should look like
> >>
> >> /* For special_memory_operand, it could be false for MEM_P (op),
> >>      i.e. bcst_mem_operand in i386 backend.
> >>      Extract and return real memory operand or op.  */
> >> rtx
> >> extract_mem_from_operand (rtx op)
> >> {
> >>     if (MEM_P (op))
> >>       return op;
> >>     /* Only allow one memory_operand inside special memory operand.  */
> >>     subrtx_var_iterator::array_type array;
> >>     rtx res = op;
> >>     FOR_EACH_SUBRTX_VAR (iter, array, op, ALL)
> >>       {
> >>         rtx x = *iter;
> >>         if (!MEM_P (x) || res != op)
> >>           return op;
> >>         res = op;
> > Assume you want to assign res with x.
> > Also in the iteration, x would first be op which would be false for
> > MEM_P, then op would be returned.
> > That's not what you mean, so i changed to
> >
> >    /* Only allow one memory_operand inside special memory operand.  */
> >    subrtx_var_iterator::array_type array;
> >    rtx res = op;
> >    FOR_EACH_SUBRTX_VAR (iter, array, op, ALL)
> >      {
> >        rtx x = *iter;
> >        if (!MEM_P (x))
> >          continue;
> >        /* Return op when there're multiple memory operands.  */
> >        if (res != op)
> >          return op;
> >        else
> >          res = x;
> >      }
>
> Actually I wanted to have constraint satisfying rtx with memory covered
> by **only unary** operator(s).  Your code satisfies memory covered by
> non-unary operators (e.g. binary ones).
>
> Why do I prefer less general constraint? Because other operands of
> operator containing the memory might need reloads too and the more
> general constraint will ignore this. If this situation is impossible
> now, it might be possible in the future.
>

Got your point.

> My proposed code is wrong as I forgot that FOR_EACH_SUBRTX_VAR processes
> sub-rtx recursively.  Thank you for starting the discussion.  Now I
> think the code should look like
>
> /* For special_memory_operand, it could be false for MEM_P (op),
>      i.e. bcst_mem_operand in i386 backend.
>      Extract and return real memory operand or op.  */
> rtx
> extract_mem_from_operand (rtx op)
> {
>    for (rtx x = op;; x = XEXP (x, 0)) {
>
>     if (MEM_P (x))
>       return x;
>     if (GET_RTX_LENGTH (GET_CODE (x)) != 1 || GET_RTX_FORMAT (GET_CODE
> (x))[0] != 'e')
>       break;
>
>    }
>
>    return op;
>
> }
>
> Let me know what do you think.
>

Changed, and it passed the i386/x86-64 regression test.

Update patch.

-- 
BR,
Hongtao
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Extend-special_memory_constraint-v2.patch
Type: text/x-patch
Size: 6414 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20201021/cb884d3d/attachment.bin>