| Message ID | 9875286e-cc51-abed-626a-4b36717e52e3@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [asmcons] Fix PR rtl-optimization/89313: ICE in process_alt_operands, at lra-constraints.c:2962 | expand |
Hi Peter, On Wed, Feb 20, 2019 at 09:19:58PM -0600, Peter Bergner wrote: > PR89313 exposes a bug in the asmcons pass where it replaces input operands > with matching constraints with their associated output operands, as well as > all other uses of its pseudo registers. This is normally fine. However, if > the matched output operand is marked as early clobber, then we cannot replace > the uses of 'input' that do not have matching constraints, since they by > definition conflict with the early clobber output operand and could be > clobbered if assigned to the same register as the output operand. > > The patch below fixes the bug by only doing the input pseudo replacement > if the output operand is not early clobber or the input operand is known > to be a matching constraint. > > This passed bootstrap and regression testing with no regressions on > both x86_64-linux and powerpc64le-linux. Ok for mainline? Looks fine to me, but I cannot approve it of course. Some trivial comments: > +/* If CONSTRAINT is a matching constraint, then return its number. > + Otherwise, return -1. */ > + > +static int > +matching_constraint_num (const char *constraint) > +{ > + int match; > + > + if (*constraint == '%') > + constraint++; > + > + switch (*constraint) > + { > + case '0': case '1': case '2': case '3': case '4': > + case '5': case '6': case '7': case '8': case '9': > + { > + char *end; > + match = strtoul (constraint, &end, 10); > + if (end == constraint) > + match = -1; This condition is always false, because we have at least one digit. > + break; > + } > + > + default: > + match = -1; > + break; > + } > + return match; > +} Which means you can write it as just static int matching_constraint_num (const char *constraint) { if (*constraint == '%') constraint++; if (IN_RANGE (*constraint, '0', '9')) return strtoul (constraint, NULL, 10); return -1; } Segher
On 3/6/19 8:47 AM, Segher Boessenkool wrote: > Which means you can write it as just > > static int > matching_constraint_num (const char *constraint) > { > if (*constraint == '%') > constraint++; > > if (IN_RANGE (*constraint, '0', '9')) > return strtoul (constraint, NULL, 10); > > return -1; > } Ok, changed. Thanks! Peter
On 3/6/19 8:56 AM, Peter Bergner wrote: > On 3/6/19 8:47 AM, Segher Boessenkool wrote: >> Which means you can write it as just >> >> static int >> matching_constraint_num (const char *constraint) >> { >> if (*constraint == '%') >> constraint++; >> >> if (IN_RANGE (*constraint, '0', '9')) >> return strtoul (constraint, NULL, 10); >> >> return -1; >> } > > Ok, changed. Thanks! ...and re-bootstrapping and regtesting were clean with that change. Peter
On 2/20/19 8:19 PM, Peter Bergner wrote: > PR89313 exposes a bug in the asmcons pass where it replaces input operands > with matching constraints with their associated output operands, as well as > all other uses of its pseudo registers. This is normally fine. However, if > the matched output operand is marked as early clobber, then we cannot replace > the uses of 'input' that do not have matching constraints, since they by > definition conflict with the early clobber output operand and could be > clobbered if assigned to the same register as the output operand. > > The patch below fixes the bug by only doing the input pseudo replacement > if the output operand is not early clobber or the input operand is known > to be a matching constraint. > > This passed bootstrap and regression testing with no regressions on > both x86_64-linux and powerpc64le-linux. Ok for mainline? > > Peter > > > gcc/ > PR rtl-optimization/89313 > * function.c (matching_constraint_num): New static function. > (match_asm_constraints_1): Use it. Fixup white space and comment. > Don't replace inputs with non-matching constraints which conflict > with early clobber outputs. > > gcc/testsuite/ > PR rtl-optimization/89313 > * gcc.dg/pr89313.c: New test. OK. jeff
On 3/25/19 3:36 PM, Jeff Law wrote: > On 2/20/19 8:19 PM, Peter Bergner wrote: >> gcc/ >> PR rtl-optimization/89313 >> * function.c (matching_constraint_num): New static function. >> (match_asm_constraints_1): Use it. Fixup white space and comment. >> Don't replace inputs with non-matching constraints which conflict >> with early clobber outputs. >> >> gcc/testsuite/ >> PR rtl-optimization/89313 >> * gcc.dg/pr89313.c: New test. > OK. I did another round of bootstrap and regtesting, since I was on vacation and trunk has changed. The testing was still clean, so it's committed now. Thanks! Peter
Index: gcc/function.c =================================================================== --- gcc/function.c (revision 268883) +++ gcc/function.c (working copy) @@ -6395,6 +6395,36 @@ make_pass_thread_prologue_and_epilogue ( } +/* If CONSTRAINT is a matching constraint, then return its number. + Otherwise, return -1. */ + +static int +matching_constraint_num (const char *constraint) +{ + int match; + + if (*constraint == '%') + constraint++; + + switch (*constraint) + { + case '0': case '1': case '2': case '3': case '4': + case '5': case '6': case '7': case '8': case '9': + { + char *end; + match = strtoul (constraint, &end, 10); + if (end == constraint) + match = -1; + break; + } + + default: + match = -1; + break; + } + return match; +} + /* This mini-pass fixes fall-out from SSA in asm statements that have in-out constraints. Say you start with @@ -6453,14 +6483,10 @@ match_asm_constraints_1 (rtx_insn *insn, rtx input, output; rtx_insn *insns; const char *constraint = ASM_OPERANDS_INPUT_CONSTRAINT (op, i); - char *end; int match, j; - if (*constraint == '%') - constraint++; - - match = strtoul (constraint, &end, 10); - if (end == constraint) + match = matching_constraint_num (constraint); + if (match < 0) continue; gcc_assert (match < noutputs); @@ -6477,14 +6503,14 @@ match_asm_constraints_1 (rtx_insn *insn, /* We can't do anything if the output is also used as input, as we're going to overwrite it. */ for (j = 0; j < ninputs; j++) - if (reg_overlap_mentioned_p (output, RTVEC_ELT (inputs, j))) + if (reg_overlap_mentioned_p (output, RTVEC_ELT (inputs, j))) break; if (j != ninputs) continue; /* Avoid changing the same input several times. For asm ("" : "=mr" (out1), "=mr" (out2) : "0" (in), "1" (in)); - only change in once (to out1), rather than changing it + only change it once (to out1), rather than changing it first to out1 and afterwards to out2. */ if (i > 0) { @@ -6502,6 +6528,9 @@ match_asm_constraints_1 (rtx_insn *insn, end_sequence (); emit_insn_before (insns, insn); + constraint = ASM_OPERANDS_OUTPUT_CONSTRAINT(SET_SRC(p_sets[match])); + bool early_clobber_p = strchr (constraint, '&') != NULL; + /* Now replace all mentions of the input with output. We can't just replace the occurrence in inputs[i], as the register might also be used in some other input (or even in an address of an @@ -6523,7 +6552,14 @@ match_asm_constraints_1 (rtx_insn *insn, value, but different pseudos) where we formerly had only one. With more complicated asms this might lead to reload failures which wouldn't have happen without this pass. So, iterate over - all operands and replace all occurrences of the register used. */ + all operands and replace all occurrences of the register used. + + However, if one or more of the 'input' uses have a non-matching + constraint and the matched output operand is an early clobber + operand, then do not replace the input operand, since by definition + it conflicts with the output operand and cannot share the same + register. See PR89313 for details. */ + for (j = 0; j < noutputs; j++) if (!rtx_equal_p (SET_DEST (p_sets[j]), input) && reg_overlap_mentioned_p (input, SET_DEST (p_sets[j]))) @@ -6531,8 +6567,13 @@ match_asm_constraints_1 (rtx_insn *insn, input, output); for (j = 0; j < ninputs; j++) if (reg_overlap_mentioned_p (input, RTVEC_ELT (inputs, j))) - RTVEC_ELT (inputs, j) = replace_rtx (RTVEC_ELT (inputs, j), - input, output); + { + if (!early_clobber_p + || match == matching_constraint_num + (ASM_OPERANDS_INPUT_CONSTRAINT (op, j))) + RTVEC_ELT (inputs, j) = replace_rtx (RTVEC_ELT (inputs, j), + input, output); + } changed = true; } Index: gcc/testsuite/gcc.dg/pr89313.c =================================================================== --- gcc/testsuite/gcc.dg/pr89313.c (nonexistent) +++ gcc/testsuite/gcc.dg/pr89313.c (working copy) @@ -0,0 +1,26 @@ +/* PR rtl-optimization/89313 */ +/* { dg-do compile { target aarch64*-*-* arm*-*-* i?86-*-* powerpc*-*-* s390*-*-* x86_64-*-* } } */ +/* { dg-options "-O2" } */ + +#if defined (__aarch64__) +# define REG "x0" +#elif defined (__arm__) +# define REG "r0" +#elif defined (__i386__) +# define REG "%eax" +#elif defined (__powerpc__) +# define REG "r3" +#elif defined (__s390__) +# define REG "0" +#elif defined (__x86_64__) +# define REG "rax" +#endif + +long +bug (long arg) +{ + register long output asm (REG); + long input = arg; + asm ("blah %0, %1, %2" : "=&r" (output) : "r" (input), "0" (input)); + return output; +}