disintegrate integrate.[ch]

On Tue, May 29, 2012 at 8:43 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> The attached patch moves the code from integrate.c to (what I hope you
>> agree to be) better places:
>>
>> * inliner code goes to tree-inline.c
>> * functions only called from dwarf2out.c are moved there.
>> * allocate_initial_values is moved to ira.c
>> * the initial-value stuff is moved to function.c
>>
>> The rest is just mechanical updates: Don't include integrate.h
>> anywhere, and include function.h if something is needed from there.
>>
>> The files integrate.c and integrate.h can be removed after this change.
>
> Please just drop the reference to integrate.c in expmed.c, there will be no
> point in keeping it after the remnants of the old inliner are eliminated.

Yes, I realize that, but it looks like that code is not doing
something because old integrate.c choked on it. Quoting that part of
the patch:


Does that "But not if..." comment refer to the "if (tmode != mode)
subtarget = 0;" line? Of so, can/should we drop that line (as well as
the comment)? I don't know this part of the compiler well enough to
tell. You're much more into this than me, so perhaps you can help ;-)

Is the patch otherwise OK?

Ciao!
Steven

> Yes, I realize that, but it looks like that code is not doing
> something because old integrate.c choked on it. Quoting that part of
> the patch:
>
> Index: expmed.c
> ===================================================================
> --- expmed.c	(revision 187936)
> +++ expmed.c	(working copy)
> @@ -1866,6 +1866,9 @@ extract_fixed_bit_field (enum machine_mode tmode,
>  	     shift it so it does.  */
>  	  /* Maybe propagate the target for the shift.  */
>  	  /* But not if we will return it--could confuse integrate.c.  */
> +	  /* ??? What does the above comment mean in relation to the code
> +	     below?  NB, integrate.c is no more, so I guess it can't be
> +	     confused by anything anymore...  */
>  	  rtx subtarget = (target != 0 && REG_P (target) ? target : 0);
>  	  if (tmode != mode) subtarget = 0;
>  	  op0 = expand_shift (RSHIFT_EXPR, mode, op0, bitpos, subtarget, 1);
>
> Does that "But not if..." comment refer to the "if (tmode != mode)
> subtarget = 0;" line? Of so, can/should we drop that line (as well as
> the comment)? I don't know this part of the compiler well enough to
> tell.

I don't think it's a direct reference.  There is the same pattern at the end of 
the function, where TARGET has already been zeroed if mode != tmode.  If the 
mode don't match, expand_shift cannot reuse [SUB]TARGET as-is, so the code 
consistently avoids to pass it.

I'd just drop the dangling reference to integrate.c and reformat the line

if (tmode != mode) subtarget = 0;

into

if (mode != mode)
  subtarget = 0;

to make it more closely ressemble the other instance.

On Tue, May 29, 2012 at 11:17 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Yes, I realize that, but it looks like that code is not doing
>> something because old integrate.c choked on it. Quoting that part of
>> the patch:
>>
>> Index: expmed.c
>> ===================================================================
>> --- expmed.c  (revision 187936)
>> +++ expmed.c  (working copy)
>> @@ -1866,6 +1866,9 @@ extract_fixed_bit_field (enum machine_mode tmode,
>>            shift it so it does.  */
>>         /* Maybe propagate the target for the shift.  */
>>         /* But not if we will return it--could confuse integrate.c.  */
>> +       /* ??? What does the above comment mean in relation to the code
>> +          below?  NB, integrate.c is no more, so I guess it can't be
>> +          confused by anything anymore...  */
>>         rtx subtarget = (target != 0 && REG_P (target) ? target : 0);
>>         if (tmode != mode) subtarget = 0;
>>         op0 = expand_shift (RSHIFT_EXPR, mode, op0, bitpos, subtarget, 1);
>>
>> Does that "But not if..." comment refer to the "if (tmode != mode)
>> subtarget = 0;" line? Of so, can/should we drop that line (as well as
>> the comment)? I don't know this part of the compiler well enough to
>> tell.
>
> I don't think it's a direct reference.  There is the same pattern at the end of
> the function, where TARGET has already been zeroed if mode != tmode.  If the
> mode don't match, expand_shift cannot reuse [SUB]TARGET as-is, so the code
> consistently avoids to pass it.
>
> I'd just drop the dangling reference to integrate.c and reformat the line
>
> if (tmode != mode) subtarget = 0;
>
> into
>
> if (mode != mode)
>  subtarget = 0;
>
> to make it more closely ressemble the other instance.

Done, updated diff attached. OK for trunk?

Ciao!
Steven

> Done, updated diff attached.

You also need to adjust the ChangeLog:

	* expmed.c (extract_fixed_bit_field): Add ??? for seemingly
	outdated comment about integrate.c.

> OK for trunk? 

I can only formally approve the cse.c and expmed.c hunks, so you probably need 
to run this by a GR.  Nice cleanup in any case!

On 12-05-29 11:09 , Eric Botcazou wrote:

> I can only formally approve the cse.c and expmed.c hunks, so you probably need
> to run this by a GR.

The other changes are OK.

 > Nice cleanup in any case!

Indeed.  Though you probably made my next cxx-conversion merge more 
difficult ;)

Thanks for doing this.


Diego.