RE: [PATCH] Fix MIPS-specific ICE in gcc.dg/pr77834.c (PR rtl-optimization/79150).
Hi Catherine,
What is your opinion on this patch? I am OK with the temporary
workaround on the basis that the additional nop is successfully
eliminated so there is no code size increase. Also, I am happy
enough that the CFG is fixed very soon after the block move is
expanded so the 'bad' basic block is fixed almost immediately
anyway making the offending check potentially unnecessary in
the first place.
Thanks,
Matthew
> -Original Message-
> From: Toma Tabacu
> Sent: 07 March 2017 11:44
> To: gcc-patches@gcc.gnu.org
> Cc: Matthew Fortune; Segher Boessenkool (seg...@kernel.crashing.org)
> Subject: [PATCH] Fix MIPS-specific ICE in gcc.dg/pr77834.c (PR rtl-
> optimization/79150).
>
> Hi,
>
> This ICE is caused by "gcc_assert (!JUMP_P (last))" in
> commit_one_edge_insertion (gcc/cfgrtl.c):
>
> if (returnjump_p (last))
> {
> /* ??? Remove all outgoing edges from BB and add one for EXIT.
>This is not currently a problem because this only happens
>for the (single) epilogue, which already has a fallthru edge
>to EXIT. */
>
> e = single_succ_edge (bb);
> gcc_assert (e->dest == EXIT_BLOCK_PTR_FOR_FN (cfun)
> && single_succ_p (bb) && (e->flags & EDGE_FALLTHRU));
>
> e->flags &= ~EDGE_FALLTHRU;
> emit_barrier_after (last);
>
> if (before)
> delete_insn (before);
> }
> else
> gcc_assert (!JUMP_P (last));
>
> The assert is triggered because we're removing an edge which ends on a
> conditional jump, which is part of a loop.
>
> The reason for the existence of this loop is the size of the vector used
> in gcc.dg/pr77834.c.
> When compiling code which uses vectors for MIPS, a looping memcpy is
> generated if the vectors are big enough (>32 bytes for MIPS32 or >64
> bytes for MIPS64).
> This takes place in mips_expand_block_move (gcc/config/mips.c).
>
> In our case, a looping memcpy gets generated for a partition copy which
> is inserted on an edge (in insert_partition_copy_on_edge, gcc/tree-
> outof-ssa.c).
> This happens during PHI node elimination, which is done by eliminate_phi
> (gcc/tree-outof-ssa.c), as a part of expand_phi_nodes, which is called
> by the expand pass (gcc/cfgexpand.c).
>
> My original fix was to update the CFG by calling
> find_many_sub_basic_blocks with an all-one block bitmap (which also
> happens in cfgexpand.c, after edge
> removal) whenever an edge contains an insn which satisfies
> control_flow_insn_p.
> However, Segher Boessenkool said that this would be too risky for stage
> 4 and suggested inserting a NOP after the conditional jump in order to
> fool the assert. This assumes that it is safe to delay the CFG update
> for this particular case.
>
> This patch changes mips_block_move_loop to emit a NOP after the
> conditional jump, if the jump is the last insn of the loop. This
> prevents "gcc_assert (!JUMP_P (last))" from triggering.
>
> The NOP will never make it into the final assembly code because it is
> removed during the cse1 pass through DCE for -O1, -O2, and -O3, and it's
> not even emitted for -O0 and -Os because looping memcpy's are not
> generated for those optimization levels, as can be seen in
> mips_expand_block_move from mips.c:
>
> if (INTVAL (length) <= MIPS_MAX_MOVE_BYTES_STRAIGHT)
> {
> mips_block_move_straight (dest, src, INTVAL (length));
> return true;
> }
> else if (optimize)
> {
> mips_block_move_loop (dest, src, INTVAL (length),
> MIPS_MAX_MOVE_BYTES_PER_LOOP_ITER);
> return true;
> }
>
> Tested with mips-mti-elf.
>
> Regards,
> Toma Tabacu
>
> gcc/
>
> * config/mips/mips.c (mips_block_move_loop): Emit a NOP after the
> conditional jump, if the jump is the last insn of the loop.
>
> diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c index
> 4e13fbe..43e719f 100644
> --- a/gcc/config/mips/mips.c
> +++ b/gcc/config/mips/mips.c
> @@ -8098,6 +8098,9 @@ mips_block_move_loop (rtx dest, rtx src,
> HOST_WIDE_INT length,
>/* Mop up any left-over bytes. */
>if (leftover)
> mips_block_move_straight (dest, src, leftover);
> + else
> +/* Temporary fix for PR79150. */
> +emit_insn (gen_nop ());
> }
>
> /* Expand a movmemsi instruction, which copies LENGTH bytes from
RE: [PATCH] Fix MIPS-specific ICE in gcc.dg/pr77834.c (PR rtl-optimization/79150).
> -Original Message-
> From: Matthew Fortune [mailto:matthew.fort...@imgtec.com]
> Sent: Wednesday, March 15, 2017 11:30 AM
> To: Moore, Catherine
> Cc: Segher Boessenkool (seg...@kernel.crashing.org)
> ; Toma Tabacu
> ; gcc-patches@gcc.gnu.org
> Subject: RE: [PATCH] Fix MIPS-specific ICE in gcc.dg/pr77834.c (PR rtl-
> optimization/79150).
>
> Hi Catherine,
>
> What is your opinion on this patch? I am OK with the temporary
> workaround on the basis that the additional nop is successfully
> eliminated so there is no code size increase. Also, I am happy
> enough that the CFG is fixed very soon after the block move is
> expanded so the 'bad' basic block is fixed almost immediately
> anyway making the offending check potentially unnecessary in
> the first place.
>
I'm okay with the workaround for stage 4, but would like to see the pr remain
open until a proper fix is installed on trunk.
Toma, would you be able to pursue the original patch that you attached to the
bug report?
Catherine
>
> > -Original Message-
> > From: Toma Tabacu
> > Sent: 07 March 2017 11:44
> > To: gcc-patches@gcc.gnu.org
> > Cc: Matthew Fortune; Segher Boessenkool
> (seg...@kernel.crashing.org)
> > Subject: [PATCH] Fix MIPS-specific ICE in gcc.dg/pr77834.c (PR rtl-
> > optimization/79150).
> >
> > Hi,
> >
> > This ICE is caused by "gcc_assert (!JUMP_P (last))" in
> > commit_one_edge_insertion (gcc/cfgrtl.c):
> >
> > if (returnjump_p (last))
> > {
> > /* ??? Remove all outgoing edges from BB and add one for EXIT.
> > This is not currently a problem because this only happens
> > for the (single) epilogue, which already has a fallthru edge
> > to EXIT. */
> >
> > e = single_succ_edge (bb);
> > gcc_assert (e->dest == EXIT_BLOCK_PTR_FOR_FN (cfun)
> > && single_succ_p (bb) && (e->flags &
> EDGE_FALLTHRU));
> >
> > e->flags &= ~EDGE_FALLTHRU;
> > emit_barrier_after (last);
> >
> > if (before)
> > delete_insn (before);
> > }
> > else
> > gcc_assert (!JUMP_P (last));
> >
> > The assert is triggered because we're removing an edge which ends on a
> > conditional jump, which is part of a loop.
> >
> > The reason for the existence of this loop is the size of the vector used
> > in gcc.dg/pr77834.c.
> > When compiling code which uses vectors for MIPS, a looping memcpy is
> > generated if the vectors are big enough (>32 bytes for MIPS32 or >64
> > bytes for MIPS64).
> > This takes place in mips_expand_block_move (gcc/config/mips.c).
> >
> > In our case, a looping memcpy gets generated for a partition copy
> which
> > is inserted on an edge (in insert_partition_copy_on_edge, gcc/tree-
> > outof-ssa.c).
> > This happens during PHI node elimination, which is done by
> eliminate_phi
> > (gcc/tree-outof-ssa.c), as a part of expand_phi_nodes, which is called
> > by the expand pass (gcc/cfgexpand.c).
> >
> > My original fix was to update the CFG by calling
> > find_many_sub_basic_blocks with an all-one block bitmap (which also
> > happens in cfgexpand.c, after edge
> > removal) whenever an edge contains an insn which satisfies
> > control_flow_insn_p.
> > However, Segher Boessenkool said that this would be too risky for stage
> > 4 and suggested inserting a NOP after the conditional jump in order to
> > fool the assert. This assumes that it is safe to delay the CFG update
> > for this particular case.
> >
> > This patch changes mips_block_move_loop to emit a NOP after the
> > conditional jump, if the jump is the last insn of the loop. This
> > prevents "gcc_assert (!JUMP_P (last))" from triggering.
> >
> > The NOP will never make it into the final assembly code because it is
> > removed during the cse1 pass through DCE for -O1, -O2, and -O3, and
> it's
> > not even emitted for -O0 and -Os because looping memcpy's are not
> > generated for those optimization levels, as can be seen in
> > mips_expand_block_move from mips.c:
> >
> > if (INTVAL (length) <= MIPS_MAX_MOVE_BYTES_STRAIGHT)
> > {
> > mips_block_move_straight (dest, src, INTVAL (length));
> > return true;
> > }
> > else if (optimize)
> > {
> > mips_block_move_loop (dest, src, INTVAL (length),
> >
> MIPS_MAX_MOVE_BYTES_PER_LOOP_ITER);
> > return true;
> > }
> >
> > Tested with m
Re: [PATCH] Fix MIPS-specific ICE in gcc.dg/pr77834.c (PR rtl-optimization/79150).
On Mon, Mar 20, 2017 at 10:08:25PM +, Moore, Catherine wrote: > I'm okay with the workaround for stage 4, but would like to see the pr remain > open until a proper fix is installed on trunk. Yeah. > Toma, would you be able to pursue the original patch that you attached to the > bug report? It isn't clear we should allow jump instructions there, but something needs to be done certainly. Segher
RE: [PATCH] Fix MIPS-specific ICE in gcc.dg/pr77834.c (PR rtl-optimization/79150).
Hi, > From: Segher Boessenkool > On Mon, Mar 20, 2017 at 10:08:25PM +, Moore, Catherine wrote: > > I'm okay with the workaround for stage 4, but would like to see the pr > > remain > open until a proper fix is installed on trunk. > > Yeah. > Sure, I'll keep it open. > > Toma, would you be able to pursue the original patch that you attached to > > the > bug report? Yes, I intend to work on it after stage 4 ends. The temporary fix was committed as r246320. Thanks, Toma
