Fix mode checks in var-tracking.c

Fix mode checks in var-tracking.c

[Richard Sandiford]

As Jakub pointed out, my recent wrap_constant pass removed some
XPASSes in the guality.exp testsuite.  This is a repost of the
patch I suggested there, having now bootstrapped & regression-tested
it on x86_64-linux-gnu.  The idea is to check the mode of the
original variable location (which is documented as being a
VALUE, MEM or REG) instead of the expanded version, which could
be a CONST_INT.  In the second hunk, I moved the mode check above
the call to vt_expand_loc, since it's far cheaper.

The "after" results for this patch were exactly the same as the "before"
results, which wasn't what I was hoping.  By rights, the XPASSes should
have reappeared.  I looked at the logs again, and it seems many of the
guality.exp tests are failing for me when run from a normal -j5 top-level
"make check".  E.g. I get:

FAIL: b is not computable, expected 3003
FAIL: ab is not computable, expected 6296328
FAIL: ac is not computable, expected 6296332
FAIL: msg is not computable, expected 4198085
FAIL: 0 PASS, 4 FAIL, 0 UNRESOLVED
XFAIL: gcc.dg/guality/inline-params.c  -Os  execution test

both before and after the patch.  (This is one of the tests that my
patch regressed.  The other regressing tests have similar output.)

I went back and looked at my logs for the wrap_constant patch,
and saw the same thing.  So I hadn't missed the dropped XPASSes
after all; they never were XPASSes in my results to begin with.
I had to run the guality.exp separately, which is what I did when
looking at Jakub's original bug report.  The tests do XPASS after
the patch when run individually, and do fail before the patch
when run in the same way.

My guess is that I'm hitting some problem that causes the underlying gdb
session to fail in some unreported way.  But some tests do XPASS for me
even in the normal results, and that list of XPASSes has been consistent
over several runs.  So it does seem to be deterministic.

OK to install?

Richard


gcc/
	* var-tracking.c (emit_note_insn_var_location): Get the mode of
	a variable part from its REG, MEM or VALUE.

Index: gcc/var-tracking.c
===================================================================
--- gcc/var-tracking.c	2009-10-22 19:20:19.000000000 +0100
+++ gcc/var-tracking.c	2009-10-22 19:20:54.000000000 +0100
@@ -6402,7 +6402,7 @@ emit_note_insn_var_location (void **varp
 	  continue;
 	}
       loc[n_var_parts] = loc2;
-      mode = GET_MODE (loc[n_var_parts]);
+      mode = GET_MODE (var->var_part[i].loc_chain->loc);
       initialized = var->var_part[i].loc_chain->init;
       last_limit = offsets[n_var_parts] + GET_MODE_SIZE (mode);
 
@@ -6413,9 +6413,9 @@ emit_note_insn_var_location (void **varp
 	  break;
       if (j < var->n_var_parts
 	  && wider_mode != VOIDmode
+	  && mode == GET_MODE (var->var_part[j].loc_chain->loc)
 	  && (loc2 = vt_expand_loc (var->var_part[j].loc_chain->loc, vars))
 	  && GET_CODE (loc[n_var_parts]) == GET_CODE (loc2)
-	  && mode == GET_MODE (loc2)
 	  && last_limit == var->var_part[j].offset)
 	{
 	  rtx new_loc = NULL;

Re: Fix mode checks in var-tracking.c

[Jakub Jelinek]

On Thu, Oct 22, 2009 at 10:19:40PM +0100, Richard Sandiford wrote:
> The "after" results for this patch were exactly the same as the "before"
> results, which wasn't what I was hoping.  By rights, the XPASSes should
> have reappeared.  I looked at the logs again, and it seems many of the
> guality.exp tests are failing for me when run from a normal -j5 top-level
> "make check".  E.g. I get:
> 
> FAIL: b is not computable, expected 3003
> FAIL: ab is not computable, expected 6296328
> FAIL: ac is not computable, expected 6296332
> FAIL: msg is not computable, expected 4198085
> FAIL: 0 PASS, 4 FAIL, 0 UNRESOLVED
> XFAIL: gcc.dg/guality/inline-params.c  -Os  execution test
> 
> both before and after the patch.  (This is one of the tests that my
> patch regressed.  The other regressing tests have similar output.)

I guess it depends what gdb you are using.  Should be 7.0 or some prerelease
of it at least.

> gcc/
> 	* var-tracking.c (emit_note_insn_var_location): Get the mode of
> 	a variable part from its REG, MEM or VALUE.

Looks good to me (and the similar patch I've posted bootstrapped/regtested
and cured the regressions, so I don't see why this one wouldn't).
After all, before your patch check_wrap_constant that was called by
vt_expand_loc I believe asserted the mode is the same or the new mode is
VOIDmode.

> @@ -6413,9 +6413,9 @@ emit_note_insn_var_location (void **varp
>  	  break;
>        if (j < var->n_var_parts
>  	  && wider_mode != VOIDmode
> +	  && mode == GET_MODE (var->var_part[j].loc_chain->loc)

Though, I think we should also do the
	&& (REG_P (loc[n_var_parts]) || MEM_P (loc[n_var_parts]))
test here as I had in my patch, even when mode is the same one part might
be something other than reg/mem.

>  	  && (loc2 = vt_expand_loc (var->var_part[j].loc_chain->loc, vars))
>  	  && GET_CODE (loc[n_var_parts]) == GET_CODE (loc2)
> -	  && mode == GET_MODE (loc2)
>  	  && last_limit == var->var_part[j].offset)
>  	{
>  	  rtx new_loc = NULL;

	Jakub

Re: Fix mode checks in var-tracking.c

[Richard Sandiford]

Jakub Jelinek <jakub@redhat.com> writes:
> On Thu, Oct 22, 2009 at 10:19:40PM +0100, Richard Sandiford wrote:
>> gcc/
>> 	* var-tracking.c (emit_note_insn_var_location): Get the mode of
>> 	a variable part from its REG, MEM or VALUE.
>
> Looks good to me (and the similar patch I've posted bootstrapped/regtested
> and cured the regressions, so I don't see why this one wouldn't).

Doh!  Sorry for not noticing that message until now.  You cc:ed it
to rsandiford@googlemail.com rather than rdsandiford@googlemail.com,
so I don't know who got it.  (The original problem report went
to the right address FWIW.)

Must have seemed strange for you to get a patch that ignored yours
completely, sorry ;(  I wasn't deliberately ignoring it.

> After all, before your patch check_wrap_constant that was called by
> vt_expand_loc I believe asserted the mode is the same or the new mode is
> VOIDmode.

Right.  I don't think there's any reason to look at the mode of loc2 here.

>
>> @@ -6413,9 +6413,9 @@ emit_note_insn_var_location (void **varp
>>  	  break;
>>        if (j < var->n_var_parts
>>  	  && wider_mode != VOIDmode
>> +	  && mode == GET_MODE (var->var_part[j].loc_chain->loc)
>
> Though, I think we should also do the
> 	&& (REG_P (loc[n_var_parts]) || MEM_P (loc[n_var_parts]))
> test here as I had in my patch, even when mode is the same one part might
> be something other than reg/mem.

As an optimisation, you mean?  Yeah, that makes sense, but can
we leave it for a follow-on patch?  I still think this hunk is
the right fix in itself, as well as being a similar kind of
optimisation.

Richard

Re: Fix mode checks in var-tracking.c

[Jakub Jelinek]

On Thu, Oct 22, 2009 at 11:27:24PM +0100, Richard Sandiford wrote:
> Jakub Jelinek <jakub@redhat.com> writes:
> > On Thu, Oct 22, 2009 at 10:19:40PM +0100, Richard Sandiford wrote:
> >> gcc/
> >> 	* var-tracking.c (emit_note_insn_var_location): Get the mode of
> >> 	a variable part from its REG, MEM or VALUE.
> >
> > Looks good to me (and the similar patch I've posted bootstrapped/regtested
> > and cured the regressions, so I don't see why this one wouldn't).
> 
> Doh!  Sorry for not noticing that message until now.  You cc:ed it
> to rsandiford@googlemail.com rather than rdsandiford@googlemail.com,
> so I don't know who got it.  (The original problem report went
> to the right address FWIW.)

Oops, sorry.

> > After all, before your patch check_wrap_constant that was called by
> > vt_expand_loc I believe asserted the mode is the same or the new mode is
> > VOIDmode.
> 
> Right.  I don't think there's any reason to look at the mode of loc2 here.

Yeah.

> > Though, I think we should also do the
> > 	&& (REG_P (loc[n_var_parts]) || MEM_P (loc[n_var_parts]))
> > test here as I had in my patch, even when mode is the same one part might
> > be something other than reg/mem.
> 
> As an optimisation, you mean?  Yeah, that makes sense, but can
> we leave it for a follow-on patch?  I still think this hunk is
> the right fix in itself, as well as being a similar kind of
> optimisation.

Yeah, ok.  I'll submit it as follow-on patch then.

	Jakub

[PATCH] Slight emit_note_insn_var_location optimization (was Re: Fix mode checks in var-tracking.c)

[Jakub Jelinek]

On Thu, Oct 22, 2009 at 11:27:24PM +0100, Richard Sandiford wrote:
> As an optimisation, you mean?  Yeah, that makes sense, but can
> we leave it for a follow-on patch?  I still think this hunk is
> the right fix in itself, as well as being a similar kind of
> optimisation.

Here is what I've bootstrapped/regtested on x86_64-linux and i686-linux
on top of the patch you've posted.  Ok for trunk when your patch is
committed?

2009-10-23  Jakub Jelinek  <jakub@redhat.com>

	* var-tracking.c (emit_note_insn_var_location): Don't call the second
	vt_expand_loc unnecessarily when location is not a register nor
	memory.

--- gcc/var-tracking.c.jj	2009-10-23 18:41:33.000000000 +0200
+++ gcc/var-tracking.c	2009-10-23 18:54:55.000000000 +0200
@@ -6414,6 +6414,7 @@ emit_note_insn_var_location (void **varp
       if (j < var->n_var_parts
 	  && wider_mode != VOIDmode
 	  && mode == GET_MODE (var->var_part[j].loc_chain->loc)
+	  && (REG_P (loc[n_var_parts]) || MEM_P (loc[n_var_parts]))
 	  && (loc2 = vt_expand_loc (var->var_part[j].loc_chain->loc, vars))
 	  && GET_CODE (loc[n_var_parts]) == GET_CODE (loc2)
 	  && last_limit == var->var_part[j].offset)


	Jakub

Re: Fix mode checks in var-tracking.c

[Ian Lance Taylor]

Richard Sandiford <rdsandiford@googlemail.com> writes:

> gcc/
> 	* var-tracking.c (emit_note_insn_var_location): Get the mode of
> 	a variable part from its REG, MEM or VALUE.

This is OK.

Thanks.

Ian

Re: [PATCH] Slight emit_note_insn_var_location optimization (was Re: Fix mode checks in var-tracking.c)

[Ian Lance Taylor]

Jakub Jelinek <jakub@redhat.com> writes:

> 2009-10-23  Jakub Jelinek  <jakub@redhat.com>
>
> 	* var-tracking.c (emit_note_insn_var_location): Don't call the second
> 	vt_expand_loc unnecessarily when location is not a register nor
> 	memory.

This is OK.

Thanks.

Ian