Hi, On Fri, Feb 4, 2022 at 10:43 AM Richard Sandiford wrote: > Christophe Lyon writes: > > On Tue, Feb 1, 2022 at 4:42 AM Richard Sandiford < > richard.sandiford@arm.com> > > wrote: > > > >> Christophe Lyon via Gcc-patches 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 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. > > I think we need to keep const_true_rtx for both [BImode][1] and > [BImode][3]. > BImode is an awkward special case in that the (only) nonzero value must be > exactly STORE_FLAG_VALUE, even if that leads to an otherwise non-canonical > const_int representation. > OK, done. > > For the multi-bit booleans, [1] needs to be const1_rtx rather than > const_true_rtx in case STORE_FLAG_VALUE != 1. > > >> >> > @@ -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) > > BTW: the final argument should be the length of the __simd__t > type name (for mangling purposes). It looks like the existing 32-bit > and 64-bit bfloat entries also get this wrong. > > But as far as Andre's point goes: I think we need to construct > a boolean type explicitly, using build_truth_vector_type_for_mode > or truth_type_for. Although the entries above specify the correct mode > (V16BI, etc.), the mode is really a function of the type tree properties, > rather than the other way round. > > The main thing that makes truth vector types special is that those > types are the only ones that allow multiple elements in the same byte. > A “normal” 16-byte vector created by build_vector_type(_for_mode) > cannot be smaller than 16 bytes. > > Thanks for the help, here is a new version of this patch, which contains all the changes requested. If OK, I'll rebase and commit the series. Thanks Christophe > Thanks, > Richard >