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 >