ipa-cp: Fix up ipcp_print_widest_int
Checks
Commit Message
On Mon, Mar 31, 2025 at 03:34:07PM +0200, Martin Jambor wrote:
> This patch just introduces a form of dumping of widest ints that only
> have zeros in the lowest 128 bits so that instead of printing
> thousands of f's the output looks like:
>
> Bits: value = 0xffff, mask = all ones folled by 0xffffffffffffffffffffffffffff0000
>
> and then makes sure we use the function not only to print bits but
> also to print masks where values like these can also occur.
Shouldn't that be followed by instead?
And the widest_int checks seems to be quite expensive (especially for
large widest_ints), I think for the first one we can just == -1
and for the second one wi::arshift (value, 128) == -1 and the zero extension
by using wi::zext.
Anyway, I wonder if it wouldn't be better to use something shorter,
the variant patch uses 0xf..f prefix before the 128-bit hexadecimal
number (maybe we could also special case the even more common bits 64+
are all ones case). Or it could be 0xf*f prefix. Or printing such
numbers as -0x prefixed negative, though that is not a good idea for masks.
Jakub
2025-04-15 Jakub Jelinek <jakub@redhat.com>
* ipa-cp.cc (ipcp_print_widest_int): Fix a typo, folled -> followed.
Simplify wide_int check for -1 or all ones above least significant
128 bits.
2025-04-15 Jakub Jelinek <jakub@redhat.com>
* ipa-cp.cc (ipcp_print_widest_int): Print values with all ones in
bits 128+ with "0xf..f" prefix instead of "all ones folled by ".
Simplify wide_int check for -1 or all ones above least significant
128 bits.
--- gcc/ipa-cp.cc.jj 2025-04-15 07:55:18.369479825 +0200
+++ gcc/ipa-cp.cc 2025-04-15 11:54:45.369704056 +0200
@@ -313,14 +313,20 @@ ipcp_lattice<valtype>::print (FILE * f,
static void
ipcp_print_widest_int (FILE *f, const widest_int &value)
{
- if (wi::eq_p (wi::bit_not (value), 0))
+ if (value == -1)
fprintf (f, "-1");
- else if (wi::eq_p (wi::bit_not (wi::bit_or (value,
- wi::sub (wi::lshift (1, 128),
- 1))), 0))
+ else if (wi::arshift (value, 128) == -1)
{
- fprintf (f, "all ones folled by ");
- print_hex (wi::bit_and (value, wi::sub (wi::lshift (1, 128), 1)), f);
+ char buf[35];
+ widest_int v = wi::zext (value, 128);
+ size_t len;
+ print_hex (v, buf);
+ len = strlen (buf + 2);
+ if (len == 32)
+ fprintf (f, "0xf..f");
+ else
+ fprintf (f, "0xf..f%0*d", (int) (32 - len), 0);
+ fputs (buf + 2, f);
}
else
print_hex (value, f);
Comments
On Tue, 15 Apr 2025, Jakub Jelinek wrote:
> On Mon, Mar 31, 2025 at 03:34:07PM +0200, Martin Jambor wrote:
> > This patch just introduces a form of dumping of widest ints that only
> > have zeros in the lowest 128 bits so that instead of printing
> > thousands of f's the output looks like:
> >
> > Bits: value = 0xffff, mask = all ones folled by 0xffffffffffffffffffffffffffff0000
> >
> > and then makes sure we use the function not only to print bits but
> > also to print masks where values like these can also occur.
>
> Shouldn't that be followed by instead?
> And the widest_int checks seems to be quite expensive (especially for
> large widest_ints), I think for the first one we can just == -1
> and for the second one wi::arshift (value, 128) == -1 and the zero extension
> by using wi::zext.
>
> Anyway, I wonder if it wouldn't be better to use something shorter,
> the variant patch uses 0xf..f prefix before the 128-bit hexadecimal
> number (maybe we could also special case the even more common bits 64+
> are all ones case). Or it could be 0xf*f prefix. Or printing such
> numbers as -0x prefixed negative, though that is not a good idea for masks.
I'd accept 0xf..f as reasonable, possibly 0xf<repeated N times>f
when there are more than sizeof("repeated N times") fs inbetween.
It does make matching up masks more difficult when tracking changes
(from my experience with bit-CCP debugging, where such large masks
appear as well). So IMO we can live with large 0xffffff but for
all-ones we could print -1 if that's the common noisy thing.
Richard.
Hi,
On Tue, Apr 15 2025, Jakub Jelinek wrote:
> On Mon, Mar 31, 2025 at 03:34:07PM +0200, Martin Jambor wrote:
>> This patch just introduces a form of dumping of widest ints that only
>> have zeros in the lowest 128 bits so that instead of printing
>> thousands of f's the output looks like:
>>
>> Bits: value = 0xffff, mask = all ones folled by 0xffffffffffffffffffffffffffff0000
>>
>> and then makes sure we use the function not only to print bits but
>> also to print masks where values like these can also occur.
>
> Shouldn't that be followed by instead?
Yes, of course.
> And the widest_int checks seems to be quite expensive (especially for
> large widest_ints), I think for the first one we can just == -1
> and for the second one wi::arshift (value, 128) == -1 and the zero extension
> by using wi::zext.
>
> Anyway, I wonder if it wouldn't be better to use something shorter,
> the variant patch uses 0xf..f prefix before the 128-bit hexadecimal
> number (maybe we could also special case the even more common bits 64+
> are all ones case). Or it could be 0xf*f prefix. Or printing such
> numbers as -0x prefixed negative, though that is not a good idea for masks.
I was not sure myself what the best way would be and so proposed the
simplest variant I could think of. I am fine with anything that does
not print thousands of f's which could be the case before.
So if you like the second variant more, I and I guess I do as well, by
all means go ahead and commit it.
Thanks,
Martin
>
> Jakub
> 2025-04-15 Jakub Jelinek <jakub@redhat.com>
>
> * ipa-cp.cc (ipcp_print_widest_int): Fix a typo, folled -> followed.
> Simplify wide_int check for -1 or all ones above least significant
> 128 bits.
>
> --- gcc/ipa-cp.cc.jj 2025-04-15 07:55:18.369479825 +0200
> +++ gcc/ipa-cp.cc 2025-04-15 11:37:03.059964475 +0200
> @@ -313,14 +313,12 @@ ipcp_lattice<valtype>::print (FILE * f,
> static void
> ipcp_print_widest_int (FILE *f, const widest_int &value)
> {
> - if (wi::eq_p (wi::bit_not (value), 0))
> + if (value == -1)
> fprintf (f, "-1");
> - else if (wi::eq_p (wi::bit_not (wi::bit_or (value,
> - wi::sub (wi::lshift (1, 128),
> - 1))), 0))
> + else if (wi::arshift (value, 128) == -1)
> {
> - fprintf (f, "all ones folled by ");
> - print_hex (wi::bit_and (value, wi::sub (wi::lshift (1, 128), 1)), f);
> + fprintf (f, "all ones followed by ");
> + print_hex (wi::zext (value, 128), f);
> }
> else
> print_hex (value, f);
> 2025-04-15 Jakub Jelinek <jakub@redhat.com>
>
> * ipa-cp.cc (ipcp_print_widest_int): Print values with all ones in
> bits 128+ with "0xf..f" prefix instead of "all ones folled by ".
> Simplify wide_int check for -1 or all ones above least significant
> 128 bits.
>
> --- gcc/ipa-cp.cc.jj 2025-04-15 07:55:18.369479825 +0200
> +++ gcc/ipa-cp.cc 2025-04-15 11:54:45.369704056 +0200
> @@ -313,14 +313,20 @@ ipcp_lattice<valtype>::print (FILE * f,
> static void
> ipcp_print_widest_int (FILE *f, const widest_int &value)
> {
> - if (wi::eq_p (wi::bit_not (value), 0))
> + if (value == -1)
> fprintf (f, "-1");
> - else if (wi::eq_p (wi::bit_not (wi::bit_or (value,
> - wi::sub (wi::lshift (1, 128),
> - 1))), 0))
> + else if (wi::arshift (value, 128) == -1)
> {
> - fprintf (f, "all ones folled by ");
> - print_hex (wi::bit_and (value, wi::sub (wi::lshift (1, 128), 1)), f);
> + char buf[35];
> + widest_int v = wi::zext (value, 128);
> + size_t len;
> + print_hex (v, buf);
> + len = strlen (buf + 2);
> + if (len == 32)
> + fprintf (f, "0xf..f");
> + else
> + fprintf (f, "0xf..f%0*d", (int) (32 - len), 0);
> + fputs (buf + 2, f);
> }
> else
> print_hex (value, f);
On Tue, Apr 15, 2025 at 02:17:46PM +0200, Martin Jambor wrote:
> Hi,
>
> On Tue, Apr 15 2025, Jakub Jelinek wrote:
> > On Mon, Mar 31, 2025 at 03:34:07PM +0200, Martin Jambor wrote:
> >> This patch just introduces a form of dumping of widest ints that only
> >> have zeros in the lowest 128 bits so that instead of printing
> >> thousands of f's the output looks like:
> >>
> >> Bits: value = 0xffff, mask = all ones folled by 0xffffffffffffffffffffffffffff0000
> >>
> >> and then makes sure we use the function not only to print bits but
> >> also to print masks where values like these can also occur.
> >
> > Shouldn't that be followed by instead?
>
> Yes, of course.
>
> > And the widest_int checks seems to be quite expensive (especially for
> > large widest_ints), I think for the first one we can just == -1
> > and for the second one wi::arshift (value, 128) == -1 and the zero extension
> > by using wi::zext.
> >
> > Anyway, I wonder if it wouldn't be better to use something shorter,
> > the variant patch uses 0xf..f prefix before the 128-bit hexadecimal
> > number (maybe we could also special case the even more common bits 64+
> > are all ones case). Or it could be 0xf*f prefix. Or printing such
> > numbers as -0x prefixed negative, though that is not a good idea for masks.
>
> I was not sure myself what the best way would be and so proposed the
> simplest variant I could think of. I am fine with anything that does
> not print thousands of f's which could be the case before.
>
> So if you like the second variant more, I and I guess I do as well, by
> all means go ahead and commit it.
Here is perhaps even better one which doesn't print e.g.
0xf..fffffffffffffffffffffffffffff0000
but just
0xf..f0000
(of course, for say mask of
0xf..f0000000000000000000000000000ffff
it prints it like that, doesn't try to shorten the 0 digits.
But if the most significant bits aren't set, it will be just
0xffff
2025-04-15 Jakub Jelinek <jakub@redhat.com>
* ipa-cp.cc (ipcp_print_widest_int): Print values with all ones in
bits 128+ with "0xf..f" prefix instead of "all ones folled by ".
Simplify wide_int check for -1 or all ones above least significant
128 bits.
--- gcc/ipa-cp.cc.jj 2025-04-15 12:22:07.485558525 +0200
+++ gcc/ipa-cp.cc 2025-04-15 14:12:01.327407951 +0200
@@ -313,14 +313,24 @@ ipcp_lattice<valtype>::print (FILE * f,
static void
ipcp_print_widest_int (FILE *f, const widest_int &value)
{
- if (wi::eq_p (wi::bit_not (value), 0))
+ if (value == -1)
fprintf (f, "-1");
- else if (wi::eq_p (wi::bit_not (wi::bit_or (value,
- wi::sub (wi::lshift (1, 128),
- 1))), 0))
+ else if (wi::arshift (value, 128) == -1)
{
- fprintf (f, "all ones folled by ");
- print_hex (wi::bit_and (value, wi::sub (wi::lshift (1, 128), 1)), f);
+ char buf[35], *p = buf + 2;
+ widest_int v = wi::zext (value, 128);
+ size_t len;
+ print_hex (v, buf);
+ len = strlen (p);
+ if (len == 32)
+ {
+ fprintf (f, "0xf..f");
+ while (*p == 'f')
+ ++p;
+ }
+ else
+ fprintf (f, "0xf..f%0*d", (int) (32 - len), 0);
+ fputs (p, f);
}
else
print_hex (value, f);
Jakub
On Tue, Apr 15, 2025 at 01:56:25PM +0200, Richard Biener wrote:
> On Tue, 15 Apr 2025, Jakub Jelinek wrote:
>
> > On Mon, Mar 31, 2025 at 03:34:07PM +0200, Martin Jambor wrote:
> > > This patch just introduces a form of dumping of widest ints that only
> > > have zeros in the lowest 128 bits so that instead of printing
> > > thousands of f's the output looks like:
> > >
> > > Bits: value = 0xffff, mask = all ones folled by 0xffffffffffffffffffffffffffff0000
> > >
> > > and then makes sure we use the function not only to print bits but
> > > also to print masks where values like these can also occur.
> >
> > Shouldn't that be followed by instead?
> > And the widest_int checks seems to be quite expensive (especially for
> > large widest_ints), I think for the first one we can just == -1
> > and for the second one wi::arshift (value, 128) == -1 and the zero extension
> > by using wi::zext.
> >
> > Anyway, I wonder if it wouldn't be better to use something shorter,
> > the variant patch uses 0xf..f prefix before the 128-bit hexadecimal
> > number (maybe we could also special case the even more common bits 64+
> > are all ones case). Or it could be 0xf*f prefix. Or printing such
> > numbers as -0x prefixed negative, though that is not a good idea for masks.
>
> I'd accept 0xf..f as reasonable, possibly 0xf<repeated N times>f
> when there are more than sizeof("repeated N times") fs inbetween.
> It does make matching up masks more difficult when tracking changes
> (from my experience with bit-CCP debugging, where such large masks
> appear as well). So IMO we can live with large 0xffffff but for
> all-ones we could print -1 if that's the common noisy thing.
There is already -1 special case, the problem which Martin was trying
to solve is that the 0xf<32762 times f>f0000 and similar cases were
still way too common.
The primary problem is using wrong type for it, IMHO both ipa-cp and
tree-ssa-ccp should be using wide_int with the precision of the particular
SSA_NAME etc., rather than widest_int. But there is no chance rewriting
that now for GCC 15.
Jakub
On Tue, 15 Apr 2025, Jakub Jelinek wrote:
> On Tue, Apr 15, 2025 at 01:56:25PM +0200, Richard Biener wrote:
> > On Tue, 15 Apr 2025, Jakub Jelinek wrote:
> >
> > > On Mon, Mar 31, 2025 at 03:34:07PM +0200, Martin Jambor wrote:
> > > > This patch just introduces a form of dumping of widest ints that only
> > > > have zeros in the lowest 128 bits so that instead of printing
> > > > thousands of f's the output looks like:
> > > >
> > > > Bits: value = 0xffff, mask = all ones folled by 0xffffffffffffffffffffffffffff0000
> > > >
> > > > and then makes sure we use the function not only to print bits but
> > > > also to print masks where values like these can also occur.
> > >
> > > Shouldn't that be followed by instead?
> > > And the widest_int checks seems to be quite expensive (especially for
> > > large widest_ints), I think for the first one we can just == -1
> > > and for the second one wi::arshift (value, 128) == -1 and the zero extension
> > > by using wi::zext.
> > >
> > > Anyway, I wonder if it wouldn't be better to use something shorter,
> > > the variant patch uses 0xf..f prefix before the 128-bit hexadecimal
> > > number (maybe we could also special case the even more common bits 64+
> > > are all ones case). Or it could be 0xf*f prefix. Or printing such
> > > numbers as -0x prefixed negative, though that is not a good idea for masks.
> >
> > I'd accept 0xf..f as reasonable, possibly 0xf<repeated N times>f
> > when there are more than sizeof("repeated N times") fs inbetween.
> > It does make matching up masks more difficult when tracking changes
> > (from my experience with bit-CCP debugging, where such large masks
> > appear as well). So IMO we can live with large 0xffffff but for
> > all-ones we could print -1 if that's the common noisy thing.
>
> There is already -1 special case, the problem which Martin was trying
> to solve is that the 0xf<32762 times f>f0000 and similar cases were
> still way too common.
> The primary problem is using wrong type for it, IMHO both ipa-cp and
> tree-ssa-ccp should be using wide_int with the precision of the particular
> SSA_NAME etc., rather than widest_int. But there is no chance rewriting
> that now for GCC 15.
ISTR at least CCP _prints_ wide_int (or it's tree form), not widest_int
(but the lattice has widest_int indeed). But CCP knows the precision
of the lattice entry (which is for an SSA name), possibly IPA CP
doesn't.
Richard.
On Tue, Apr 15, 2025 at 02:33:23PM +0200, Richard Biener wrote:
> ISTR at least CCP _prints_ wide_int (or it's tree form), not widest_int
> (but the lattice has widest_int indeed). But CCP knows the precision
> of the lattice entry (which is for an SSA name), possibly IPA CP
> doesn't.
ipcp_bits_lattice that is being printed doesn't seem to know the precision.
If it used wide_int, it would know it trivially from get_precision ()
on the m_value and m_mask.
Jakub
Hi,
On Tue, Apr 15 2025, Jakub Jelinek wrote:
> On Tue, Apr 15, 2025 at 02:17:46PM +0200, Martin Jambor wrote:
>> Hi,
>>
>> On Tue, Apr 15 2025, Jakub Jelinek wrote:
>> > On Mon, Mar 31, 2025 at 03:34:07PM +0200, Martin Jambor wrote:
>> >> This patch just introduces a form of dumping of widest ints that only
>> >> have zeros in the lowest 128 bits so that instead of printing
>> >> thousands of f's the output looks like:
>> >>
>> >> Bits: value = 0xffff, mask = all ones folled by 0xffffffffffffffffffffffffffff0000
>> >>
>> >> and then makes sure we use the function not only to print bits but
>> >> also to print masks where values like these can also occur.
>> >
>> > Shouldn't that be followed by instead?
>>
>> Yes, of course.
>>
>> > And the widest_int checks seems to be quite expensive (especially for
>> > large widest_ints), I think for the first one we can just == -1
>> > and for the second one wi::arshift (value, 128) == -1 and the zero extension
>> > by using wi::zext.
>> >
>> > Anyway, I wonder if it wouldn't be better to use something shorter,
>> > the variant patch uses 0xf..f prefix before the 128-bit hexadecimal
>> > number (maybe we could also special case the even more common bits 64+
>> > are all ones case). Or it could be 0xf*f prefix. Or printing such
>> > numbers as -0x prefixed negative, though that is not a good idea for masks.
>>
>> I was not sure myself what the best way would be and so proposed the
>> simplest variant I could think of. I am fine with anything that does
>> not print thousands of f's which could be the case before.
>>
>> So if you like the second variant more, I and I guess I do as well, by
>> all means go ahead and commit it.
>
> Here is perhaps even better one which doesn't print e.g.
> 0xf..fffffffffffffffffffffffffffff0000
> but just
> 0xf..f0000
> (of course, for say mask of
> 0xf..f0000000000000000000000000000ffff
> it prints it like that, doesn't try to shorten the 0 digits.
> But if the most significant bits aren't set, it will be just
> 0xffff
>
> 2025-04-15 Jakub Jelinek <jakub@redhat.com>
>
> * ipa-cp.cc (ipcp_print_widest_int): Print values with all ones in
> bits 128+ with "0xf..f" prefix instead of "all ones folled by ".
> Simplify wide_int check for -1 or all ones above least significant
> 128 bits.
That is great, thank you.
Martin
>
> --- gcc/ipa-cp.cc.jj 2025-04-15 12:22:07.485558525 +0200
> +++ gcc/ipa-cp.cc 2025-04-15 14:12:01.327407951 +0200
> @@ -313,14 +313,24 @@ ipcp_lattice<valtype>::print (FILE * f,
> static void
> ipcp_print_widest_int (FILE *f, const widest_int &value)
> {
> - if (wi::eq_p (wi::bit_not (value), 0))
> + if (value == -1)
> fprintf (f, "-1");
> - else if (wi::eq_p (wi::bit_not (wi::bit_or (value,
> - wi::sub (wi::lshift (1, 128),
> - 1))), 0))
> + else if (wi::arshift (value, 128) == -1)
> {
> - fprintf (f, "all ones folled by ");
> - print_hex (wi::bit_and (value, wi::sub (wi::lshift (1, 128), 1)), f);
> + char buf[35], *p = buf + 2;
> + widest_int v = wi::zext (value, 128);
> + size_t len;
> + print_hex (v, buf);
> + len = strlen (p);
> + if (len == 32)
> + {
> + fprintf (f, "0xf..f");
> + while (*p == 'f')
> + ++p;
> + }
> + else
> + fprintf (f, "0xf..f%0*d", (int) (32 - len), 0);
> + fputs (p, f);
> }
> else
> print_hex (value, f);
>
>
> Jakub
@@ -313,14 +313,12 @@ ipcp_lattice<valtype>::print (FILE * f,
static void
ipcp_print_widest_int (FILE *f, const widest_int &value)
{
- if (wi::eq_p (wi::bit_not (value), 0))
+ if (value == -1)
fprintf (f, "-1");
- else if (wi::eq_p (wi::bit_not (wi::bit_or (value,
- wi::sub (wi::lshift (1, 128),
- 1))), 0))
+ else if (wi::arshift (value, 128) == -1)
{
- fprintf (f, "all ones folled by ");
- print_hex (wi::bit_and (value, wi::sub (wi::lshift (1, 128), 1)), f);
+ fprintf (f, "all ones followed by ");
+ print_hex (wi::zext (value, 128), f);
}
else
print_hex (value, f);