Segher Boessenkool <seg...@kernel.crashing.org> writes:
> Hi Jiufu,
>
> Just reviewing random things as I see them...
>
> On Wed, Apr 15, 2020 at 09:56:00AM +0800, Jiufu Guo wrote:
>> This patch only supports simple loops: one exit edge with one major basic
>> block.
>
> That is fine for a proof-of-concept, but will need fixing perhaps.
Hi Segher,
Thanks so much. You are always point out things to improve!
>
>> --- a/gcc/common.opt
>> +++ b/gcc/common.opt
>> @@ -2526,6 +2526,10 @@ fvariable-expansion-in-unroller
>> Common Report Var(flag_variable_expansion_in_unroller) Optimization
>> Apply variable expansion when loops are unrolled.
>>
>> +fassin-new-operand-in-unroller
>
> Typo ("assign"). But, is there ever any reason to disable this? (You
> want an option for it during development, of course, but all public
> options cost something).
Helpful question, development is one reason for now to test benefits and
lost, and incase some one want to disable it.
Your question let me think if it really need to disable it. In theory,
this behavior is correct and not harmful.
>
>> +struct def_to_split
>> +{
>> + rtx_insn *insn; /* The insn in that the assigment occurs. */
>> + rtx reg; /* The def/set pseudo. */
>> + vec<rtx> defs; /* The copies of the defs which is split to. */
>> + struct def_to_split *next; /* Next entry in walking order. */
>> + int count; /* Number of DEFS. */
>> + int use_before_def; /* The pseudo is used before re-defined. */
>> + rtx_insn *last_insn;
>> +};
>
> Three different kinds of indentation here.
>
> It might work better to explain it all in a bigger comment before this
> struct definition. It also would help to have better, more obvious
> names? Not "reg" but "orig_reg", not "defs" but "regs", not "count"
> but "nregs"?
Style, I may enhance it through tools.
Thanks for comments about naming :)
>
>> +/* Return a hash for VES. */
>> +
>> +inline hashval_t
>> +def_split_hasher::hash (const def_to_split *i)
>> +{
>> + return (hashval_t) INSN_UID (i->insn);
>> +}
>
> What does "VES" mean? Something from an older version of the patch?
it is typo :(
>
>
> Segher
