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.
> --- 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).
> +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"?
> +/* 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?
Segher
