Hi!
Please Cc: me on combine patches, I cannot keep up with gcc-patches@,
and even if I could I would miss things.
On Thu, Feb 03, 2022 at 11:05:48AM -0000, Roger Sayle wrote:
> This patch addresses PR rtl-optimization/101885 which is a P2 wrong code
> regression. In combine, if the resulting fused instruction is a parallel
> of two sets which fails to be recognized by the backend, combine tries to
> emit these as two sequential set instructions (known as split_i2i3).
> As each set is recognized the backend may add any necessary "clobbers".
> The code currently checks that any clobbers added to the first "set"
> don't interfere with the second set, but doesn't currently handle the
> case that clobbers added to the second set may interfere/kill the
> destination of the first set (which must be live at this point).
The insns are inserted where the original I2 and I3 were, so "must be"
isn't obvious at all.
> The solution is to cut'n'paste the "clobber" logic from just a few
> lines earlier, suitably adjusted for the second instruction.
>
> One minor nit that may confuse a reviewer is that at this point in
> the code we've lost track of which set was first and which was second
> (combine chooses dynamically, and the recog processes that adds the
> clobbers may have obfuscated the original SET_DEST) so the actual test
> below is to confirm that any newly added clobbers (on the second set
> instruction) don't overlap either set0's or set1's destination.
There is no "which was": they both were to be inserted at I3, as a
parallel. In no case does combine backtrack, or try multiple solutions
to do something (this would be prohibitively expensive).
> gcc/ChangeLog
> PR rtl-optimization/101885
> * combine.c (try_combine): When splitting a parallel into two
> sequential sets, check not only that the first doesn't clobber
> the second but also that the second doesn't clobber the first.
>
> gcc/testsuite/ChangeLog
> PR rtl-optimization/101885
> * gcc.dg/pr101885.c: New test case.
> index 2d40635..38e0369 100644
> --- a/gcc/combine.cc
> +++ b/gcc/combine.cc
> @@ -4017,6 +4017,23 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1,
> rtx_insn *i0,
>
> insn_code_number = recog_for_combine (&newpat, i3, &new_i3_notes);
>
> + /* Likewise, recog_for_combine might have added clobbers to NEWPAT.
> + Make sure NEWI2PAT2's original SET_DEST isn't clobbered. */
"original SET_DEST"? Please rephrase this, so it is what the code does
(and makes sense :-) )? It is fine to talk about SET0 and SET1.
> + if (insn_code_number >= 0 && GET_CODE (newpat) == PARALLEL)
> + {
> + for (i = XVECLEN (newpat, 0) - 1; i >= 0; i--)
> + if (GET_CODE (XVECEXP (newpat, 0, i)) == CLOBBER)
> + {
> + rtx reg = XEXP (XVECEXP (newpat, 0, i), 0);
> + if (reg_overlap_mentioned_p (reg, SET_DEST (set0))
> + || reg_overlap_mentioned_p (reg, SET_DEST (set1)))
> + {
> + undo_all ();
> + return 0;
> + }
> + }
> + }
The patch is okay for trunk. Thank you!
Also okay for backports (after waiting for fallout a week or two).
Segher
