diff mbox

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

Message ID AANLkTi=jwL+eKfs7P2XO2XmyqU8iicsJjsxB4y4aq3+M@mail.gmail.com
State New
Headers show

Commit Message

Uros Bizjak Aug. 4, 2010, 8:58 p.m. UTC 
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.

Comments

Steven Bosscher Aug. 4, 2010, 9:22 p.m. UTC | #1 
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
Uros Bizjak Aug. 4, 2010, 9:49 p.m. UTC | #2 
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.
Steven Bosscher Aug. 4, 2010, 10 p.m. UTC | #3 
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
Uros Bizjak Aug. 4, 2010, 10:20 p.m. UTC | #4 
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.
Steven Bosscher Aug. 4, 2010, 11:09 p.m. UTC | #5 
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
diff mbox

Patch

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;