On Tue, Mar 16, 2021 at 11:10 AM Jakub Jelinek <ja...@redhat.com> wrote:
>
> Hi!
>
> As the testcase shows, the compiler hangs and eats all memory when compiling
> it. This is because in r11-7274-gdecd8fb0128870d0d768ba53dae626913d6d9c54
> I have changed the ix86_avoid_lea_for_addr splitting from a splitter
> into a peephole2 (because during splitting passes we don't have guaranteed
> df, while during peephole2 we do).
> The problem is we have another peephole2 that works in the opposite way,
> when seeing split lea (in particular ASHIFT followed by PLUS) it attempts
> to turn it back into a lea.
> In the past, they were fighting against each other, but as they were in
> different passes, simply the last one won. So, split after reload
> split the lea into shift left and plus, peephole2 reverted that (but, note
> not perfectly, the peephole2 doesn't understand that something can be placed
> into lea disp; to be fixed for GCC12) and then another split pass split the
> lea appart again.
> But my changes and the way peephole2 works means that we endlessly iterate
> over those two, the first peephole2 splits the lea, the second one reverts
> it, the first peephole2 splits the new lea back into new 2 insns and so
> forth forever.
> So, we need to break the cycle somehow. Best would be to call
> ix86_avoid_lea_for_addr in the second peephole2 and if it says it would be
> split, undo, but unfortunately calling that function is very non-trivial,
> because it needs to walk insns forwards and backwards from the given insn
> and uses df in those walks. Furthermore, one of the registers in the new
> lea is set by the newly added insn. So I'd be afraid we'd need to
> temporarily register the new insns with df and replace in the IL the old
> insns with new insns (and undo their df), then call the function and then
> undo everything we did.
> So, this patch instead breaks the cycle by remembering INSN_UID of the last
> ASHIFT ix86_split_lea_for_addr emitted and punting on the ASHIFT + PLUS
> peephole2 when it is that uid. We need to reset it somewhere, so I've
> added clearing of the var to ix86_expand_prologue which is guaranteed to be
> a few passes before peephole2.
Maybe we could simply emit a special form of a ASHIFT pattern, tagged
with some unspec (similar to e.g. divmod<mode>4_1), and teach
ix86_split_lea_for_addr to emit it instead? Peephole pass is so late
in the pass sequence that we won't lose anything. We only need one
additional SWI48mode ASHIFT pattern with a const123_operand immediate.
Uros.
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> Or would you e.g. prefer if I don't use a global var but stick it into
> cfun->machine->last_lea_split_shift_uid ? That way it would be cleared
> automatically.
>
> 2021-03-16 Jakub Jelinek <ja...@redhat.com>
>
> PR target/99600
> * config/i386/i386-protos.h (ix86_last_lea_split_shift_uid): Declare.
> * config/i386/i386-expand.c (ix86_last_lea_split_shift_uid): New
> variable.
> (ix86_split_lea_for_addr): Set it to INSN_UID of the emitted shift
> insn.
> * config/i386/i386.md (ashift + add peephole2 into lea): Punt if
> INSN_UID of ashift is ix86_last_lea_split_shift_uid.
> * config/i386/i386.c (ix86_expand_prologue): Clear
> ix86_last_lea_split_shift_uid.
>
> * gcc.target/i386/pr99600.c: New test.
>
> --- gcc/config/i386/i386-protos.h.jj 2021-01-04 10:25:45.436159056 +0100
> +++ gcc/config/i386/i386-protos.h 2021-03-15 20:06:36.267515383 +0100
> @@ -109,6 +109,7 @@ extern bool ix86_binary_operator_ok (enu
> extern bool ix86_avoid_lea_for_add (rtx_insn *, rtx[]);
> extern bool ix86_use_lea_for_mov (rtx_insn *, rtx[]);
> extern bool ix86_avoid_lea_for_addr (rtx_insn *, rtx[]);
> +extern int ix86_last_lea_split_shift_uid;
> extern void ix86_split_lea_for_addr (rtx_insn *, rtx[], machine_mode);
> extern bool ix86_lea_for_add_ok (rtx_insn *, rtx[]);
> extern bool ix86_vec_interleave_v2df_operator_ok (rtx operands[3], bool
> high);
> --- gcc/config/i386/i386-expand.c.jj 2021-03-15 12:34:26.549901726 +0100
> +++ gcc/config/i386/i386-expand.c 2021-03-15 20:08:52.992016204 +0100
> @@ -1291,6 +1291,10 @@ find_nearest_reg_def (rtx_insn *insn, in
> return false;
> }
>
> +/* INSN_UID of the last ASHIFT insn emitted by ix86_split_lea_for_addr
> + or zero if none has been emitted yet. */
> +int ix86_last_lea_split_shift_uid;
> +
> /* Split lea instructions into a sequence of instructions
> which are executed on ALU to avoid AGU stalls.
> It is assumed that it is allowed to clobber flags register
> @@ -1352,6 +1356,9 @@ ix86_split_lea_for_addr (rtx_insn *insn,
> ix86_emit_binop (ASHIFT, mode, target,
> GEN_INT (exact_log2 (parts.scale)));
>
> + rtx_insn *ashift_insn = get_last_insn ();
> + ix86_last_lea_split_shift_uid = INSN_UID (ashift_insn);
> +
> if (parts.base)
> ix86_emit_binop (PLUS, mode, target, parts.base);
>
> --- gcc/config/i386/i386.md.jj 2021-03-05 21:51:33.675350463 +0100
> +++ gcc/config/i386/i386.md 2021-03-15 20:16:03.796292449 +0100
> @@ -20680,6 +20680,11 @@ (define_peephole2
> (match_operand 4 "x86_64_general_operand")))
> (clobber (reg:CC FLAGS_REG))])]
> "IN_RANGE (INTVAL (operands[2]), 1, 3)
> + /* Avoid the ix86_avoid_lea_for_addr peephole2 from splitting
> + lea and this peephole2 to undo that into another lea that
> + would be split again. Break that cycle by ignoring the last
> + shift that has been emitted by ix86_avoid_lea_for_addr. */
> + && INSN_UID (insn) != ix86_last_lea_split_shift_uid
> /* Validate MODE for lea. */
> && ((!TARGET_PARTIAL_REG_STALL
> && (GET_MODE (operands[0]) == QImode
> --- gcc/config/i386/i386.c.jj 2021-03-04 16:00:44.062251517 +0100
> +++ gcc/config/i386/i386.c 2021-03-15 20:09:40.382496567 +0100
> @@ -8155,6 +8155,7 @@ ix86_expand_prologue (void)
> bool save_stub_call_needed;
> rtx static_chain = NULL_RTX;
>
> + ix86_last_lea_split_shift_uid = 0;
> if (ix86_function_naked (current_function_decl))
> {
> if (flag_stack_usage_info)
> --- gcc/testsuite/gcc.target/i386/pr99600.c.jj 2021-03-15 20:26:43.868274088
> +0100
> +++ gcc/testsuite/gcc.target/i386/pr99600.c 2021-03-15 20:20:08.802605967
> +0100
> @@ -0,0 +1,16 @@
> +/* PR target/99600 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -march=atom" } */
> +
> +char a, b;
> +long c;
> +
> +long
> +foo (void)
> +{
> + if (a)
> + c = b == 1 ? 1 << 3 : 1 << 2;
> + else
> + c = 0;
> + return 0;
> +}
>
> Jakub
>
