Re: [PATCH 09/12] [i386] Add patterns and predicates foutline-msabi-xlouges

[Uros Bizjak <ubizjak@gmail.com>]

On Thu, Apr 27, 2017 at 10:09 AM, Daniel Santos <daniel.santos@pobox.com> wrote:
> Adds the predicates save_multiple and restore_multiple to predicates.md,
> which are used by following patterns in sse.md:
>
> * save_multiple - insn that calls a save stub
> * restore_multiple - call_insn that calls a save stub and returns to the
>   function to allow a sibling call (which should typically offer better
>   optimization than the restore stub as the tail call)
> * restore_multiple_and_return - a jump_insn that returns from the
>   function as a tail-call.
> * restore_multiple_leave_return - like the above, but restores the frame
>   pointer before returning.
>
> Signed-off-by: Daniel Santos <daniel.santos@pobox.com>
> ---
>  gcc/config/i386/predicates.md | 155 ++++++++++++++++++++++++++++++++++++++++++
>  gcc/config/i386/sse.md        |  37 ++++++++++
>  2 files changed, 192 insertions(+)
>
> diff --git a/gcc/config/i386/predicates.md b/gcc/config/i386/predicates.md
> index 8f250a2e720..36fe8abc3f4 100644
> --- a/gcc/config/i386/predicates.md
> +++ b/gcc/config/i386/predicates.md
> @@ -1657,3 +1657,158 @@
>    (ior (match_operand 0 "register_operand")
>         (and (match_code "const_int")
>             (match_test "op == constm1_rtx"))))
> +
> +;; Return true if:
> +;; 1. first op is a symbol reference,
> +;; 2. >= 13 operands, and
> +;; 3. operands 2 to end is one of:
> +;;   a. save a register to a memory location, or
> +;;   b. restore stack pointer.
> +(define_predicate "save_multiple"
> +  (match_code "parallel")
> +{
> +  const unsigned nregs = XVECLEN (op, 0);
> +  rtx head = XVECEXP (op, 0, 0);
> +  unsigned i;
> +
> +  if (GET_CODE (head) != USE)
> +    return false;
> +  else
> +    {
> +      rtx op0 = XEXP (head, 0);
> +      if (op0 == NULL_RTX || GET_CODE (op0) != SYMBOL_REF)
> +       return false;
> +    }
> +
> +  if (nregs < 13)
> +    return false;
> +
> +  for (i = 2; i < nregs; i++)
> +    {
> +      rtx e, src, dest;
> +
> +      e = XVECEXP (op, 0, i);
> +
> +      switch (GET_CODE (e))
> +       {
> +         case SET:
> +           src  = SET_SRC (e);
> +           dest = SET_DEST (e);
> +
> +           /* storing a register to memory.  */
> +           if (GET_CODE (src) == REG && GET_CODE (dest) == MEM)

Please use REG_P (...) and MEM_P (...) - and possible others -
predicates in the code.

> +             {
> +               rtx addr = XEXP (dest, 0);
> +
> +               /* Good if dest address is in RAX.  */
> +               if (GET_CODE (addr) == REG
> +                   && REGNO (addr) == AX_REG)
> +                 continue;
> +
> +               /* Good if dest address is offset of RAX.  */
> +               if (GET_CODE (addr) == PLUS
> +                   && GET_CODE (XEXP (addr, 0)) == REG
> +                   && REGNO (XEXP (addr, 0)) == AX_REG)
> +                 continue;
> +             }
> +           break;
> +
> +         default:
> +           break;
> +       }
> +       return false;
> +    }
> +  return true;
> +})
> +
> +;; Return true if:
> +;; * first op is (return) or a a use (symbol reference),
> +;; * >= 14 operands, and
> +;; * operands 2 to end are one of:
> +;;   - restoring a register from a memory location that's an offset of RSI.
> +;;   - clobbering a reg
> +;;   - adjusting SP
> +(define_predicate "restore_multiple"
> +  (match_code "parallel")
> +{
> +  const unsigned nregs = XVECLEN (op, 0);
> +  rtx head = XVECEXP (op, 0, 0);
> +  unsigned i;
> +
> +  switch (GET_CODE (head))
> +    {
> +      case RETURN:
> +       i = 3;
> +       break;
> +
> +      case USE:
> +      {
> +       rtx op0 = XEXP (head, 0);
> +
> +       if (op0 == NULL_RTX || GET_CODE (op0) != SYMBOL_REF)
> +         return false;
> +
> +       i = 1;
> +       break;
> +      }
> +
> +      default:
> +       return false;
> +    }
> +
> +  if (nregs < i + 12)
> +    return false;
> +
> +  for (; i < nregs; i++)
> +    {
> +      rtx e, src, dest;
> +
> +      e = XVECEXP (op, 0, i);
> +
> +      switch (GET_CODE (e))
> +       {
> +         case CLOBBER:
> +           continue;

I don't see where CLOBBER is genreated in ix86_emit_outlined_ms2sysv_restore.

> +
> +         case SET:
> +           src  = SET_SRC (e);
> +           dest = SET_DEST (e);
> +
> +           /* Restoring a register from memory.  */
> +           if (GET_CODE (src) == MEM && GET_CODE (dest) == REG)
> +             {
> +               rtx addr = XEXP (src, 0);
> +
> +               /* Good if src address is in RSI.  */
> +               if (GET_CODE (addr) == REG
> +                   && REGNO (addr) == SI_REG)
> +                 continue;
> +
> +               /* Good if src address is offset of RSI.  */
> +               if (GET_CODE (addr) == PLUS
> +                   && GET_CODE (XEXP (addr, 0)) == REG
> +                   && REGNO (XEXP (addr, 0)) == SI_REG)
> +                 continue;
> +
> +               /* Good if adjusting stack pointer.  */
> +               if (GET_CODE (dest) == REG
> +                   && REGNO (dest) == SP_REG
> +                   && GET_CODE (src) == PLUS
> +                   && GET_CODE (XEXP (src, 0)) == REG
> +                   && REGNO (XEXP (src, 0)) == SP_REG)
> +                 continue;
> +             }
> +
> +           /* Restoring stack pointer from another register.  */
> +           if (GET_CODE (dest) == REG && REGNO (dest) == SP_REG
> +               && GET_CODE (src) == REG)
> +             continue;
> +           break;
> +
> +         default:
> +           break;
> +       }
> +       return false;
> +    }
> +  return true;
> +})

I think that the above functions should check only if the function is
storing/restoring correct registers, all other RTXes should be
explicitly written in the insn patterns.

> diff --git a/gcc/config/i386/sse.md b/gcc/config/i386/sse.md
> index e8ccb1e10c3..c9fe7274def 100644
> --- a/gcc/config/i386/sse.md
> +++ b/gcc/config/i386/sse.md
> @@ -19997,3 +19997,40 @@
>            (match_operand:VI48_512 1 "nonimmediate_operand" "vm")))]
>    "TARGET_AVX512VPOPCNTDQ"
>    "vpopcnt<ssemodesuffix>\t{%1, %0<mask_operand2>|%0<mask_operand2>, %1}")
> +
> +;; Save multiple registers out-of-line.
> +(define_insn "save_multiple<mode>"
> +  [(match_parallel 0 "save_multiple"
> +    [(use (match_operand:P 1 "symbol_operand"))
> +     (const_int 0)
> +    ])]
> +  "TARGET_SSE && TARGET_64BIT"
> +  "call\t%P1")

You probably don't need a (const_int 0) tag. According to the
documentation, RTX matching guarantees subexpression match, so in the
predicate you should check only stores of registers (as suggested
above).

> +;; Restore multiple registers out-of-line.
> +(define_insn "restore_multiple<mode>"
> +  [(match_parallel 0 "restore_multiple"
> +    [(use (match_operand:P 1 "symbol_operand"))])]
> +  "TARGET_SSE && TARGET_64BIT"
> +  "call\t%P1")
> +
> +;; Restore multiple registers out-of-line and return.
> +(define_insn "restore_multiple_and_return<mode>"
> +  [(match_parallel 0 "restore_multiple"
> +    [(return)
> +     (use (match_operand:P 1 "symbol_operand"))
> +     (const_int 0)
> +    ])]
> +  "TARGET_SSE && TARGET_64BIT"
> +  "jmp\t%P1")

No need for (const_int 0) tag.

> +;; Restore multiple registers out-of-line when hard frame pointer is used,
> +;; perform the leave operation prior to returning (from the function).
> +(define_insn "restore_multiple_leave_return<mode>"
> +  [(match_parallel 0 "restore_multiple"
> +    [(return)
> +     (use (match_operand:P 1 "symbol_operand"))
> +     (const_int 1)
> +    ])]
> +  "TARGET_SSE && TARGET_64BIT"
> +  "jmp\t%P1")

You will have to write out all  sub-RTXes of the "leave" pattern,
including clobber.

I'd recommend that in the predicate, you check match_parallel from the
bottom up, since subexpressions on the top are already matched, and
you can have different number of subexpressions at the top.

Uros.