| Message ID | AM4PR0701MB2162A54B3CAFABC7BE6FD859E4290@AM4PR0701MB2162.eurprd07.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
On 06/19/2016 07:25 AM, Bernd Edlinger wrote: > Hi, > > ping... > > As this discussion did not make any progress, I just attached > the latest version of my patch with the the changes that > Vladimir proposed. > > Boot-strapped and reg-tested again on x86_64-linux-gnu. > Is it OK for the trunk? Well, I don't think we've got any kind of consensus on whether or not this is reasonable or not. The fundamental issue is that "X" is supposed to accept anything, literally anything. That implies it's really the downstream users of those operands that are broken. Jeff
On 06/22/16 21:51, Jeff Law wrote: > On 06/19/2016 07:25 AM, Bernd Edlinger wrote: >> Hi, >> >> ping... >> >> As this discussion did not make any progress, I just attached >> the latest version of my patch with the the changes that >> Vladimir proposed. >> >> Boot-strapped and reg-tested again on x86_64-linux-gnu. >> Is it OK for the trunk? > Well, I don't think we've got any kind of consensus on whether or not > this is reasonable or not. > > The fundamental issue is that "X" is supposed to accept anything, > literally anything. That implies it's really the downstream users of > those operands that are broken. > Hmm... I think it must be pretty easy to write something in a .md file with the X constraint that ends up in an ICE, right? But in an .md file we have much more control on what happens. That's why I did not propose to change the meaning of "X" in .md files. And we only have problems with asm statements that use "X" constraints. Nobody has any use case where the really anything kind of RTL operand is actually useful in a user-written assembler statement. Please correct me if that is wrong. But I think we have a use case where "X" means really more possible registers (i.e. includes ss2, mmx etc.) than "g" (only general registers). Otherwise, in the test cases of pr59155 we would not have any benefit for using "+X" instead of "+g" or "+r". Does that sound reasonable? Bernd.
On 06/22/2016 02:48 PM, Bernd Edlinger wrote: > On 06/22/16 21:51, Jeff Law wrote: >> On 06/19/2016 07:25 AM, Bernd Edlinger wrote: >>> Hi, >>> >>> ping... >>> >>> As this discussion did not make any progress, I just attached >>> the latest version of my patch with the the changes that >>> Vladimir proposed. >>> >>> Boot-strapped and reg-tested again on x86_64-linux-gnu. >>> Is it OK for the trunk? >> Well, I don't think we've got any kind of consensus on whether or not >> this is reasonable or not. >> >> The fundamental issue is that "X" is supposed to accept anything, >> literally anything. That implies it's really the downstream users of >> those operands that are broken. >> > > Hmm... > > I think it must be pretty easy to write something in a .md file with the > X constraint that ends up in an ICE, right? Probably not terribly hard. > > But in an .md file we have much more control on what happens. > That's why I did not propose to change the meaning of "X" in .md files. We have control over RTL generation, operand predicates and the like. And those are how we control things like combine. > > And we only have problems with asm statements that use "X" constraints. But I'd disagree. I think we could easily have problems with "X" constraints in the MD file. But the most common uses of "X" probably don't try to refer to that operand in the output string and use good predicates. And that's one of the key differences here. In an MD file the operand predicate has to pass -- that's not the case in an ASM. The operand predicate allows the backend to prevent all kinds of things from showing up. > > But I think we have a use case where "X" means really more possible > registers (i.e. includes ss2, mmx etc.) than "g" (only general > registers). Otherwise, in the test cases of pr59155 we would not > have any benefit for using "+X" instead of "+g" or "+r". > > Does that sound reasonable? If it's the case that the real benefit of +X is that it's allowing more registers, then that argues that the backend ought to be providing another (larger) register class. jeff
On 07/20/16 22:04, Jeff Law wrote: > On 06/22/2016 02:48 PM, Bernd Edlinger wrote: >> On 06/22/16 21:51, Jeff Law wrote: >>> On 06/19/2016 07:25 AM, Bernd Edlinger wrote: >>>> Hi, >>>> >>>> ping... >>>> >>>> As this discussion did not make any progress, I just attached >>>> the latest version of my patch with the the changes that >>>> Vladimir proposed. >>>> >>>> Boot-strapped and reg-tested again on x86_64-linux-gnu. >>>> Is it OK for the trunk? >>> Well, I don't think we've got any kind of consensus on whether or not >>> this is reasonable or not. >>> >>> The fundamental issue is that "X" is supposed to accept anything, >>> literally anything. That implies it's really the downstream users of >>> those operands that are broken. >>> >> >> Hmm... >> >> I think it must be pretty easy to write something in a .md file with the >> X constraint that ends up in an ICE, right? > Probably not terribly hard. > >> >> But in an .md file we have much more control on what happens. >> That's why I did not propose to change the meaning of "X" in .md files. > We have control over RTL generation, operand predicates and the like. > And those are how we control things like combine. > >> >> And we only have problems with asm statements that use "X" constraints. > But I'd disagree. I think we could easily have problems with "X" > constraints in the MD file. But the most common uses of "X" probably > don't try to refer to that operand in the output string and use good > predicates. > > And that's one of the key differences here. In an MD file the operand > predicate has to pass -- that's not the case in an ASM. The operand > predicate allows the backend to prevent all kinds of things from showing > up. > >> >> But I think we have a use case where "X" means really more possible >> registers (i.e. includes ss2, mmx etc.) than "g" (only general >> registers). Otherwise, in the test cases of pr59155 we would not >> have any benefit for using "+X" instead of "+g" or "+r". >> >> Does that sound reasonable? > If it's the case that the real benefit of +X is that it's allowing more > registers, then that argues that the backend ought to be providing > another (larger) register class. > X allows more different registers than r, and it is already documented. In the cases where it is already used, the patch should not break anything. I would not understand, why we should forbid the use of X and waste another letter of the alphabet for a slightly modified version of X. Bernd.
On 07/21/2016 10:29 AM, Bernd Edlinger wrote: >>> But I think we have a use case where "X" means really more possible >>> registers (i.e. includes ss2, mmx etc.) than "g" (only general >>> registers). Otherwise, in the test cases of pr59155 we would not >>> have any benefit for using "+X" instead of "+g" or "+r". >>> >>> Does that sound reasonable? >> If it's the case that the real benefit of +X is that it's allowing more >> registers, then that argues that the backend ought to be providing >> another (larger) register class. >> > > X allows more different registers than r, and it is already documented. > In the cases where it is already used, the patch should not break > anything. I would not understand, why we should forbid the use of X and > waste another letter of the alphabet for a slightly modified version > of X. Doing so essentially allows us to deprecate "X" to used by target patterns only -- where what's acceptable is limited by the operand predicates. Those limits ultimately protect the rest of the routines from having to handle arbitrary RTL. Meanwhile asms can use the new letter to say "I'll take any register of any class". Which is, AFAICT, what's desired here. jeff
On 08/04/16 22:27, Jeff Law wrote: > On 07/21/2016 10:29 AM, Bernd Edlinger wrote: >>>> But I think we have a use case where "X" means really more possible >>>> registers (i.e. includes ss2, mmx etc.) than "g" (only general >>>> registers). Otherwise, in the test cases of pr59155 we would not >>>> have any benefit for using "+X" instead of "+g" or "+r". >>>> >>>> Does that sound reasonable? >>> If it's the case that the real benefit of +X is that it's allowing more >>> registers, then that argues that the backend ought to be providing >>> another (larger) register class. >>> >> >> X allows more different registers than r, and it is already documented. >> In the cases where it is already used, the patch should not break >> anything. I would not understand, why we should forbid the use of X and >> waste another letter of the alphabet for a slightly modified version >> of X. > Doing so essentially allows us to deprecate "X" to used by target > patterns only -- where what's acceptable is limited by the operand > predicates. Those limits ultimately protect the rest of the routines > from having to handle arbitrary RTL. > > Meanwhile asms can use the new letter to say "I'll take any register of > any class". Which is, AFAICT, what's desired here. > > jeff Yes. To be useful it should be a target independent letter. While "g" implies a general register of class GENERAL_REGS "X" implies any register of class ALL_REGS. I have looked for uses of "X" and actually found some of them in glibc: ./sysdeps/powerpc/powerpc64/dl-machine.h: /* GCC 4.9+ eliminates the branch as dead code, force the odp set dependency. */ asm ("" : "=r" (value) : "0" (&opd), "X" (opd)); ./sysdeps/mach/hurd/i386/init-first.c: *--newsp = *((int *) __builtin_frame_address (0) + 1); /* GCC 4.4.6 also wants us to force loading *NEWSP already here. */ asm volatile ("# %0" : : "X" (*newsp)); and same file: usercode = *((int *) __builtin_frame_address (0) + 1); /* GCC 4.4.6 also wants us to force loading USERCODE already here. */ asm volatile ("# %0" : : "X" (usercode)); So in is mostly used for obfuscating the data flow. The documentation of "g" at md.texi says @cindex @samp{g} in constraint @item @samp{g} Any register, memory or immediate integer operand is allowed, except for registers that are not general registers. and "X" says: @ifset INTERNALS Any operand whatsoever is allowed, even if it does not satisfy @code{general_operand}. This is normally used in the constraint of a @code{match_scratch} when certain alternatives will not actually require a scratch register. @end ifset @ifclear INTERNALS Any operand whatsoever is allowed. @end ifclear The part ifset INTERNALS describes the rules for target patterns, while the ifclear INTERNALS part describes the rules for asms. This is exactly what we want. Not saying "except for registers that are not general registers" is a hint that there are more registers in the ALL_REGS class. We could make it more explicit by adding "Including registers that are not general registers". And "whatsoever" means anything you can write down at the source code level IMO. So I think restricting "X" in asms to this definition while keeping the current meaning of "X" in target patterns is consistent with the current documentation and compatible to the current uses of the "X" constraint elsewhere. Bernd.
Index: gcc/lra-constraints.c =================================================================== --- gcc/lra-constraints.c (revision 237133) +++ gcc/lra-constraints.c (working copy) @@ -1854,8 +1854,9 @@ process_alt_operands (int only_alternative) if (curr_static_id->operand_alternative[opalt_num].anything_ok) { /* Fast track for no constraints at all. */ - curr_alt[nop] = NO_REGS; - CLEAR_HARD_REG_SET (curr_alt_set[nop]); + curr_alt[nop] = ALL_REGS; + COPY_HARD_REG_SET (curr_alt_set[nop], + reg_class_contents[ALL_REGS]); curr_alt_win[nop] = true; curr_alt_match_win[nop] = false; curr_alt_offmemok[nop] = false; Index: gcc/recog.c =================================================================== --- gcc/recog.c (revision 237133) +++ gcc/recog.c (working copy) @@ -1778,6 +1778,10 @@ asm_operand_ok (rtx op, const char *constraint, co result = 1; break; + case 'X': + if (scratch_operand (op, VOIDmode)) + result = 1; + /* ... fall through ... */ case 'g': if (general_operand (op, VOIDmode)) result = 1; Index: gcc/testsuite/gcc.dg/torture/pr59155-1.c =================================================================== --- gcc/testsuite/gcc.dg/torture/pr59155-1.c (revision 0) +++ gcc/testsuite/gcc.dg/torture/pr59155-1.c (working copy) @@ -0,0 +1,8 @@ +/* { dg-do compile } */ +double f(double x){ + asm volatile("":"+X"(x)); + return x; +} +double g(double x,double y){ + return f(f(x)+f(y)); +} Index: gcc/testsuite/gcc.dg/torture/pr59155-2.c =================================================================== --- gcc/testsuite/gcc.dg/torture/pr59155-2.c (revision 0) +++ gcc/testsuite/gcc.dg/torture/pr59155-2.c (working copy) @@ -0,0 +1,8 @@ +/* { dg-do compile } */ +double f(double x){ + asm volatile("":"+X"(x)); + return x; +} +double g(){ + return f(1.); +} Index: gcc/testsuite/gcc.dg/torture/pr59155-3.c =================================================================== --- gcc/testsuite/gcc.dg/torture/pr59155-3.c (revision 0) +++ gcc/testsuite/gcc.dg/torture/pr59155-3.c (working copy) @@ -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)); +}