[PATCH, rtl]: Do not generate insns with mismatched REG_EQUAL mode for multiword MULT RTXes.

[PATCH, rtl]: Do not generate insns with mismatched REG_EQUAL mode for multiword MULT RTXes.

[Uros Bizjak]

Hello!

Currently, multiword MULT RTX, synthesized via alg_shift algorithm,
can produce insn with REG_EQUAL note in the wrong mode:

;; D.2028_13 = acc_24 * 10;

(insn 51 50 52 920501-6.c:17 (set (reg:DI 107)
        (reg/v:DI 87 [ acc ])) -1 (nil))

(insn 52 51 53 920501-6.c:17 (set (reg:SI 109)
        (lshiftrt:SI (subreg:SI (reg:DI 107) 4)
            (const_int 31 [0x1f]))) -1 (nil))

(insn 53 52 54 920501-6.c:17 (set (subreg:SI (reg:DI 108) 0)
        (ashift:SI (subreg:SI (reg:DI 107) 0)
            (const_int 1 [0x1]))) -1 (nil))

(insn 54 53 55 920501-6.c:17 (set (subreg:SI (reg:DI 108) 0)
        (ior:SI (reg:SI 109)
            (subreg:SI (reg:DI 108) 0))) -1 (nil))

(insn 55 54 56 920501-6.c:17 (set (subreg:SI (reg:DI 108) 4)
        (ashift:SI (subreg:SI (reg:DI 107) 4)
            (const_int 1 [0x1]))) -1 (expr_list:REG_EQUAL (mult:DI
(reg/v:DI 87 [ acc ])
            (const_int 2 [0x2]))
        (nil)))

(...)

The last insn then triggers assert in loop-iv.c, iv_analyze_expr:

  gcc_assert (GET_MODE (rhs) == mode || GET_MODE (rhs) == VOIDmode);

where iv_analyze_expr is called from iv_analyze_def with the RTX from
attached REG_EQUAL note:

  if (!REG_P (reg))
    return false;

  set = single_set (insn);
  if (!set)
    return false;

  if (!REG_P (SET_DEST (set)))
    return false;

  gcc_assert (SET_DEST (set) == reg);
  rhs = find_reg_equal_equiv_note (insn);
  if (rhs)
    rhs = XEXP (rhs, 0);
  else
    rhs = SET_SRC (set);

  iv_analyze_expr (insn, rhs, GET_MODE (reg), iv);

To avoid mismatched modes, proposed patch introduces dummy move, where
REG_EQUAL note can be attached:

(insn 55 54 56 920501-6.c:17 (set (subreg:SI (reg:DI 108) 4)
        (ashift:SI (subreg:SI (reg:DI 107) 4)
            (const_int 1 [0x1]))) -1 (nil))

(insn 56 55 57 920501-6.c:17 (set (reg:DI 108)
        (reg:DI 108)) -1 (expr_list:REG_EQUAL (mult:DI (reg/v:DI 87 [ acc ])
            (const_int 2 [0x2]))
        (nil)))

This fixes ICE on my private (unreleased) target. I didn't find the
testcase that would trigger on FSF targets, but the solutions should
be obvious from the above analysis.

2010-08-04  Uros Bizjak  <ubizjak@gmail.com>

	* expmed.c (expand_mult_const) <case alg_shift>: Generate dummy
	move, so REG_EQUAL note will be attached to the correct insn.

Patch was tested on x86_64-pc-linux-gnu.

OK for mainline and all release branches?

Uros.

Index: expmed.c
===================================================================
--- expmed.c	(revision 162879)
+++ expmed.c	(working copy)
@@ -2907,6 +2907,8 @@ expand_mult_const (enum machine_mode mod
 	  accum = expand_shift (LSHIFT_EXPR, mode, accum,
 				build_int_cst (NULL_TREE, log),
 				NULL_RTX, 0);
+	  /* REG_EQUAL note will be attached to following insn.  */
+	  emit_move_insn (accum, accum);
 	  val_so_far <<= log;
 	  break;

Re: [PATCH, rtl]: Do not generate insns with mismatched REG_EQUAL mode for multiword MULT RTXes.

[Steven Bosscher]

On Wed, Aug 4, 2010 at 10:58 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> Hello!
>
> Currently, multiword MULT RTX, synthesized via alg_shift algorithm,
> can produce insn with REG_EQUAL note in the wrong mode:
>
> ;; D.2028_13 = acc_24 * 10;
>
> (insn 51 50 52 920501-6.c:17 (set (reg:DI 107)
>        (reg/v:DI 87 [ acc ])) -1 (nil))
>
> (insn 52 51 53 920501-6.c:17 (set (reg:SI 109)
>        (lshiftrt:SI (subreg:SI (reg:DI 107) 4)
>            (const_int 31 [0x1f]))) -1 (nil))
>
> (insn 53 52 54 920501-6.c:17 (set (subreg:SI (reg:DI 108) 0)
>        (ashift:SI (subreg:SI (reg:DI 107) 0)
>            (const_int 1 [0x1]))) -1 (nil))
>
> (insn 54 53 55 920501-6.c:17 (set (subreg:SI (reg:DI 108) 0)
>        (ior:SI (reg:SI 109)
>            (subreg:SI (reg:DI 108) 0))) -1 (nil))
>
> (insn 55 54 56 920501-6.c:17 (set (subreg:SI (reg:DI 108) 4)
>        (ashift:SI (subreg:SI (reg:DI 107) 4)
>            (const_int 1 [0x1]))) -1 (expr_list:REG_EQUAL (mult:DI
> (reg/v:DI 87 [ acc ])
>            (const_int 2 [0x2]))
>        (nil)))
>
> (...)
>
> The last insn then triggers assert in loop-iv.c, iv_analyze_expr:
>
>  gcc_assert (GET_MODE (rhs) == mode || GET_MODE (rhs) == VOIDmode);
>
> where iv_analyze_expr is called from iv_analyze_def with the RTX from
> attached REG_EQUAL note:
>
>  if (!REG_P (reg))
>    return false;
>
>  set = single_set (insn);
>  if (!set)
>    return false;
>
>  if (!REG_P (SET_DEST (set)))
>    return false;
>
>  gcc_assert (SET_DEST (set) == reg);
>  rhs = find_reg_equal_equiv_note (insn);
>  if (rhs)
>    rhs = XEXP (rhs, 0);
>  else
>    rhs = SET_SRC (set);
>
>  iv_analyze_expr (insn, rhs, GET_MODE (reg), iv);
>
> To avoid mismatched modes, proposed patch introduces dummy move, where
> REG_EQUAL note can be attached:
>
> (insn 55 54 56 920501-6.c:17 (set (subreg:SI (reg:DI 108) 4)
>        (ashift:SI (subreg:SI (reg:DI 107) 4)
>            (const_int 1 [0x1]))) -1 (nil))
>
> (insn 56 55 57 920501-6.c:17 (set (reg:DI 108)
>        (reg:DI 108)) -1 (expr_list:REG_EQUAL (mult:DI (reg/v:DI 87 [ acc ])
>            (const_int 2 [0x2]))
>        (nil)))
>
> This fixes ICE on my private (unreleased) target. I didn't find the
> testcase that would trigger on FSF targets, but the solutions should
> be obvious from the above analysis.

What happens with the REG_EQUAL note on the dummy move when the move
is deleted? I wouldn't be surprised if this REG_EQUAL note is lost
after the first DCE pass (which is probably a fast_dce run), because
the dummy move looks like a noop_move_p move and these are removed
unconditionally.

Ciao!
Steven

Re: [PATCH, rtl]: Do not generate insns with mismatched REG_EQUAL mode for multiword MULT RTXes.

[Uros Bizjak]

On Wed, Aug 4, 2010 at 11:22 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:

>> (insn 56 55 57 920501-6.c:17 (set (reg:DI 108)
>>        (reg:DI 108)) -1 (expr_list:REG_EQUAL (mult:DI (reg/v:DI 87 [ acc ])
>>            (const_int 2 [0x2]))
>>        (nil)))
>>
>> This fixes ICE on my private (unreleased) target. I didn't find the
>> testcase that would trigger on FSF targets, but the solutions should
>> be obvious from the above analysis.
>
> What happens with the REG_EQUAL note on the dummy move when the move
> is deleted? I wouldn't be surprised if this REG_EQUAL note is lost
> after the first DCE pass (which is probably a fast_dce run), because
> the dummy move looks like a noop_move_p move and these are removed
> unconditionally.

Hm, I don't know, but force_opreand in <case alg_add_factor> creates
the same dummy move at the end (so REG_NOTE attaches without problems
there):

(insn 61 60 62 920501-6.c:17 (clobber (reg:DI 112)) -1 (nil))

(insn 62 61 63 920501-6.c:17 (set (subreg:SI (reg:DI 112) 4)
        (plus:SI (subreg:SI (reg:DI 108) 4)
            (subreg:SI (reg:DI 110) 4))) -1 (nil))

(insn 63 62 64 920501-6.c:17 (set (reg:SI 113)
        (const_int 1 [0x1])) -1 (nil))

(jump_insn 64 63 65 920501-6.c:17 (set (pc)
        (if_then_else (ltu (subreg:SI (reg:DI 112) 4)
                (subreg:SI (reg:DI 108) 4))
            (label_ref 66)
            (pc))) -1 (nil))

(insn 65 64 66 920501-6.c:17 (set (reg:SI 113)
        (const_int 0 [0x0])) -1 (nil))

(code_label 66 65 67 3 "" [0 uses])

(insn 67 66 68 920501-6.c:17 (set (subreg:SI (reg:DI 112) 0)
        (plus:SI (subreg:SI (reg:DI 108) 0)
            (subreg:SI (reg:DI 110) 0))) -1 (nil))

(insn 68 67 69 920501-6.c:17 (set (reg:SI 114)
        (plus:SI (reg:SI 113)
            (subreg:SI (reg:DI 112) 0))) -1 (nil))

(insn 69 68 70 920501-6.c:17 (set (subreg:SI (reg:DI 112) 0)
        (reg:SI 114)) -1 (nil))

(insn 70 69 71 920501-6.c:17 (set (reg:DI 112)
        (reg:DI 112)) -1 (expr_list:REG_EQUAL (mult:DI (reg/v:DI 87 [ acc ])
            (const_int 10 [0xa]))
        (nil)))

Uros.

Re: [PATCH, rtl]: Do not generate insns with mismatched REG_EQUAL mode for multiword MULT RTXes.

[Steven Bosscher]

On Wed, Aug 4, 2010 at 11:49 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Wed, Aug 4, 2010 at 11:22 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
>
>>> (insn 56 55 57 920501-6.c:17 (set (reg:DI 108)
>>>        (reg:DI 108)) -1 (expr_list:REG_EQUAL (mult:DI (reg/v:DI 87 [ acc ])
>>>            (const_int 2 [0x2]))
>>>        (nil)))
>>>
>>> This fixes ICE on my private (unreleased) target. I didn't find the
>>> testcase that would trigger on FSF targets, but the solutions should
>>> be obvious from the above analysis.
>>
>> What happens with the REG_EQUAL note on the dummy move when the move
>> is deleted? I wouldn't be surprised if this REG_EQUAL note is lost
>> after the first DCE pass (which is probably a fast_dce run), because
>> the dummy move looks like a noop_move_p move and these are removed
>> unconditionally.
>
> Hm, I don't know, but force_opreand in <case alg_add_factor> creates
> the same dummy move at the end (so REG_NOTE attaches without problems
> there):

I'm not saying attaching the REG_NOTE causes problems, but I am quite
sure that note also gets lost when the dummy move is removed.

It looks like the first pass that may remove noop_move_p insns is the
first rtl_dce pass. Is your REG_NOTE still there if you look at the
.dce1 rtl dump?

Ciao!
Steven

Re: [PATCH, rtl]: Do not generate insns with mismatched REG_EQUAL mode for multiword MULT RTXes.

[Uros Bizjak]

On Thu, Aug 5, 2010 at 12:00 AM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:

>>>> (insn 56 55 57 920501-6.c:17 (set (reg:DI 108)
>>>>        (reg:DI 108)) -1 (expr_list:REG_EQUAL (mult:DI (reg/v:DI 87 [ acc ])
>>>>            (const_int 2 [0x2]))
>>>>        (nil)))
>>>>
>>>> This fixes ICE on my private (unreleased) target. I didn't find the
>>>> testcase that would trigger on FSF targets, but the solutions should
>>>> be obvious from the above analysis.
>>>
>>> What happens with the REG_EQUAL note on the dummy move when the move
>>> is deleted? I wouldn't be surprised if this REG_EQUAL note is lost
>>> after the first DCE pass (which is probably a fast_dce run), because
>>> the dummy move looks like a noop_move_p move and these are removed
>>> unconditionally.
>>
>> Hm, I don't know, but force_opreand in <case alg_add_factor> creates
>> the same dummy move at the end (so REG_NOTE attaches without problems
>> there):
>
> I'm not saying attaching the REG_NOTE causes problems, but I am quite
> sure that note also gets lost when the dummy move is removed.
>
> It looks like the first pass that may remove noop_move_p insns is the
> first rtl_dce pass. Is your REG_NOTE still there if you look at the
> .dce1 rtl dump?

Ugh, you are right. REG_EQUAL notes on noop moves are removed just
after vreg pass (I'm testing this on gcc-4.5), so it looks to me that
the whole business with REG_EQUAL notes in expmed.c/expand_mult_const
() is flawed...

Perhaps we should generate a new insn that would survive optimization passes?

Uros.

Re: [PATCH, rtl]: Do not generate insns with mismatched REG_EQUAL mode for multiword MULT RTXes.

[Steven Bosscher]

On Thu, Aug 5, 2010 at 12:20 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> Perhaps we should generate a new insn that would survive optimization passes?

I think you should just take a new register, instead of generating a
no-op move. This probably helps for other things as well, e.g. for
passes that look at DF_REG_DEF_COUNT (reg_scan, alias, ...). OTOH,
maybe that only "helps" up to CPROP pass, if the register is
copy-propagated and the set is made useless.  Dunno...

Ciao!
Steven

Re: [PATCH, rtl]: Do not generate insns with mismatched REG_EQUAL mode for multiword MULT RTXes.

[Uros Bizjak]

On Thu, Aug 5, 2010 at 1:09 AM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> On Thu, Aug 5, 2010 at 12:20 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> Perhaps we should generate a new insn that would survive optimization passes?
>
> I think you should just take a new register, instead of generating a
> no-op move. This probably helps for other things as well, e.g. for
> passes that look at DF_REG_DEF_COUNT (reg_scan, alias, ...). OTOH,
> maybe that only "helps" up to CPROP pass, if the register is
> copy-propagated and the set is made useless.  Dunno...

Thanks for your comments! Following your suggestion, I propose
attached patch, where we expand shift into temporary and copy this
temporary to final target (the same way other algorithms do via
force_operand). The note now attaches to non-noop insn:

;; D.2028_13 = acc_24 * 10;

(insn 51 50 52 920501-6.c:17 (set (reg:DI 107)
        (reg/v:DI 87 [ acc ])) -1 (nil))

(insn 52 51 53 920501-6.c:17 (set (reg:SI 109)
        (lshiftrt:SI (subreg:SI (reg:DI 107) 4)
            (const_int 31 [0x1f]))) -1 (nil))

(insn 53 52 54 920501-6.c:17 (set (subreg:SI (reg:DI 108) 0)
        (ashift:SI (subreg:SI (reg:DI 107) 0)
            (const_int 1 [0x1]))) -1 (nil))

(insn 54 53 55 920501-6.c:17 (set (subreg:SI (reg:DI 108) 0)
        (ior:SI (reg:SI 109)
            (subreg:SI (reg:DI 108) 0))) -1 (nil))

(insn 55 54 56 920501-6.c:17 (set (subreg:SI (reg:DI 108) 4)
        (ashift:SI (subreg:SI (reg:DI 107) 4)
            (const_int 1 [0x1]))) -1 (nil))

(insn 56 55 57 920501-6.c:17 (set (reg:DI 107)
        (reg:DI 108)) -1 (expr_list:REG_EQUAL (mult:DI (reg/v:DI 87 [ acc ])
            (const_int 2 [0x2]))
        (nil)))

When this stream is processed by subreg1 pass, the register is split
into SImode regs, and REG_EQUAL note is *correctly* removed.

OTOH, without the patch, REG_EQUAL survives through subreg1 pass,
leaving following semi-invalid RTX:

(insn 30 29 31 4 920501-6.c:17 (set (reg:SI 225 [+4 ])
        (ashift:SI (reg:SI 223 [+4 ])
            (const_int 1 [0x1]))) 41 {ashlsi3} (expr_list:REG_EQUAL
(mult:DI (reg/v:DI 41 [ s ])
            (const_int 2 [0x2]))
        (nil)))

Also, looking through the dumps, I saw:

920501-6.c.152r.fwprop1: Setting REG_EQUAL note
920501-6.c.152r.fwprop1: Setting REG_EQUAL note
920501-6.c.152r.fwprop1: Setting REG_EQUAL note

So, it looks to me, that somebody actually thought on copying
REG_EQUALs when values are propagated...

2010-08-04  Uros Bizjak  <ubizjak@gmail.com>

	* expmed.c (expand_mult_const) <case alg_shift:>: Expand shift into
	temporary. Emit move from temporary to accum, so REG_EQUAL note will
	be attached to this insn in correct mode.

This patch fixes original ivopts failure as well.

Patch is currently regression testing on x86_64-pc-linux-gnu. OK for
mainline and release branches, if it passes testing?

Uros.

Index: expmed.c
===================================================================
--- expmed.c	(revision 162856)
+++ expmed.c	(working copy)
@@ -3006,9 +3006,10 @@
       switch (alg->op[opno])
 	{
 	case alg_shift:
-	  accum = expand_shift (LSHIFT_EXPR, mode, accum,
-				build_int_cst (NULL_TREE, log),
-				NULL_RTX, 0);
+	  tem = expand_shift (LSHIFT_EXPR, mode, accum,
+			      build_int_cst (NULL_TREE, log),
+			      NULL_RTX, 0);
+	  emit_move_insn (accum, tem);
 	  val_so_far <<= log;
 	  break;

Re: [PATCH, rtl]: Do not generate insns with mismatched REG_EQUAL mode for multiword MULT RTXes.

[Uros Bizjak]

On Thu, 2010-08-05 at 10:46 +0200, Uros Bizjak wrote:

> 2010-08-04  Uros Bizjak  <ubizjak@gmail.com>
> 
> 	* expmed.c (expand_mult_const) <case alg_shift:>: Expand shift into
> 	temporary. Emit move from temporary to accum, so REG_EQUAL note will
> 	be attached to this insn in correct mode.
> 
> This patch fixes original ivopts failure as well.
> 
> Patch is currently regression testing on x86_64-pc-linux-gnu. OK for
> mainline and release branches, if it passes testing?
> 

Regression tests passed OK.

Also, I have added a comment to the patch, see attached.

Uros.

Index: expmed.c
===================================================================
--- expmed.c	(revision 162899)
+++ expmed.c	(working copy)
@@ -2904,9 +2904,11 @@ expand_mult_const (enum machine_mode mod
       switch (alg->op[opno])
 	{
 	case alg_shift:
-	  accum = expand_shift (LSHIFT_EXPR, mode, accum,
-				build_int_cst (NULL_TREE, log),
-				NULL_RTX, 0);
+	  tem = expand_shift (LSHIFT_EXPR, mode, accum,
+			      build_int_cst (NULL_TREE, log),
+			      NULL_RTX, 0);
+	  /* REG_EQUAL note will be attached to the following insn.  */
+	  emit_move_insn (accum, tem);
 	  val_so_far <<= log;
 	  break;
 

Re: [PATCH, rtl]: Do not generate insns with mismatched REG_EQUAL mode for multiword MULT RTXes.

[Bernd Schmidt]

On 08/05/2010 01:17 PM, Uros Bizjak wrote:
> Index: expmed.c
> ===================================================================
> --- expmed.c	(revision 162899)
> +++ expmed.c	(working copy)
> @@ -2904,9 +2904,11 @@ expand_mult_const (enum machine_mode mod
>        switch (alg->op[opno])
>  	{
>  	case alg_shift:
> -	  accum = expand_shift (LSHIFT_EXPR, mode, accum,
> -				build_int_cst (NULL_TREE, log),
> -				NULL_RTX, 0);
> +	  tem = expand_shift (LSHIFT_EXPR, mode, accum,
> +			      build_int_cst (NULL_TREE, log),
> +			      NULL_RTX, 0);
> +	  /* REG_EQUAL note will be attached to the following insn.  */
> +	  emit_move_insn (accum, tem);
>  	  val_so_far <<= log;
>  	  break;

Ok.


Bernd