Uros Bizjak <ubiz...@gmail.com> writes:
> Hello!
>
> RTL store motion pass transforms:
>
> (insn 19 18 20 4 (parallel [
> (set (mem/c:SI (symbol_ref:SI ("bar_arg") [flags 0x2]
> <var_decl 0x7f68136f2b40 bar_arg>) [1 bar_arg+0 S4 A32])
> (fix:SI (reg:DF 89)))
> (clobber (scratch:XF))
> ]) "stdarg-3.c":63:7 156 {fix_truncsi_i387_fisttp}
> (expr_list:REG_DEAD (reg:DF 89)
> (nil)))
>
> to an unrecognisable insn:
>
> (insn 33 18 20 4 (set (reg:SI 93 [ bar_arg ])
> (fix:SI (reg:DF 89))) "stdarg-3.c":63:7 -1
> (expr_list:REG_DEAD (reg:DF 89)
> (nil)))
>
> The problem is with can_assign_to_reg_without_clobbers_p in gcse.c,
> where we have:
>
> /* If the test insn is valid and doesn't need clobbers, and the target also
> has no objections, we're good. */
> if (icode >= 0
> && (num_clobbers == 0 || !added_clobbers_hard_reg_p (icode))
> && ! (targetm.cannot_copy_insn_p
> && targetm.cannot_copy_insn_p (test_insn)))
> can_assign = true;
>
> The test instruction is created as:
>
> (insn 26 0 0 (set (reg:SI 152)
> (fix:SI (reg:DF 89))) -1
> (nil))
>
> which is (correctly) recognized as
>
> (define_insn "fix_trunc<mode>_i387_fisttp"
> [(set (match_operand:SWI248x 0 "nonimmediate_operand" "=m")
> (fix:SWI248x (match_operand 1 "register_operand" "f")))
> (clobber (match_scratch:XF 2 "=&f"))]
>
> However, recog also reports that 1 clobber needs to be added. The
> instruction is recognized nevertheless due to "||
> !added_clobbers_hard_reg_p (icode)" bypass. The recognized insn
> doesn't clobber hard reg, but it also needs a clobber of a scratch reg
> to be recognized.
>
> The solution is to recognize the replacement insn in
> replace_store_insn. We are sure that the new instruction is valid (it
> passed can_assign_to_reg_without_clobbers_p when analysed), but we
> need to add clobbers, as is the case with the
> {fix_truncsi_i387_fisttp}. The call to recog will provide the number
> of necessary clobbers for recognized insn, so we are able to wrap the
> original pattern with a PARALLEL comprising required clobbers.
>
> 2019-01-22 Uroš Bizjak <ubiz...@gmail.com>
>
> PR target/88948
> * store-motion.c: Include insn-config.h and recog.h
> (replace_store_insn): Recognize new store insn and
> add clobbers if needed.
>
> testsuite/ChangeLog:
>
> 2019-01-22 Uroš Bizjak <ubiz...@gmail.com>
>
> PR target/88948
> * gcc.target/i386/pr88948.c: New test.
>
> Patch was bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.
>
> OK for mainline?
>
> Uros.
>
> Index: store-motion.c
> ===================================================================
> --- store-motion.c (revision 268157)
> +++ store-motion.c (working copy)
> @@ -27,6 +27,8 @@ along with GCC; see the file COPYING3. If not see
> #include "df.h"
> #include "toplev.h"
>
> +#include "insn-config.h"
> +#include "recog.h"
> #include "cfgrtl.h"
> #include "cfganal.h"
> #include "lcm.h"
> @@ -910,11 +912,28 @@ replace_store_insn (rtx reg, rtx_insn *del, basic_
> struct st_expr *smexpr)
> {
> rtx_insn *insn;
> + rtx pat;
> + int insn_code_number;
> + int num_clobbers_to_add = 0;
> rtx mem, note, set;
>
> mem = smexpr->pattern;
> insn = gen_move_insn (reg, SET_SRC (single_set (del)));
>
> + pat = PATTERN (insn);
> + insn_code_number = recog (pat, insn, &num_clobbers_to_add);
> +
> + if (num_clobbers_to_add)
> + {
> + rtx newpat
> + = gen_rtx_PARALLEL (VOIDmode, rtvec_alloc (num_clobbers_to_add + 1));
> +
> + XVECEXP (newpat, 0, 0) = pat;
> + add_clobbers (newpat, insn_code_number);
> +
> + PATTERN (insn) = newpat;
> + }
Another potential problem is that gen_move_insn should only really
be used for general_operands.
process_insert_insn seems to get this right, so it'd probably be
cleanest to split the innards out into something that takes the dest
and source rtxes of the move, then reuse that here.
Thanks,
Richard
