Re: [patch] disintegrate integrate.[ch]
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. -- Eric Botcazou
Re: [patch] disintegrate integrate.[ch]
On Tue, May 29, 2012 at 8:43 AM, Eric Botcazou ebotca...@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: 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. You're much more into this than me, so perhaps you can help ;-) Is the patch otherwise OK? Ciao! Steven
Re: [patch] disintegrate integrate.[ch]
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. -- Eric Botcazou
Re: [patch] disintegrate integrate.[ch]
On Tue, May 29, 2012 at 11:17 AM, Eric Botcazou ebotca...@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 disintegrate_integrate.diff Description: Binary data
Re: [patch] disintegrate integrate.[ch]
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! -- Eric Botcazou
Re: [patch] disintegrate integrate.[ch]
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.
