public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Christophe Lyon <christophe.lyon.oss@gmail.com>
To: Richard Sandiford <richard.sandiford@arm.com>,
	 Christophe Lyon via Gcc-patches <gcc-patches@gcc.gnu.org>,
	 Andre Simoes Dias Vieira <andre.simoesdiasvieira@arm.com>
Subject: Re: [PATCH v3 07/15] arm: Implement MVE predicates as vectors of booleans
Date: Mon, 31 Jan 2022 23:57:45 +0100	[thread overview]
Message-ID: <CAKhMtS+a2phexCE=jowRhi6QPinj97nHw+CwEeQpBSmOgPs7_Q@mail.gmail.com> (raw)
In-Reply-To: <mpttudjiy3c.fsf@arm.com>

On Mon, Jan 31, 2022 at 7:01 PM Richard Sandiford via Gcc-patches <
gcc-patches@gcc.gnu.org> wrote:

> Sorry for the slow response, was out last week.
>
> Christophe Lyon via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> > diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
> > index feeee16d320..5f559f8fd93 100644
> > --- a/gcc/emit-rtl.c
> > +++ b/gcc/emit-rtl.c
> > @@ -6239,9 +6239,14 @@ init_emit_once (void)
> >
> >    /* For BImode, 1 and -1 are unsigned and signed interpretations
> >       of the same value.  */
> > -  const_tiny_rtx[0][(int) BImode] = const0_rtx;
> > -  const_tiny_rtx[1][(int) BImode] = const_true_rtx;
> > -  const_tiny_rtx[3][(int) BImode] = const_true_rtx;
> > +  for (mode = MIN_MODE_BOOL;
> > +       mode <= MAX_MODE_BOOL;
> > +       mode = (machine_mode)((int)(mode) + 1))
> > +    {
> > +      const_tiny_rtx[0][(int) mode] = const0_rtx;
> > +      const_tiny_rtx[1][(int) mode] = const_true_rtx;
> > +      const_tiny_rtx[3][(int) mode] = const_true_rtx;
> > +    }
> >
> >    for (mode = MIN_MODE_PARTIAL_INT;
> >         mode <= MAX_MODE_PARTIAL_INT;
>
> Does this do the right thing for:
>
>   gen_int_mode (-1, B2Imode)
>
> (which is used e.g. in native_decode_vector_rtx)?  It looks like it
> would give 0b01 rather than 0b11.
>
> Maybe for non-BImode we should use const1_rtx and constm1_rtx, like with
> MODE_INT.
>

debug_rtx ( gen_int_mode (-1, B2Imode) says:
(const_int -1 [0xffffffffffffffff])
so that looks right?


> > @@ -1298,9 +1315,21 @@ enum machine_mode\n{");
> >        /* Don't use BImode for MIN_MODE_INT, since otherwise the middle
> >        end will try to use it for bitfields in structures and the
> >        like, which we do not want.  Only the target md file should
> > -      generate BImode widgets.  */
> > -      if (first && first->precision == 1 && c == MODE_INT)
> > -     first = first->next;
> > +      generate BImode widgets.  Since some targets such as ARM/MVE
> > +      define boolean modes with multiple bits, handle those too.  */
> > +      if (first && first->boolean)
> > +     {
> > +       struct mode_data *last_bool = first;
> > +       printf ("  MIN_MODE_BOOL = E_%smode,\n", first->name);
> > +
> > +       while (first && first->boolean)
> > +         {
> > +           last_bool = first;
> > +           first = first->next;
> > +         }
> > +
> > +       printf ("  MAX_MODE_BOOL = E_%smode,\n\n", last_bool->name);
> > +     }
> >
> >        if (first && last)
> >       printf ("  MIN_%s = E_%smode,\n  MAX_%s = E_%smode,\n\n",
>
> For the record: this means that MIN_MODE_BOOL and MAX_MODE_BOOL are
> in principle only conditionally available, whereas:
>
>    /* For BImode, 1 and -1 are unsigned and signed interpretations
>       of the same value.  */
> -  const_tiny_rtx[0][(int) BImode] = const0_rtx;
> -  const_tiny_rtx[1][(int) BImode] = const_true_rtx;
> -  const_tiny_rtx[3][(int) BImode] = const_true_rtx;
> +  for (mode = MIN_MODE_BOOL;
> +       mode <= MAX_MODE_BOOL;
> +       mode = (machine_mode)((int)(mode) + 1))
> +    {
> +      const_tiny_rtx[0][(int) mode] = const0_rtx;
> +      const_tiny_rtx[1][(int) mode] = const_true_rtx;
> +      const_tiny_rtx[3][(int) mode] = const_true_rtx;
> +    }
>
> assumes that they are unconditionally available.  In some ways it
> might be clearer if we assert that first->boolean is true and
> emit the MIN/MAX stuff unconditionally.
>
> However, that would make the generator less robust against malformed
> input, and it would probably be inconsistent with the current generator
> code, so I agree that the patch's version is better on balance.
>
ack


>
> > @@ -1679,15 +1708,25 @@ emit_class_narrowest_mode (void)
> >    print_decl ("unsigned char", "class_narrowest_mode",
> "MAX_MODE_CLASS");
> >
> >    for (c = 0; c < MAX_MODE_CLASS; c++)
> > -    /* Bleah, all this to get the comment right for MIN_MODE_INT.  */
> > -    tagged_printf ("MIN_%s", mode_class_names[c],
> > -                modes[c]
> > -                ? ((c != MODE_INT || modes[c]->precision != 1)
> > -                   ? modes[c]->name
> > -                   : (modes[c]->next
> > -                      ? modes[c]->next->name
> > -                      : void_mode->name))
> > -                : void_mode->name);
> > +    {
> > +      /* Bleah, all this to get the comment right for MIN_MODE_INT.  */
> > +      const char *comment_name = void_mode->name;
> > +
> > +      if (modes[c])
> > +     if (c != MODE_INT || !modes[c]->boolean)
> > +       comment_name = modes[c]->name;
> > +     else
> > +       {
> > +         struct mode_data *m = modes[c];
> > +         while (m->boolean)
> > +           m = m->next;
> > +         if (m)
> > +           comment_name = m->name;
> > +         else
> > +           comment_name = void_mode->name;
> > +       }
>
> Have you tried bootstrapping the patch on a host of your choice?
> I would expect a warning/Werror about an ambiguous else here.
>
No I hadn't and indeed the build fails

>
> I guess this reduces to:
>
>     struct mode_data *m = modes[c];
>     while (m && m->boolean)
>       m = m->next;
>     const char *comment_name = (m ? m : void_mode)->name;
>
> but I don't know if that's more readable.
>
but to my understanding the problem is that the ambiguous else
is the first one, and the code should read:
 if (modes[c])
+      {
        if (c != MODE_INT || !modes[c]->boolean)
          comment_name = modes[c]->name;
        else
          {
            struct mode_data *m = modes[c];
            while (m->boolean)
              m = m->next;
            if (m)
              comment_name = m->name;
            else
              comment_name = void_mode->name;
          }
 +    }

LGTM otherwise.
>
Thanks.

Andre, what about you? Did you try my suggestion to use
 ENTRY (Pred1x16_t, V16BI, predicate, 16, pred1, 21)
ENTRY (Pred2x8_t, V8BI, predicate, 8, pred1, 21)
ENTRY (Pred4x4_t, V4BI, predicate, 4, pred1, 21)

Does that work for you?

Christophe


> Thanks,
> Richard
>

  reply	other threads:[~2022-01-31 22:57 UTC|newest]

Thread overview: 54+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-13 14:56 [PATCH v3 00/15] ARM/MVE use vectors of boolean for predicates Christophe Lyon
2022-01-13 14:56 ` [PATCH v3 01/15] arm: Add new tests for comparison vectorization with Neon and MVE Christophe Lyon
2022-01-13 14:56 ` [PATCH v3 02/15] arm: Add tests for PR target/100757 Christophe Lyon
2022-01-13 14:56 ` [PATCH v3 03/15] arm: Add tests for PR target/101325 Christophe Lyon
2022-01-13 14:56 ` [PATCH v3 04/15] arm: Add GENERAL_AND_VPR_REGS regclass Christophe Lyon
2022-01-19 18:17   ` Andre Vieira (lists)
2022-01-20  9:14     ` Christophe Lyon
2022-01-20  9:43       ` Andre Vieira (lists)
2022-01-20 10:40         ` Richard Sandiford
2022-01-20 10:45           ` Andre Vieira (lists)
2022-01-27 16:21   ` Kyrylo Tkachov
2022-01-13 14:56 ` [PATCH v3 05/15] arm: Add support for VPR_REG in arm_class_likely_spilled_p Christophe Lyon
2022-01-19 18:25   ` Andre Vieira (lists)
2022-01-20  9:20     ` Christophe Lyon
2022-01-13 14:56 ` [PATCH v3 06/15] arm: Fix mve_vmvnq_n_<supf><mode> argument mode Christophe Lyon
2022-01-19 19:03   ` Andre Vieira (lists)
2022-01-20  9:23     ` Christophe Lyon
2022-01-20  9:38       ` Andre Simoes Dias Vieira
2022-01-20  9:44         ` Christophe Lyon
2022-01-20 10:45     ` Richard Sandiford
2022-01-20 11:06       ` Andre Vieira (lists)
2022-01-13 14:56 ` [PATCH v3 07/15] arm: Implement MVE predicates as vectors of booleans Christophe Lyon
2022-01-21 11:20   ` Andre Vieira (lists)
2022-01-21 22:30     ` Christophe Lyon
2022-01-27 16:28   ` Kyrylo Tkachov
2022-01-27 18:10     ` Christophe Lyon
2022-01-31 18:01   ` Richard Sandiford
2022-01-31 22:57     ` Christophe Lyon [this message]
2022-02-01  3:42       ` Richard Sandiford
2022-02-02 16:51         ` Christophe Lyon
2022-02-04  9:42           ` Richard Sandiford
2022-02-04  9:54             ` Richard Sandiford
2022-02-17 15:39             ` Christophe Lyon
2022-02-21 18:18               ` Richard Sandiford
2022-01-13 14:56 ` [PATCH v3 08/15] arm: Implement auto-vectorized MVE comparisons with vectors of boolean predicates Christophe Lyon
2022-01-27 16:37   ` Kyrylo Tkachov
2022-01-13 14:56 ` [PATCH v3 09/15] arm: Fix vcond_mask expander for MVE (PR target/100757) Christophe Lyon
2022-01-27 16:55   ` Kyrylo Tkachov
2022-01-13 14:56 ` [PATCH v3 10/15] arm: Convert remaining MVE vcmp builtins to predicate qualifiers Christophe Lyon
2022-01-13 14:56 ` [PATCH v3 11/15] arm: Convert more MVE " Christophe Lyon
2022-01-13 14:56 ` [PATCH v3 12/15] arm: Convert more load/store " Christophe Lyon
2022-01-27 16:56   ` Kyrylo Tkachov
2022-01-13 14:56 ` [PATCH v3 13/15] arm: Convert more MVE/CDE " Christophe Lyon
2022-01-27 16:56   ` Kyrylo Tkachov
2022-01-13 14:56 ` [PATCH v3 14/15] arm: Add VPR_REG to ALL_REGS Christophe Lyon
2022-01-13 14:56 ` [PATCH v3 15/15] arm: Fix constraint check for V8HI in mve_vector_mem_operand Christophe Lyon
2022-01-14 17:03   ` [arm] MVE: Relax addressing modes for full loads and stores Andre Vieira (lists)
2022-01-17  7:48     ` Christophe Lyon
2022-03-07 14:16       ` Andre Vieira (lists)
2022-03-07 16:14         ` Kyrylo Tkachov
2022-01-14 13:18 ` [PATCH v3 00/15] ARM/MVE use vectors of boolean for predicates Christophe Lyon
2022-01-14 13:33   ` Richard Biener
2022-01-14 14:22     ` Kyrylo Tkachov
2022-01-26  8:40       ` Christophe Lyon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAKhMtS+a2phexCE=jowRhi6QPinj97nHw+CwEeQpBSmOgPs7_Q@mail.gmail.com' \
    --to=christophe.lyon.oss@gmail.com \
    --cc=andre.simoesdiasvieira@arm.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=richard.sandiford@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).