On Tue, Feb 1, 2022 at 4:42 AM Richard Sandiford <richard.sandif...@arm.com>
wrote:

> 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.
>

Indeed!

So I changed the above loop into:
   /* For BImode, 1 and -1 are unsigned and signed interpretations
     of the same value.  */
  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;
+      const_tiny_rtx[3][(int) mode] = constm1_rtx;
    }
which works, both constants are now equal and the validation still passes.



> >> > @@ -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.
>
> Ack, I have now:
   for (c = 0; c < MAX_MODE_CLASS; c++)
    {
      /* Bleah, all this to get the comment right for MIN_MODE_INT.  */
      struct mode_data *m = modes[c];
      while (m && m->boolean)
        m = m->next;
      const char *comment_name = (m ? m : void_mode)->name;

      tagged_printf ("MIN_%s", mode_class_names[c], comment_name);
    }


Andre, any chance you tried the suggestion of:
ENTRY (Pred1x16_t, V16BI, predicate, 16, pred1, 21)
ENTRY (Pred2x8_t, V8BI, predicate, 8, pred1, 21)
ENTRY (Pred4x4_t, V4BI, predicate, 4, pred1, 21)


Thanks,
Christophe




> Thanks,
> Richard
>

Reply via email to