Re: [PATCH] Forward propagation on RTL (4.1?)

["Giovanni Bajo" <rasky at develer dot com>]

Paolo Bonzini <paolo.bonzini@lu.unisi.ch> wrote:

>> Bootstrap and SPEC benchmarking was done with -fno-cse-follow-jumps and
>> -fno-cse-skip-blocks, in order to validate the patch as a replacement
>> for CSE path following.
>>
>> Bootstrap is sped up by 2.75% on i686-pc-linux-gnu and the attached
>> results of SPEC benchmarks are quite encouraging.  Compilation is faster
>> by 2 to 5%.  On 64-bit, SPECfp is neutral in practice and SPECint gains
>> three points (losing much on perlbmk, but winning more on twolf).  On
>> 32-bit, things are worse, because SPECfp loses 6 points (winning only on
>> wupwise, in practice) and SPECint gains 1 (thanks again to twolf, and
>> gcc).

This is awesome!

>> Even though we are in stage 3, if the concept is fine I'd love if this
>> patch could go in as a preview, of course not enabling the new pass by
>> default.  In the 4.2 stage1 timeframe, I would like to move
>> simplifications from combine.c to simplify-rtx.c, and look at the
>> performance regressions (esp. mesa and crafty could be interesting) with
>> the goal of finally getting rid of CSE path following.

Given that the patch was submitted before Stage 3, I think it qualifies to
be committed and activated by default even now.

>> Ok for mainline?

One note: not all functions in fwprop.c have a comment, and some comments do
not explain all the parameters.

>> @@ -3256,17 +3256,16 @@ df_bb_regs_lives_compare (struct df *df,
>>     block as USE, is available at USE.  So DEF may as well be
>>     dead, in which case using it will extend its live range.  */
>>  bool
>> -df_local_def_available_p (struct df *df, struct ref *def, struct ref
*use)
>> +df_local_def_available_p (struct df *df, struct ref *def, rtx insn)

You forgot to update the documentation.


>> +/* Queue a substitution of SUBST into LOC in INSN.  */
>> +
>> +static void
>> +save_change (rtx *loc, rtx insn, rtx subst)

This comment is not clear to me at all. Can you please elaborate it a little
bit more?

>> +      pool = create_alloc_pool ("forward propagation",
>> +         sizeof (struct fwprop_change), df->n_uses);

Why not simply a VEC?

Thanks for woking on this!
-- 
Giovanni Bajo