| Message ID | CACgzC7BAgdoKmKdNTnqoB+WgihgrAhSc4=FdXVUCRB7Pw6T+6Q@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Mar 26, 2014 at 02:16:16PM +0800, Zhenqiang Chen wrote: > The patch checks the number of the expected operands in > ASM_OPERANDS_TEMPLATE with the same logic as it in output_asm_insn to > make sure the ASM_OPERANDS are legal. > > Bootstrap and no make check regression on X86-64 and ARM chromebook. > > OK for trunk? No, this is very wrong. How many operands you refer to and how many times to each is completely unrelated to the fact that CSE should never modify asm insns to drop some of the outputs. You can refer to no operands at all and still it would be invalid to drop the outputs, or you can have output template like "%0 %0 %0 %0 %0 ... million times %0". Jakub
On 26 March 2014 15:00, Jakub Jelinek <jakub@redhat.com> wrote: > On Wed, Mar 26, 2014 at 02:16:16PM +0800, Zhenqiang Chen wrote: >> The patch checks the number of the expected operands in >> ASM_OPERANDS_TEMPLATE with the same logic as it in output_asm_insn to >> make sure the ASM_OPERANDS are legal. >> >> Bootstrap and no make check regression on X86-64 and ARM chromebook. >> >> OK for trunk? > > No, this is very wrong. How many operands you refer to and how many times This is how the output_asm_insn (final.c) check and output the error message. The logic in my patch is the same as it in output_asm_insn. > to each is completely unrelated to the fact that CSE should never modify > asm insns to drop some of the outputs. Agree. CSE should never modify asm insns to drop some of the outputs. But in this case, CSE does not drop any of the outputs. It just takes the SRC of a set and replace the reference of the set. And the instruction validation tells CSE that it is legal instruction after replacement. (The original correct asm insn is optimized away after this replacement) I think it is common for most rtl-optimizations to do such kind of validation. So to avoid such kind of bug, check_asm_operands must tell the optimizer the asm is illegal. > You can refer to no operands at all and still it would be invalid to drop > the outputs, or you can have output template like "%0 %0 %0 %0 %0 ... million times %0". I think it does not check the number of %0, but the max "n" in %n. Thanks! -Zhenqiang > Jakub
On Wed, Mar 26, 2014 at 03:30:44PM +0800, Zhenqiang Chen wrote: > Agree. CSE should never modify asm insns to drop some of the outputs. So the right fix is top prevent this from happening, not papering over about it. > > But in this case, CSE does not drop any of the outputs. It just takes > the SRC of a set and replace the reference of the set. And the > instruction validation tells CSE that it is legal instruction after > replacement. (The original correct asm insn is optimized away after > this replacement) > > I think it is common for most rtl-optimizations to do such kind of > validation. So to avoid such kind of bug, check_asm_operands must tell > the optimizer the asm is illegal. As it is wrong if CSE does that even with asm ("" : "=r" (i), "=r" (j));, your patch is not the right place to fix this. CSE just must check where the ASM_OPERANDS is coming from and if it comes from a PARALLEL with multiple outputs, either give up or duplicate all outputs (if it makes sense at all). Or just don't enter into the hash tables ASM_OPERANDS with multiple outputs. Jakub
On 26 March 2014 15:45, Jakub Jelinek <jakub@redhat.com> wrote: > On Wed, Mar 26, 2014 at 03:30:44PM +0800, Zhenqiang Chen wrote: >> Agree. CSE should never modify asm insns to drop some of the outputs. > > So the right fix is top prevent this from happening, not papering over about > it. >> >> But in this case, CSE does not drop any of the outputs. It just takes >> the SRC of a set and replace the reference of the set. And the >> instruction validation tells CSE that it is legal instruction after >> replacement. (The original correct asm insn is optimized away after >> this replacement) >> >> I think it is common for most rtl-optimizations to do such kind of >> validation. So to avoid such kind of bug, check_asm_operands must tell >> the optimizer the asm is illegal. > > As it is wrong if CSE does that even with asm ("" : "=r" (i), "=r" (j));, > your patch is not the right place to fix this. CSE just must check where > the ASM_OPERANDS is coming from and if it comes from a PARALLEL with > multiple outputs, either give up or duplicate all outputs (if it makes sense > at all). Or just don't enter into the hash tables ASM_OPERANDS with > multiple outputs. Thanks for the comments. I will rework the patch to check it in CSE. -Zhenqiang > Jakub
diff --git a/gcc/recog.c b/gcc/recog.c index f9040dc..65078ad 100644 --- a/gcc/recog.c +++ b/gcc/recog.c @@ -135,8 +135,8 @@ check_asm_operands (rtx x) { int noperands; rtx *operands; - const char **constraints; - int i; + const char **constraints, *templ; + int i, c; if (!asm_labels_ok (x)) return 0; @@ -159,7 +159,29 @@ check_asm_operands (rtx x) operands = XALLOCAVEC (rtx, noperands); constraints = XALLOCAVEC (const char *, noperands); - decode_asm_operands (x, operands, NULL, constraints, NULL, NULL); + templ = decode_asm_operands (x, operands, NULL, constraints, NULL, NULL); + /* The following logic is similar with it in output_asm_insn (final.c). + It checks the number of expected operands in ASM_OPERANDS_TEMPLATE. */ + if (*templ) + { + const char* p = templ; + while ((c = *p++)) + { + if (c == '%') + if (ISDIGIT (*p)) + { + int opnum; + char *endptr; + + opnum = strtoul (p, &endptr, 10); + if (opnum >= noperands) + return 0; + + p = endptr; + c = *p; + } + } + } for (i = 0; i < noperands; i++) { diff --git a/gcc/testsuite/gcc.dg/pr60663.c b/gcc/testsuite/gcc.dg/pr60663.c new file mode 100644 index 0000000..6c01084 --- /dev/null +++ b/gcc/testsuite/gcc.dg/lp.c @@ -0,0 +1,9 @@ +/* { dg-do compile } */ +/* { dg-options " -O2 " } */ + +int g (void) +{ + unsigned i, j; + asm("// %0 %1" : "=r" (i), "=r"(j)); + return i; +}
Hi, The patch checks the number of the expected operands in ASM_OPERANDS_TEMPLATE with the same logic as it in output_asm_insn to make sure the ASM_OPERANDS are legal. Bootstrap and no make check regression on X86-64 and ARM chromebook. OK for trunk? Thanks! -Zhenqiang ChangeLog: 2014-03-26 Zhenqiang Chen <zhenqiang.chen@linaro.org> PR rtl-optimization/pr60663 * recog.c (check_asm_operands): Check the number of expected operands. testsuite/ChangeLog: 2014-03-26 Zhenqiang Chen <zhenqiang.chen@linaro.org> * gcc.dg/pr60663: New testcase.