Christophe Lyon <christophe.lyon....@gmail.com> writes:
> 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.
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.
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<N>_<elt>_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,
Richard
