| Message ID | 87sipejow3.fsf@talisman.default |
|---|---|
| State | New |
| Headers | show |
On Tue, Apr 15, 2014 at 09:53:16PM +0100, Richard Sandiford wrote: > As Robert pointed out here: > > http://gcc.gnu.org/ml/gcc-patches/2014-04/msg00416.html > > we're a bit too eager when folding stuff into an 'X' constraint. > The value at expand time is sensible, but after that asm_operand_ok > allows arbitrary rtx expressions, including any number of registers > as well as MEMs with unchecked addresses. > > This is a target-independent problem, as shown by the testcase below. > Reload would give bogus "impossible constraint in asm" errors > while LRA ICEs. > > Tested on x86_64-linux-gnu. OK to install? But then what will be "X" good for compared to "gin" or similar? X constraint is meant for operands that aren't really needed, trying to print is a user error. I guess the documentation agrees with this: 'X' Any operand whatsoever is allowed, even if it does not satisfy 'general_operand'. This is normally used in the constraint of a 'match_scratch' when certain alternatives will not actually require a scratch register. So I think we should just error out if somebody tries to print something that satisfies X constraint. Jakub
On Tue, Apr 15, 2014 at 1:53 PM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > As Robert pointed out here: > > http://gcc.gnu.org/ml/gcc-patches/2014-04/msg00416.html > > we're a bit too eager when folding stuff into an 'X' constraint. > The value at expand time is sensible, but after that asm_operand_ok > allows arbitrary rtx expressions, including any number of registers > as well as MEMs with unchecked addresses. > > This is a target-independent problem, as shown by the testcase below. > Reload would give bogus "impossible constraint in asm" errors > while LRA ICEs. > > Tested on x86_64-linux-gnu. OK to install? AARCH64 ran into something similar and we did a similar patch though rejecting only mems which are invalid: http://gcc.gnu.org/ml/gcc-patches/2012-12/msg00765.html (http://gcc.gnu.org/ml/gcc-patches/2013-01/msg01128.html) Thanks, Andrew Pinski > > Thanks, > Richard > > > gcc/ > * recog.c (asm_operand_ok): Tighten MEM validity for 'X'. > > gcc/testsuite/ > * gcc.dg/torture/asm-x-constraint-1.c: New test. > > Index: gcc/recog.c > =================================================================== > --- gcc/recog.c 2014-04-12 22:43:54.729854903 +0100 > +++ gcc/recog.c 2014-04-15 21:47:32.139873570 +0100 > @@ -1840,7 +1840,17 @@ asm_operand_ok (rtx op, const char *cons > break; > > case 'X': > - result = 1; > + /* Although the asm itself doesn't impose any restrictions on > + the operand, we still need to restrict it to something that > + can be reloaded and printed. > + > + MEM operands are always reloaded to make them legitimate, > + regardless of the constraint, so we need to handle them > + in the same way as for 'm' and 'g'. Since 'X' is not treated > + as an address constraint, the only other valid operand types > + are constants and registers. */ > + result = (CONSTANT_P (op) > + || general_operand (op, VOIDmode)); > break; > > case 'g': > Index: gcc/testsuite/gcc.dg/torture/asm-x-constraint-1.c > =================================================================== > --- /dev/null 2014-04-15 08:10:27.294524132 +0100 > +++ gcc/testsuite/gcc.dg/torture/asm-x-constraint-1.c 2014-04-15 19:11:29.830962008 +0100 > @@ -0,0 +1,27 @@ > +void > +noprop1 (int **x, int y, int z) > +{ > + int *ptr = *x + y * z / 11; > + __asm__ __volatile__ ("noprop1 %0" : : "X" (*ptr)); > +} > + > +void > +noprop2 (int **x, int y, int z) > +{ > + int *ptr = *x + y * z / 11; > + __asm__ __volatile__ ("noprop2 %0" : : "X" (ptr)); > +} > + > +int *global_var; > + > +void > +const1 (void) > +{ > + __asm__ __volatile__ ("const1 %0" : : "X" (global_var)); > +} > + > +void > +const2 (void) > +{ > + __asm__ __volatile__ ("const2 %0" : : "X" (*global_var)); > +}
Jakub Jelinek <jakub@redhat.com> writes: > On Tue, Apr 15, 2014 at 09:53:16PM +0100, Richard Sandiford wrote: >> As Robert pointed out here: >> >> http://gcc.gnu.org/ml/gcc-patches/2014-04/msg00416.html >> >> we're a bit too eager when folding stuff into an 'X' constraint. >> The value at expand time is sensible, but after that asm_operand_ok >> allows arbitrary rtx expressions, including any number of registers >> as well as MEMs with unchecked addresses. >> >> This is a target-independent problem, as shown by the testcase below. >> Reload would give bogus "impossible constraint in asm" errors >> while LRA ICEs. >> >> Tested on x86_64-linux-gnu. OK to install? > > But then what will be "X" good for compared to "gin" or similar? > X constraint is meant for operands that aren't really needed, trying to > print is a user error. > > I guess the documentation agrees with this: > > 'X' > Any operand whatsoever is allowed, even if it does not satisfy > 'general_operand'. This is normally used in the constraint of a > 'match_scratch' when certain alternatives will not actually require > a scratch register. > > So I think we should just error out if somebody tries to print something > that satisfies X constraint. That's the internal documentation though, whereas here we're checking asm uses. The documentation for asms just says "Any operand whatsoever is allowed." It doesn't say anything about it being unprintable. I just added the printing side for completeness though. It wasn't the point of the patch. Like I say, the point is that LRA ICEs (on x86_64) because it can't reload the operands. Reload couldn't reload them either but raised "impossible constraint in asm" errors instead of internal errors. (IMO those errors were also invalid though. If "X" allows "any operand whatsoever", how can the operands in the testcase be invalid?) "X" was defined against reload, which always reloaded MEM addresses to follow the appropriate base and index register classes. This was done as a first pass before matching against the constraints: /* Examine each operand that is a memory reference or memory address and reload parts of the addresses into index registers. Also here any references to pseudo regs that didn't get hard regs but are equivalent to constants get replaced in the insn itself with those constants. Nobody will ever see them again. Finally, set up the preferred classes of each operand. */ for (i = 0; i < noperands; i++) { ... else if (code == MEM) { address_reloaded[i] = find_reloads_address (GET_MODE (recog_data.operand[i]), recog_data.operand_loc[i], XEXP (recog_data.operand[i], 0), &XEXP (recog_data.operand[i], 0), i, address_type[i], ind_levels, insn); recog_data.operand[i] = *recog_data.operand_loc[i]; substed_operand[i] = recog_data.operand[i]; So I don't think it has ever been the case that "X" allowed MEMs with arbitrary expressions as the address. IMO the point of "X" (as implied the doc you quoted) is that it allows (scratch) operands to be kept as (scratch)s in cases where no scratch register is needed. Thanks, Richard
Andrew Pinski <pinskia@gmail.com> writes: > On Tue, Apr 15, 2014 at 1:53 PM, Richard Sandiford > <rdsandiford@googlemail.com> wrote: >> As Robert pointed out here: >> >> http://gcc.gnu.org/ml/gcc-patches/2014-04/msg00416.html >> >> we're a bit too eager when folding stuff into an 'X' constraint. >> The value at expand time is sensible, but after that asm_operand_ok >> allows arbitrary rtx expressions, including any number of registers >> as well as MEMs with unchecked addresses. >> >> This is a target-independent problem, as shown by the testcase below. >> Reload would give bogus "impossible constraint in asm" errors >> while LRA ICEs. >> >> Tested on x86_64-linux-gnu. OK to install? > > AARCH64 ran into something similar and we did a similar patch though > rejecting only mems which are invalid: > http://gcc.gnu.org/ml/gcc-patches/2012-12/msg00765.html > (http://gcc.gnu.org/ml/gcc-patches/2013-01/msg01128.html) Sorry, missed that. I went for the same thing at first, but the second example in the testcase shows that it's needed for non-MEM operands too. Thanks, Richard
On Wed, Apr 16, 2014 at 11:43:12AM +0100, Richard Sandiford wrote: > "X" was defined against reload, which always reloaded MEM addresses > to follow the appropriate base and index register classes. This was > done as a first pass before matching against the constraints: I think it would be fine if "X" had a MEM that isn't valid to replace it say by (mem (scratch)) or similar. What I think "X" is useful for is e.g. if you want to describe e.g. a side-effect of inline-asm on certain location in memory, but don't really need the address of that memory. Often "memory" is too big hammer, people often say that certain inline-asm uses or sets or uses/sets or clobbers say 100 byte long piece of memory somewhere, but the operand is there solely to tell the compiler what memory it is. I think "X" constraint is good for that if you aren't planning to actually use the address anywhere. E.g. you call in inline-asm some function, but the address construction is in the callee, there is no point to costly compute the address in the caller (say for -fPIC). Jakub
Jakub Jelinek <jakub@redhat.com> writes: > On Wed, Apr 16, 2014 at 11:43:12AM +0100, Richard Sandiford wrote: >> "X" was defined against reload, which always reloaded MEM addresses >> to follow the appropriate base and index register classes. This was >> done as a first pass before matching against the constraints: > > I think it would be fine if "X" had a MEM that isn't valid to replace > it say by (mem (scratch)) or similar. > What I think "X" is useful for is e.g. if you want to describe e.g. a > side-effect of inline-asm on certain location in memory, but don't really > need the address of that memory. Often "memory" is too big hammer, > people often say that certain inline-asm uses or sets or uses/sets or > clobbers say 100 byte long piece of memory somewhere, but the operand is > there solely to tell the compiler what memory it is. > I think "X" constraint is good for that if you aren't planning to actually > use the address anywhere. E.g. you call in inline-asm some function, > but the address construction is in the callee, there is no point to costly > compute the address in the caller (say for -fPIC). If we want to replace the address with a scratch, I think we should do it at expand time so that all unneeded dependent code gets removed, rather than doing it only if the address isn't valid after optimisation. But since there's not AFAIK ever been a rule that "X" operands can't be printed, I think we should only do that if the operand isn't mentioned in the asm string. So would that be OK as a compromise? At expand time, check which operands are used (via get_referenced_operands). If an operand isn't used, is a MEM, has a constraint string that is exactly "X", and is not tied to other operands via matching constraints, replace the MEM address with (scratch). Then allow (mem (scratch)) as well as CONSTANT_P and general_operand in check_asm_operands? I suppose a follow-on optimisation would be to convert "m" into "X" in the same situation. Thanks, Richard
On Wed, Apr 16, 2014 at 02:24:06PM +0100, Richard Sandiford wrote: > > side-effect of inline-asm on certain location in memory, but don't really > > need the address of that memory. Often "memory" is too big hammer, > > people often say that certain inline-asm uses or sets or uses/sets or > > clobbers say 100 byte long piece of memory somewhere, but the operand is > > there solely to tell the compiler what memory it is. > > I think "X" constraint is good for that if you aren't planning to actually > > use the address anywhere. E.g. you call in inline-asm some function, > > but the address construction is in the callee, there is no point to costly > > compute the address in the caller (say for -fPIC). > > If we want to replace the address with a scratch, I think we should do > it at expand time so that all unneeded dependent code gets removed, > rather than doing it only if the address isn't valid after optimisation. > But since there's not AFAIK ever been a rule that "X" operands can't be > printed, I think we should only do that if the operand isn't mentioned > in the asm string. > > So would that be OK as a compromise? At expand time, check which > operands are used (via get_referenced_operands). If an operand isn't > used, is a MEM, has a constraint string that is exactly "X", and is > not tied to other operands via matching constraints, replace the MEM > address with (scratch). Then allow (mem (scratch)) as well as > CONSTANT_P and general_operand in check_asm_operands? > > I suppose a follow-on optimisation would be to convert "m" into "X" > in the same situation. Creating a (mem (scratch)) too early may pessimize code too much, perhaps it can be used during say sched1 etc. for alias analysis, (mem (scratch)) is considered to alias everything,. Plus, I think at least so far we have not been doing different decisions based on whether some operand has been referenced in the template or not, not sure if it is desirable to introduce it. Anyway, others can have different opinion on what "X" should mean, CCing Jeff and Eric. Jakub
> Anyway, others can have different opinion on what "X" should mean, > CCing Jeff and Eric. I personally think that we should not change it and adjust LRA instead to error out instead of ICEing (even if this means erroring out in a few more cases with LRA than with reload for now, e.g. gcc.dg/torture/asm-subreg-1.c which looks somewhat dubious to me).
Eric Botcazou <ebotcazou@adacore.com> writes: >> Anyway, others can have different opinion on what "X" should mean, >> CCing Jeff and Eric. > > I personally think that we should not change it and adjust LRA instead to > error out instead of ICEing (even if this means erroring out in a few more > cases with LRA than with reload for now, e.g. gcc.dg/torture/asm-subreg-1.c > which looks somewhat dubious to me). But the error isn't really under user control. The original asm in the testcase is: __asm__ __volatile__ ("..." : : "X" (ptr)); which seems fine, rather than: __asm__ __volatile__ ("..." : : "X" (*x + y * z / 11)); I don't think it's the user's fault that the compiler decided to convert the former to the latter. Thanks, Richard
On 04/16/14 07:37, Jakub Jelinek wrote: > > Creating a (mem (scratch)) too early may pessimize code too much, > perhaps it can be used during say sched1 etc. for alias analysis, (mem > (scratch)) is considered to alias everything,. > Plus, I think at least so far we have not been doing different decisions > based on whether some operand has been referenced in the template or not, > not sure if it is desirable to introduce it. > > Anyway, others can have different opinion on what "X" should mean, > CCing Jeff and Eric. My recollection is that "X" was supposed to be used in cases where we conditionally needed a scratch operand. The "X" constraint was used to identify alternatives where the scratch operand wasn't actually needed. Conceptually that meant that literally anything could go in there, it need not be valid or reloadable. I'm a bit surprised to see it showing up outside MD files. jeff
Jeff Law <law@redhat.com> writes: > On 04/16/14 07:37, Jakub Jelinek wrote: >> >> Creating a (mem (scratch)) too early may pessimize code too much, >> perhaps it can be used during say sched1 etc. for alias analysis, (mem >> (scratch)) is considered to alias everything,. >> Plus, I think at least so far we have not been doing different decisions >> based on whether some operand has been referenced in the template or not, >> not sure if it is desirable to introduce it. >> >> Anyway, others can have different opinion on what "X" should mean, >> CCing Jeff and Eric. > My recollection is that "X" was supposed to be used in cases where we > conditionally needed a scratch operand. The "X" constraint was used to > identify alternatives where the scratch operand wasn't actually needed. > > Conceptually that meant that literally anything could go in there, it > need not be valid or reloadable. My understanding was that this use of "X" was usually done using scratch_operands, usually indirectly via match_scratch. And scratch_operand only accepts (scratch) and (reg ...): int scratch_operand (rtx op, enum machine_mode mode) { if (GET_MODE (op) != mode && mode != VOIDmode) return 0; return (GET_CODE (op) == SCRATCH || (REG_P (op) && (lra_in_progress || REGNO (op) < FIRST_PSEUDO_REGISTER))); } That reg could be replaced by a MEM or equivalent constant during reloading, but I don't think it's fully general. You wouldn't get the kind of nested MEMs and division operations seen in the testcase. > I'm a bit surprised to see it showing up outside MD files. Yeah, but it was documented in the asm section too, as "Any operand whatsoever is allowed." I'm not sure where to go from here. It sounds like there are three separate suggestions: 1. what the patch did 2. continue to allow the rtl optimisers to do any propagation, etc. If this causes the operand to become invalid, report an error to the user. My objection to this is that the user can't really control what propagation the compiler does. If the operand in the source asm was correct then IMO the compiler should make sure it stays correct. 3. continue to allow the rtl optimisers to do any propagation, etc. If this causes the operand to become invalid, replace parts of it with a scratch. This is not something we do for any other operand and might cause confusion for matching operands. It also means that the operand can't be printed. I assume that if we end up with nested MEMs, we would only want to replace the innermost addresses with scratches, since otherwise we'd be removing a memory read. So even with the scratches, we could end up with some odd-looking rtl. As evidence that "X" operands are being printed, a version of linux's arch/s390/include/asm/jump_label.h that I have lying around has: static __always_inline bool arch_static_branch(struct static_key *key) { asm goto("0: brcl 0,0\n" ".pushsection __jump_table, \"aw\"\n" ASM_ALIGN "\n" ASM_PTR " 0b, %l[label], %0\n" ".popsection\n" : : "X" (key) : : label); return false; label: return true; } This one wouldn't be affected by (3), but it does seem dangerous to assume that noone cares whether "X" operands are printable. Thanks, Richard
Index: gcc/recog.c =================================================================== --- gcc/recog.c 2014-04-12 22:43:54.729854903 +0100 +++ gcc/recog.c 2014-04-15 21:47:32.139873570 +0100 @@ -1840,7 +1840,17 @@ asm_operand_ok (rtx op, const char *cons break; case 'X': - result = 1; + /* Although the asm itself doesn't impose any restrictions on + the operand, we still need to restrict it to something that + can be reloaded and printed. + + MEM operands are always reloaded to make them legitimate, + regardless of the constraint, so we need to handle them + in the same way as for 'm' and 'g'. Since 'X' is not treated + as an address constraint, the only other valid operand types + are constants and registers. */ + result = (CONSTANT_P (op) + || general_operand (op, VOIDmode)); break; case 'g': Index: gcc/testsuite/gcc.dg/torture/asm-x-constraint-1.c =================================================================== --- /dev/null 2014-04-15 08:10:27.294524132 +0100 +++ gcc/testsuite/gcc.dg/torture/asm-x-constraint-1.c 2014-04-15 19:11:29.830962008 +0100 @@ -0,0 +1,27 @@ +void +noprop1 (int **x, int y, int z) +{ + int *ptr = *x + y * z / 11; + __asm__ __volatile__ ("noprop1 %0" : : "X" (*ptr)); +} + +void +noprop2 (int **x, int y, int z) +{ + int *ptr = *x + y * z / 11; + __asm__ __volatile__ ("noprop2 %0" : : "X" (ptr)); +} + +int *global_var; + +void +const1 (void) +{ + __asm__ __volatile__ ("const1 %0" : : "X" (global_var)); +} + +void +const2 (void) +{ + __asm__ __volatile__ ("const2 %0" : : "X" (*global_var)); +}
As Robert pointed out here: http://gcc.gnu.org/ml/gcc-patches/2014-04/msg00416.html we're a bit too eager when folding stuff into an 'X' constraint. The value at expand time is sensible, but after that asm_operand_ok allows arbitrary rtx expressions, including any number of registers as well as MEMs with unchecked addresses. This is a target-independent problem, as shown by the testcase below. Reload would give bogus "impossible constraint in asm" errors while LRA ICEs. Tested on x86_64-linux-gnu. OK to install? Thanks, Richard gcc/ * recog.c (asm_operand_ok): Tighten MEM validity for 'X'. gcc/testsuite/ * gcc.dg/torture/asm-x-constraint-1.c: New test.