Re: [PATCH v3 07/15] arm: Implement MVE predicates as vectors of booleans

[Richard Sandiford <richard.sandiford@arm.com>]

Christophe Lyon via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> 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?

Ah, right, I forgot that the mode is unused for the small constant lookup.
But it looks like CONSTM1_RTX (B2Imode) would be (const_int 1) instead,
even though the two should be equal.

>> > @@ -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;
>           }
>  +    }

Yeah.  I just meant that the alternative loop was probably simpler,
as a replacement for the outer “if”.

It looks like that the outer “if” is effectively a peeled iteration of
the while loop in the outer “else”.  And the “c != MODE_INT” part ought
to be redundant: as it stands, the boolean modes don't belong to any class.

Thanks,
Richard