| Message ID | 20170106162553.GS21933@tucnak |
|---|---|
| State | New |
| Headers | show |
On Fri, 2017-01-06 at 17:25 +0100, Jakub Jelinek wrote: > On Thu, Jan 05, 2017 at 03:20:26PM -0500, David Malcolm wrote: > > + /* Handle "reuse_rtx". */ > > + if (strcmp (code_name, "reuse_rtx") == 0) > > + { > > + read_name (&name); > > + long idx = atoi (name.string); > > + /* Look it up by ID. */ > > + gcc_assert (idx < m_reuse_rtx_by_id.length ()); > > + return_rtx = m_reuse_rtx_by_id[idx]; > > + return return_rtx; > > + } > > This broke bootstrap on i686-linux (and other ILP32 hosts), because > vec.h length () returns unsigned. Sorry about the breakage. I'm not able to approve the patch, but the fix looks to me like it would be covered under the "obvious" rule. > Is the following ok for trunk if it passes bootstrap/regtest? > > 2017-01-06 Jakub Jelinek <jakub@redhat.com> > > * read-rtl.c (rtx_reader::read_rtx_code): Avoid -Wsign-compare > warning. > > --- gcc/read-rtl.c.jj 2017-01-06 16:58:43.000000000 +0100 > +++ gcc/read-rtl.c 2017-01-06 17:22:32.105744812 +0100 > @@ -1255,7 +1255,7 @@ rtx_reader::read_rtx_code (const char *c > if (strcmp (code_name, "reuse_rtx") == 0) > { > read_name (&name); > - long idx = atoi (name.string); > + unsigned idx = atoi (name.string); > /* Look it up by ID. */ > gcc_assert (idx < m_reuse_rtx_by_id.length ()); > return_rtx = m_reuse_rtx_by_id[idx]; > > > Jakub
On 01/06/2017 09:43 AM, David Malcolm wrote: > On Fri, 2017-01-06 at 17:25 +0100, Jakub Jelinek wrote: >> On Thu, Jan 05, 2017 at 03:20:26PM -0500, David Malcolm wrote: >>> + /* Handle "reuse_rtx". */ >>> + if (strcmp (code_name, "reuse_rtx") == 0) >>> + { >>> + read_name (&name); >>> + long idx = atoi (name.string); >>> + /* Look it up by ID. */ >>> + gcc_assert (idx < m_reuse_rtx_by_id.length ()); >>> + return_rtx = m_reuse_rtx_by_id[idx]; >>> + return return_rtx; >>> + } >> >> This broke bootstrap on i686-linux (and other ILP32 hosts), because >> vec.h length () returns unsigned. > > Sorry about the breakage. > > I'm not able to approve the patch, but the fix looks to me like it > would be covered under the "obvious" rule. Can't the code string be "-1" for an insn that hasn't passed through recog yet? You can probably get those in a dump at various times. Jeff
On Fri, 2017-01-06 at 10:25 -0700, Jeff Law wrote: > On 01/06/2017 09:43 AM, David Malcolm wrote: > > On Fri, 2017-01-06 at 17:25 +0100, Jakub Jelinek wrote: > > > On Thu, Jan 05, 2017 at 03:20:26PM -0500, David Malcolm wrote: > > > > + /* Handle "reuse_rtx". */ > > > > + if (strcmp (code_name, "reuse_rtx") == 0) > > > > + { > > > > + read_name (&name); > > > > + long idx = atoi (name.string); > > > > + /* Look it up by ID. */ > > > > + gcc_assert (idx < m_reuse_rtx_by_id.length ()); > > > > + return_rtx = m_reuse_rtx_by_id[idx]; > > > > + return return_rtx; > > > > + } > > > > > > This broke bootstrap on i686-linux (and other ILP32 hosts), > > > because > > > vec.h length () returns unsigned. > > > > Sorry about the breakage. > > > > I'm not able to approve the patch, but the fix looks to me like it > > would be covered under the "obvious" rule. > Can't the code string be "-1" for an insn that hasn't passed through > recog yet? You can probably get those in a dump at various times. This is a different kind of ID, for handling identity of SCRATCH instances within a dump; see the discussion here: https://gcc.gnu.org/ml/gcc-patches/2016-10/msg01801.html They're all non-negative (see class rtx_reuse_manager, though I see now that I used an "int" rather than an "unsigned" for them - but they start at zero and go up).
On Fri, Jan 06, 2017 at 12:34:01PM -0500, David Malcolm wrote: > On Fri, 2017-01-06 at 10:25 -0700, Jeff Law wrote: > > On 01/06/2017 09:43 AM, David Malcolm wrote: > > > On Fri, 2017-01-06 at 17:25 +0100, Jakub Jelinek wrote: > > > > On Thu, Jan 05, 2017 at 03:20:26PM -0500, David Malcolm wrote: > > > > > + /* Handle "reuse_rtx". */ > > > > > + if (strcmp (code_name, "reuse_rtx") == 0) > > > > > + { > > > > > + read_name (&name); > > > > > + long idx = atoi (name.string); > > > > > + /* Look it up by ID. */ > > > > > + gcc_assert (idx < m_reuse_rtx_by_id.length ()); > > > > > + return_rtx = m_reuse_rtx_by_id[idx]; > > > > > + return return_rtx; > > > > > + } > > > > > > > > This broke bootstrap on i686-linux (and other ILP32 hosts), > > > > because > > > > vec.h length () returns unsigned. > > > > > > Sorry about the breakage. > > > > > > I'm not able to approve the patch, but the fix looks to me like it > > > would be covered under the "obvious" rule. > > Can't the code string be "-1" for an insn that hasn't passed through > > recog yet? You can probably get those in a dump at various times. > > This is a different kind of ID, for handling identity of SCRATCH > instances within a dump; see the discussion here: > https://gcc.gnu.org/ml/gcc-patches/2016-10/msg01801.html > > They're all non-negative (see class rtx_reuse_manager, though I see now > that I used an "int" rather than an "unsigned" for them - but they > start at zero and go up). Note, I've already committed the patch. Another option would be to print it as unsigned and read it as unsigned using strtoul. But even the printing of unsigned value as int and atoi + cast to unsigned should work right. Jakub
--- gcc/read-rtl.c.jj 2017-01-06 16:58:43.000000000 +0100 +++ gcc/read-rtl.c 2017-01-06 17:22:32.105744812 +0100 @@ -1255,7 +1255,7 @@ rtx_reader::read_rtx_code (const char *c if (strcmp (code_name, "reuse_rtx") == 0) { read_name (&name); - long idx = atoi (name.string); + unsigned idx = atoi (name.string); /* Look it up by ID. */ gcc_assert (idx < m_reuse_rtx_by_id.length ()); return_rtx = m_reuse_rtx_by_id[idx];