[PATCH, loop2_invariant] Pre-check invariants

[PATCH, loop2_invariant] Pre-check invariants

[Zhenqiang Chen]

Hi,

During tests, I found some invariants could not be replaced at the
last stage. If we can identify such invariants earlier, we can skip
them and give the chance to other invariants.  So the patch pre-checks
candidates to skip the one which can not make a valid insn during
replacement in move_invariant_reg.

Bootstrap and no make check regression on X86-64.
Bootstrap and no make check regression on X86-64 with
flag_ira_loop_pressure = true.

OK for trunk?

Thanks!
-Zhenqiang

ChangeLog:
2014-06-10  Zhenqiang Chen  <zhenqiang.chen@linaro.org>

        * loop-invariant.c (find_invariant_insn): Skip invariants, which
        can not make a valid insn during replacement in move_invariant_reg.

diff --git a/gcc/loop-invariant.c b/gcc/loop-invariant.c
index c43206a..7be4b29 100644
--- a/gcc/loop-invariant.c
+++ b/gcc/loop-invariant.c
@@ -881,6 +881,35 @@ find_invariant_insn (rtx insn, bool
always_reached, bool always_executed)
       || HARD_REGISTER_P (dest))
     simple = false;

+  /* Pre-check candidate to skip the one which can not make a valid insn
+     during move_invariant_reg.  */
+  if (flag_ira_loop_pressure && df_live && simple
+      && REG_P (dest) && DF_REG_DEF_COUNT (REGNO (dest)) > 1)
+    {
+      df_ref use;
+      rtx ref;
+      unsigned int i = REGNO (dest);
+      struct df_insn_info *insn_info;
+      df_ref *def_rec;
+
+      for (use = DF_REG_USE_CHAIN (i); use; use = DF_REF_NEXT_REG (use))
+       {
+         ref = DF_REF_INSN (use);
+         insn_info = DF_INSN_INFO_GET (ref);
+
+         for (def_rec = DF_INSN_INFO_DEFS (insn_info); *def_rec; def_rec++)
+           if (DF_REF_REGNO (*def_rec) == i)
+             {
+               /* Multi definitions at this stage, most likely are due to
+                  instruction constrain, which requires both read and write
+                  on the same register.  Since move_invariant_reg is not
+                  powerful enough to handle such cases, just ignore the INV
+                  and leave the chance to others.  */
+               return;
+             }
+       }
+    }
+
   if (!may_assign_reg_p (SET_DEST (set))
       || !check_maybe_invariant (SET_SRC (set)))
     return;

Re: [PATCH, loop2_invariant] Pre-check invariants

[Steven Bosscher]

On Tue, Jun 10, 2014 at 11:55 AM, Zhenqiang Chen wrote:
>
>         * loop-invariant.c (find_invariant_insn): Skip invariants, which
>         can not make a valid insn during replacement in move_invariant_reg.
>
> --- a/gcc/loop-invariant.c
> +++ b/gcc/loop-invariant.c
> @@ -881,6 +881,35 @@ find_invariant_insn (rtx insn, bool
> always_reached, bool always_executed)
>        || HARD_REGISTER_P (dest))
>      simple = false;
>
> +  /* Pre-check candidate to skip the one which can not make a valid insn
> +     during move_invariant_reg.  */
> +  if (flag_ira_loop_pressure && df_live && simple
> +      && REG_P (dest) && DF_REG_DEF_COUNT (REGNO (dest)) > 1)

Why only do this with (flag_ira_loop_pressure && df_live)? If the
invariant can't be moved, we should ignore it regardless of whether
register pressure is taken into account.


> +    {
> +      df_ref use;
> +      rtx ref;
> +      unsigned int i = REGNO (dest);
> +      struct df_insn_info *insn_info;
> +      df_ref *def_rec;
> +
> +      for (use = DF_REG_USE_CHAIN (i); use; use = DF_REF_NEXT_REG (use))
> +       {
> +         ref = DF_REF_INSN (use);
> +         insn_info = DF_INSN_INFO_GET (ref);
> +
> +         for (def_rec = DF_INSN_INFO_DEFS (insn_info); *def_rec; def_rec++)
> +           if (DF_REF_REGNO (*def_rec) == i)
> +             {
> +               /* Multi definitions at this stage, most likely are due to
> +                  instruction constrain, which requires both read and write
> +                  on the same register.  Since move_invariant_reg is not
> +                  powerful enough to handle such cases, just ignore the INV
> +                  and leave the chance to others.  */
> +               return;
> +             }
> +       }
> +    }
> +
>    if (!may_assign_reg_p (SET_DEST (set))
>        || !check_maybe_invariant (SET_SRC (set)))
>      return;


Can you put your new check between "may_assign_reg_p (dest)" and
"check_maybe_invariant"? The may_assign_reg_p check is cheap and
triggers quite often.

Looks good to me otherwise.

Ciao!
Steven

Re: [PATCH, loop2_invariant] Pre-check invariants

[Zhenqiang Chen]

On 10 June 2014 19:01, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> On Tue, Jun 10, 2014 at 11:55 AM, Zhenqiang Chen wrote:
>>
>>         * loop-invariant.c (find_invariant_insn): Skip invariants, which
>>         can not make a valid insn during replacement in move_invariant_reg.
>>
>> --- a/gcc/loop-invariant.c
>> +++ b/gcc/loop-invariant.c
>> @@ -881,6 +881,35 @@ find_invariant_insn (rtx insn, bool
>> always_reached, bool always_executed)
>>        || HARD_REGISTER_P (dest))
>>      simple = false;
>>
>> +  /* Pre-check candidate to skip the one which can not make a valid insn
>> +     during move_invariant_reg.  */
>> +  if (flag_ira_loop_pressure && df_live && simple
>> +      && REG_P (dest) && DF_REG_DEF_COUNT (REGNO (dest)) > 1)
>
> Why only do this with (flag_ira_loop_pressure && df_live)? If the
> invariant can't be moved, we should ignore it regardless of whether
> register pressure is taken into account.

Thanks for the comments. df_live seams redundant.

With flag_ira_loop_pressure, the pass will call df_analyze () at the
beginning, which can make sure all the DF info are correct.

Can we guarantee all DF_... correct without df_analyze ()?

>> +    {
>> +      df_ref use;
>> +      rtx ref;
>> +      unsigned int i = REGNO (dest);
>> +      struct df_insn_info *insn_info;
>> +      df_ref *def_rec;
>> +
>> +      for (use = DF_REG_USE_CHAIN (i); use; use = DF_REF_NEXT_REG (use))
>> +       {
>> +         ref = DF_REF_INSN (use);
>> +         insn_info = DF_INSN_INFO_GET (ref);
>> +
>> +         for (def_rec = DF_INSN_INFO_DEFS (insn_info); *def_rec; def_rec++)
>> +           if (DF_REF_REGNO (*def_rec) == i)
>> +             {
>> +               /* Multi definitions at this stage, most likely are due to
>> +                  instruction constrain, which requires both read and write
>> +                  on the same register.  Since move_invariant_reg is not
>> +                  powerful enough to handle such cases, just ignore the INV
>> +                  and leave the chance to others.  */
>> +               return;
>> +             }
>> +       }
>> +    }
>> +
>>    if (!may_assign_reg_p (SET_DEST (set))
>>        || !check_maybe_invariant (SET_SRC (set)))
>>      return;
>
>
> Can you put your new check between "may_assign_reg_p (dest)" and
> "check_maybe_invariant"? The may_assign_reg_p check is cheap and
> triggers quite often.

Updated and also removed the "flag_ira_loop_pressure && df_live". To
make it easy, I move the codes to a new function.

OK for trunk?

diff --git a/gcc/loop-invariant.c b/gcc/loop-invariant.c
index c6bf19b..d19f3c8 100644
--- a/gcc/loop-invariant.c
+++ b/gcc/loop-invariant.c
@@ -839,6 +852,39 @@ check_dependencies (rtx insn, bitmap depends_on)
   return true;
 }

+/* Pre-check candidate DEST to skip the one which can not make a valid insn
+   during move_invariant_reg.  SIMPlE is to skip HARD_REGISTER.  */
+static bool
+pre_check_invariant_p (bool simple, rtx dest)
+{
+  if (simple && REG_P (dest) && DF_REG_DEF_COUNT (REGNO (dest)) > 1)
+    {
+      df_ref use;
+      rtx ref;
+      unsigned int i = REGNO (dest);
+      struct df_insn_info *insn_info;
+      df_ref *def_rec;
+
+      for (use = DF_REG_USE_CHAIN (i); use; use = DF_REF_NEXT_REG (use))
+       {
+         ref = DF_REF_INSN (use);
+         insn_info = DF_INSN_INFO_GET (ref);
+
+         for (def_rec = DF_INSN_INFO_DEFS (insn_info); *def_rec; def_rec++)
+           if (DF_REF_REGNO (*def_rec) == i)
+             {
+               /* Multi definitions at this stage, most likely are due to
+                  instruction constrain, which requires both read and write
+                  on the same register.  Since move_invariant_reg is not
+                  powerful enough to handle such cases, just ignore the INV
+                  and leave the chance to others.  */
+               return false;
+             }
+       }
+    }
+  return true;
+}
+
 /* Finds invariant in INSN.  ALWAYS_REACHED is true if the insn is always
    executed.  ALWAYS_EXECUTED is true if the insn is always executed,
    unless the program ends due to a function call.  */
@@ -868,7 +914,8 @@ find_invariant_insn (rtx insn, bool
always_reached, bool always_executed)
       || HARD_REGISTER_P (dest))
     simple = false;

-  if (!may_assign_reg_p (SET_DEST (set))
+  if (!may_assign_reg_p (dest)
+      || !pre_check_invariant_p (simple, dest)
       || !check_maybe_invariant (SET_SRC (set)))
     return;

Re: [PATCH, loop2_invariant] Pre-check invariants

[Jeff Law]

On 06/11/14 03:35, Zhenqiang Chen wrote:
>
> Thanks for the comments. df_live seams redundant.
>
> With flag_ira_loop_pressure, the pass will call df_analyze () at the
> beginning, which can make sure all the DF info are correct.
>
> Can we guarantee all DF_... correct without df_analyze ()?
They should be fine in this context.


> +/* Pre-check candidate DEST to skip the one which can not make a valid insn
> +   during move_invariant_reg.  SIMPlE is to skip HARD_REGISTER.  */
s/SIMPlE/SIMPLE/


> +             {
> +               /* Multi definitions at this stage, most likely are due to
> +                  instruction constrain, which requires both read and write
s/constrain/constraints/

Though that doesn't make sense.  Constraints don't come into play until 
much later in the pipeline.   Certainly there's been code in the 
expanders and elsewhere to try and make the code we generate more 
acceptable to 2-address targets and that's probably what you're really 
running into.   I think the code is fine, but that you need to improve 
the comment.

ISTM that if your primary focus is to filter out read/write operands, 
then just say that and ignore the constraints or other mechanisms by 
which we got a read/write pseudo.

So I think with those two small comment changes, this patch is OK for 
the trunk.  Please post the final version for archival purposes before 
checking it in.

Thanks,
Jeff