Small REE improvement (PR target/82682)

Message ID 20180111183248.GU1833@tucnak
State New
Headers show
Series
  • Small REE improvement (PR target/82682)
Related show

Commit Message

Jakub Jelinek Jan. 11, 2018, 6:32 p.m.
Hi!

As mentioned in this PR, we have effectively:
            (set (reg2) (expression))
            ...
            (set (reg1) (reg2))
            ...
            (set (reg2) (any_extend (reg1)))
in pr50083.c with -O2 -m32, where the expression in this case is actually
a QImode memory read has any_extend is zero extension to SImode.
Currently REE tries to change that to just
	    (set (reg2) (any_extend (reg2)))
	    ...
	    (set (reg1) (reg2))
which for one isn't useful, and fails because reg2 is %si which doesn't have
a QImode subreg in 32-bit code.  This patch instead sees the copying of
the reg2 to reg1 as def_insn and in that case looks further for the defining
insn of that (def_insn2) and transforms it thus to:
	    (set (reg2) (any_extend (expression))
	    ...
	    (set (reg1) (reg2)) // This insn is untouched, stays in the
				// mode it used before, but can be DCEd
				// later if the reg isn't really used

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

No testcase included, as it fixes an existing one:
-FAIL: gcc.target/i386/pr50038.c scan-assembler-times movzbl 2 (found 3 times)

2018-01-11  Jakub Jelinek  <jakub@redhat.com>

	PR target/82682
	* ree.c (combine_reaching_defs): Optimize also
	reg2=exp; reg1=reg2; reg2=any_extend(reg1); into
	reg2=any_extend(exp); reg1=reg2;, formatting fix.


	Jakub

Comments

Jeff Law Jan. 11, 2018, 7:14 p.m. | #1
On 01/11/2018 11:32 AM, Jakub Jelinek wrote:
> Hi!
> 
> As mentioned in this PR, we have effectively:
>             (set (reg2) (expression))
>             ...
>             (set (reg1) (reg2))
>             ...
>             (set (reg2) (any_extend (reg1)))
> in pr50083.c with -O2 -m32, where the expression in this case is actually
> a QImode memory read has any_extend is zero extension to SImode.
> Currently REE tries to change that to just
> 	    (set (reg2) (any_extend (reg2)))
> 	    ...
> 	    (set (reg1) (reg2))
> which for one isn't useful, and fails because reg2 is %si which doesn't have
> a QImode subreg in 32-bit code. 
Right.  And in fact, the whole reason why we get the code we do is
because reg2 can't be used in QImode.


 This patch instead sees the copying of
> the reg2 to reg1 as def_insn and in that case looks further for the defining
> insn of that (def_insn2) and transforms it thus to:
> 	    (set (reg2) (any_extend (expression))
> 	    ...
> 	    (set (reg1) (reg2)) // This insn is untouched, stays in the
> 				// mode it used before, but can be DCEd
> 				// later if the reg isn't really used
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Right.


> 
> No testcase included, as it fixes an existing one:
> -FAIL: gcc.target/i386/pr50038.c scan-assembler-times movzbl 2 (found 3 times)
> 
> 2018-01-11  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR target/82682
> 	* ree.c (combine_reaching_defs): Optimize also
> 	reg2=exp; reg1=reg2; reg2=any_extend(reg1); into
> 	reg2=any_extend(exp); reg1=reg2;, formatting fix.
It seems reasonable.  Your patch might even allow ree.c to pick up
secondary effects.  Totally agree with the decision to leave  the copy
in the IL, we already do the same for other cases handled by ree.c
OK.
jeff

Patch

--- gcc/ree.c.jj	2018-01-04 00:43:17.635703232 +0100
+++ gcc/ree.c	2018-01-11 11:37:10.312822260 +0100
@@ -901,7 +901,7 @@  combine_reaching_defs (ext_cand *cand, c
                                  REGNO (SET_DEST (pat)));
       emit_move_insn (new_dst, new_src);
 
-      rtx_insn *insn = get_insns();
+      rtx_insn *insn = get_insns ();
       end_sequence ();
       if (NEXT_INSN (insn))
 	return false;
@@ -910,8 +910,81 @@  combine_reaching_defs (ext_cand *cand, c
       extract_insn (insn);
       if (!constrain_operands (1, get_preferred_alternatives (insn, bb)))
 	return false;
-    }
 
+      while (REG_P (SET_SRC (*dest_sub_rtx))
+	     && (REGNO (SET_SRC (*dest_sub_rtx)) == REGNO (SET_DEST (pat))))
+	{
+	  /* Considering transformation of
+	     (set (reg2) (expression))
+	     ...
+	     (set (reg1) (reg2))
+	     ...
+	     (set (reg2) (any_extend (reg1)))
+
+	     into
+
+	     (set (reg2) (any_extend (expression)))
+	     (set (reg1) (reg2))
+	     ...  */
+	  struct df_link *defs
+	    = get_defs (def_insn, SET_SRC (*dest_sub_rtx), NULL);
+	  if (defs == NULL || defs->next)
+	    break;
+
+	  /* There is only one reaching def.  */
+	  rtx_insn *def_insn2 = DF_REF_INSN (defs->ref);
+
+	  /* The defining statement must not have been modified either.  */
+	  if (state->modified[INSN_UID (def_insn2)].kind != EXT_MODIFIED_NONE)
+	    break;
+
+	  /* The def_insn2 and candidate insn must be in the same
+	     block and def_insn follows def_insn2.  */
+	  if (bb != BLOCK_FOR_INSN (def_insn2)
+	      || DF_INSN_LUID (def_insn2) > DF_INSN_LUID (def_insn))
+	    break;
+
+	  rtx *dest_sub_rtx2 = get_sub_rtx (def_insn2);
+	  if (dest_sub_rtx2 == NULL
+	      || !REG_P (SET_DEST (*dest_sub_rtx2)))
+	    break;
+
+	  /* On RISC machines we must make sure that changing the mode of
+	     SRC_REG as destination register will not affect its reaching
+	     uses, which may read its value in a larger mode because DEF_INSN
+	     implicitly sets it in word mode.  */
+	  if (WORD_REGISTER_OPERATIONS && known_lt (prec, BITS_PER_WORD))
+	    {
+	      struct df_link *uses = get_uses (def_insn2, SET_DEST (pat));
+	      if (!uses)
+		break;
+
+	      df_link *use;
+	      rtx dest2 = SET_DEST (*dest_sub_rtx2);
+	      for (use = uses; use; use = use->next)
+		if (paradoxical_subreg_p (GET_MODE (*DF_REF_LOC (use->ref)),
+					  GET_MODE (dest2)))
+		  break;
+	      if (use)
+		break;
+	    }
+
+	  /* The destination register of the extension insn must not be
+	     used or set between the def_insn2 and def_insn exclusive.
+	     Likewise for the other reg, i.e. check both reg1 and reg2
+	     in the above comment.  */
+	  if (reg_used_between_p (SET_DEST (PATTERN (cand->insn)),
+				  def_insn2, def_insn)
+	      || reg_set_between_p (SET_DEST (PATTERN (cand->insn)),
+				    def_insn2, def_insn)
+	      || reg_used_between_p (src_reg, def_insn2, def_insn)
+	      || reg_set_between_p (src_reg, def_insn2, def_insn))
+	    break;
+
+	  state->defs_list[0] = def_insn2;
+	  break;
+	}
+    }
 
   /* If cand->insn has been already modified, update cand->mode to a wider
      mode if possible, or punt.  */