[PATCH] Fix asm X constraint (PR inline-asm/59155)

[PATCH] Fix asm X constraint (PR inline-asm/59155)

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 657 bytes --]

Hi!

This restricts the X constraint in asm statements, which
can be easily folded by combine in something completely
invalid.

It is necessary to allow scratch here, because on i386
the md_asm_adjust hook inserts them.

The second test case fails because lra does not
allow all register for anything_ok operands (aka X)
and later it fails to match the two X constraints
in case '0': if (curr_alt[m] == NO_REGS) break.

There is also an identical bug in the reload pass,
but I do not know how to fix that, as it is almost
used nowhere today.


Boot-strapped and regression-tested on x86_64-pc-linux-gnu.
OK for trunk?


Thanks
Bernd.

[-- Attachment #2: changelog-pr59155.txt --]
[-- Type: text/plain, Size: 407 bytes --]

gcc:
2016-05-23  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR inline-asm/59155
	* lra-constraints.c (process_alt_operands): Allow ALL_REGS.
	* recog.c (asm_operand_ok): Handle X constraint.

testsuite:
2016-05-23  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR inline-asm/59155
	* gcc.dg/torture/pr59155-1.c: New test.
	* gcc.dg/torture/pr59155-2.c: New test.
	* gcc.dg/torture/pr59155-3.c: New test.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: patch-pr59155.diff --]
[-- Type: text/x-patch; name="patch-pr59155.diff", Size: 2398 bytes --]

Index: gcc/lra-constraints.c
===================================================================
--- gcc/lra-constraints.c	(revision 236569)
+++ gcc/lra-constraints.c	(working copy)
@@ -1854,7 +1854,7 @@ 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;
+	      curr_alt[nop] = ALL_REGS;
 	      CLEAR_HARD_REG_SET (curr_alt_set[nop]);
 	      curr_alt_win[nop] = true;
 	      curr_alt_match_win[nop] = false;
Index: gcc/recog.c
===================================================================
--- gcc/recog.c	(revision 236569)
+++ gcc/recog.c	(working copy)
@@ -1757,6 +1757,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,7 @@
+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,7 @@
+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));
+}

[PING] [PATCH] Fix asm X constraint (PR inline-asm/59155)

[Bernd Edlinger]

Ping...

see https://gcc.gnu.org/ml/gcc-patches/2016-05/msg02010.html

Thanks
Bernd.


On 05/25/16 14:58, Bernd Edlinger wrote:
> Hi!
>
> This restricts the X constraint in asm statements, which
> can be easily folded by combine in something completely
> invalid.
>
> It is necessary to allow scratch here, because on i386
> the md_asm_adjust hook inserts them.
>
> The second test case fails because lra does not
> allow all register for anything_ok operands (aka X)
> and later it fails to match the two X constraints
> in case '0': if (curr_alt[m] == NO_REGS) break.
>
> There is also an identical bug in the reload pass,
> but I do not know how to fix that, as it is almost
> used nowhere today.
>
>
> Boot-strapped and regression-tested on x86_64-pc-linux-gnu.
> OK for trunk?
>
>
> Thanks
> Bernd.
>

Re: [PING] [PATCH] Fix asm X constraint (PR inline-asm/59155)

[Vladimir Makarov]

On 06/06/2016 09:32 AM, Bernd Edlinger wrote:
> Ping...
>
> see https://gcc.gnu.org/ml/gcc-patches/2016-05/msg02010.html
>
>
Thank you for working on the PR and sorry for the delay with LRA part of 
review.

Change in lra-constraints.c is ok for me with the following change. 
Instead of just

-	      curr_alt[nop] = NO_REGS;
+	      curr_alt[nop] = ALL_REGS;
  	      CLEAR_HARD_REG_SET (curr_alt_set[nop]);

I'd like to see

-	      curr_alt[nop] = NO_REGS;
+	      curr_alt[nop] = ALL_REGS;
- 	      CLEAR_HARD_REG_SET (curr_alt_set[nop]);
+             COPY_HARD_REG_SET (curr_alt_set[nop], reg_class_contents[ALL_REGS]);

Also I don't see /* { dg-do compile } */ in the tests (I don't know what dejagnu does when there is no any dejagnu actions in the test).
But with the addition '/* { dg-do compile } */' the test pr59155-2.c is ok for me too.

As for recog.c, I can not approve this as I am not a maintainer of it.
I only can say that the code looks questionable to me.

Re: [PING] [PATCH] Fix asm X constraint (PR inline-asm/59155)

[Jeff Law]

On 06/06/2016 11:04 AM, Vladimir Makarov wrote:
> On 06/06/2016 09:32 AM, Bernd Edlinger wrote:
>> Ping...
>>
>> see https://gcc.gnu.org/ml/gcc-patches/2016-05/msg02010.html
>>
>>
> Thank you for working on the PR and sorry for the delay with LRA part of
> review.
>
> Change in lra-constraints.c is ok for me with the following change.
> Instead of just
>
> -          curr_alt[nop] = NO_REGS;
> +          curr_alt[nop] = ALL_REGS;
>            CLEAR_HARD_REG_SET (curr_alt_set[nop]);
>
> I'd like to see
>
> -          curr_alt[nop] = NO_REGS;
> +          curr_alt[nop] = ALL_REGS;
> -           CLEAR_HARD_REG_SET (curr_alt_set[nop]);
> +             COPY_HARD_REG_SET (curr_alt_set[nop],
> reg_class_contents[ALL_REGS]);
>
> Also I don't see /* { dg-do compile } */ in the tests (I don't know what
> dejagnu does when there is no any dejagnu actions in the test).
> But with the addition '/* { dg-do compile } */' the test pr59155-2.c is
> ok for me too.
>
> As for recog.c, I can not approve this as I am not a maintainer of it.
> I only can say that the code looks questionable to me.
I think the question on the recog part is a matter of how we choose to 
interpret what the "X" constraint means.

Does it literally mean accept anything, or accept some subset expressions?

I tend to think the former, which means that things like 
reg_overlap_mentioned_p or its callers have to be bullet-proofed.

jeff

Re: [PING] [PATCH] Fix asm X constraint (PR inline-asm/59155)

[Jakub Jelinek]

On Mon, Jun 06, 2016 at 11:54:04AM -0600, Jeff Law wrote:
> >As for recog.c, I can not approve this as I am not a maintainer of it.
> >I only can say that the code looks questionable to me.
> I think the question on the recog part is a matter of how we choose to
> interpret what the "X" constraint means.
> 
> Does it literally mean accept anything, or accept some subset expressions?
> 
> I tend to think the former, which means that things like
> reg_overlap_mentioned_p or its callers have to be bullet-proofed.

I think it is a bad idea to accept really anything, even for debug insns,
which initially accepted arbitrarily large RTL expressions (and still accept
stuff like subregs otherwise considered invalid etc.) we found it is highly
undesirable, as it is not very good idea for the compile time complexity
etc., so now we are trying to limit the complexity of the expressions there
by splitting up more complex expressions into smaller ones using temporaries.
So, even accept anything should always be accept anything reasonable,
because most of the RTL passes don't really expect arbitrarily deep
expressions, or expressions where the same reg can appear thousands of times
etc.

	Jakub

Re: [PING] [PATCH] Fix asm X constraint (PR inline-asm/59155)

[Jeff Law]

On 06/06/2016 12:01 PM, Jakub Jelinek wrote:
> On Mon, Jun 06, 2016 at 11:54:04AM -0600, Jeff Law wrote:
>>> As for recog.c, I can not approve this as I am not a maintainer of it.
>>> I only can say that the code looks questionable to me.
>> I think the question on the recog part is a matter of how we choose to
>> interpret what the "X" constraint means.
>>
>> Does it literally mean accept anything, or accept some subset expressions?
>>
>> I tend to think the former, which means that things like
>> reg_overlap_mentioned_p or its callers have to be bullet-proofed.
>
> I think it is a bad idea to accept really anything, even for debug insns,
> which initially accepted arbitrarily large RTL expressions (and still accept
> stuff like subregs otherwise considered invalid etc.) we found it is highly
> undesirable, as it is not very good idea for the compile time complexity
> etc., so now we are trying to limit the complexity of the expressions there
> by splitting up more complex expressions into smaller ones using temporaries.
> So, even accept anything should always be accept anything reasonable,
> because most of the RTL passes don't really expect arbitrarily deep
> expressions, or expressions where the same reg can appear thousands of times
> etc.
The problem is how do you define this subset of expressions you're going 
to accept and those which you are going to reject.

I first pondered accepting RTL leaf nodes (reg, subreg, constants) and 
rejecting everything else.  But I couldn't convince myself that some 
port might reasonably expect (plus (x) (y)) to match the "X" constraint.

jeff

Re: [PING] [PATCH] Fix asm X constraint (PR inline-asm/59155)

[Jakub Jelinek]

On Mon, Jun 06, 2016 at 12:04:04PM -0600, Jeff Law wrote:
> On 06/06/2016 12:01 PM, Jakub Jelinek wrote:
> >On Mon, Jun 06, 2016 at 11:54:04AM -0600, Jeff Law wrote:
> >>>As for recog.c, I can not approve this as I am not a maintainer of it.
> >>>I only can say that the code looks questionable to me.
> >>I think the question on the recog part is a matter of how we choose to
> >>interpret what the "X" constraint means.
> >>
> >>Does it literally mean accept anything, or accept some subset expressions?
> >>
> >>I tend to think the former, which means that things like
> >>reg_overlap_mentioned_p or its callers have to be bullet-proofed.
> >
> >I think it is a bad idea to accept really anything, even for debug insns,
> >which initially accepted arbitrarily large RTL expressions (and still accept
> >stuff like subregs otherwise considered invalid etc.) we found it is highly
> >undesirable, as it is not very good idea for the compile time complexity
> >etc., so now we are trying to limit the complexity of the expressions there
> >by splitting up more complex expressions into smaller ones using temporaries.
> >So, even accept anything should always be accept anything reasonable,
> >because most of the RTL passes don't really expect arbitrarily deep
> >expressions, or expressions where the same reg can appear thousands of times
> >etc.
> The problem is how do you define this subset of expressions you're going to
> accept and those which you are going to reject.
> 
> I first pondered accepting RTL leaf nodes (reg, subreg, constants) and
> rejecting everything else.  But I couldn't convince myself that some port
> might reasonably expect (plus (x) (y)) to match the "X" constraint.

It is always going to be arbitrary.
Perhaps RTL leaf nodes (if including MEM, then perhaps with valid address
only), and unary/binary/ternary RTL expressions with RTL leaf node operands?
Or union of what is accepted by any other constraint?
Or "g" plus any constants?

	Jakub

Re: [PING] [PATCH] Fix asm X constraint (PR inline-asm/59155)

[Marc Glisse]

On Mon, 6 Jun 2016, Jakub Jelinek wrote:

> On Mon, Jun 06, 2016 at 12:04:04PM -0600, Jeff Law wrote:
>> On 06/06/2016 12:01 PM, Jakub Jelinek wrote:
>>> On Mon, Jun 06, 2016 at 11:54:04AM -0600, Jeff Law wrote:
>>>>> As for recog.c, I can not approve this as I am not a maintainer of it.
>>>>> I only can say that the code looks questionable to me.
>>>> I think the question on the recog part is a matter of how we choose to
>>>> interpret what the "X" constraint means.
>>>>
>>>> Does it literally mean accept anything, or accept some subset expressions?
>>>>
>>>> I tend to think the former, which means that things like
>>>> reg_overlap_mentioned_p or its callers have to be bullet-proofed.
>>>
>>> I think it is a bad idea to accept really anything, even for debug insns,
>>> which initially accepted arbitrarily large RTL expressions (and still accept
>>> stuff like subregs otherwise considered invalid etc.) we found it is highly
>>> undesirable, as it is not very good idea for the compile time complexity
>>> etc., so now we are trying to limit the complexity of the expressions there
>>> by splitting up more complex expressions into smaller ones using temporaries.
>>> So, even accept anything should always be accept anything reasonable,
>>> because most of the RTL passes don't really expect arbitrarily deep
>>> expressions, or expressions where the same reg can appear thousands of times
>>> etc.
>> The problem is how do you define this subset of expressions you're going to
>> accept and those which you are going to reject.
>>
>> I first pondered accepting RTL leaf nodes (reg, subreg, constants) and
>> rejecting everything else.  But I couldn't convince myself that some port
>> might reasonably expect (plus (x) (y)) to match the "X" constraint.
>
> It is always going to be arbitrary.
> Perhaps RTL leaf nodes (if including MEM, then perhaps with valid address
> only), and unary/binary/ternary RTL expressions with RTL leaf node operands?
> Or union of what is accepted by any other constraint?
> Or "g" plus any constants?

The last one would miss floating point registers (no 2 platforms use the 
same letter for those, hence my quest for something more generic).

The goal of the experiment is described in PR59159 (for which "+X" is 
unlikely to be the right answer, in particular because it is meaningless 
for constants). I don't know in what context people use the "X" 
constraint, or even better "=X"...

-- 
Marc Glisse

Re: [PING] [PATCH] Fix asm X constraint (PR inline-asm/59155)

[Jakub Jelinek]

On Mon, Jun 06, 2016 at 09:27:56PM +0200, Marc Glisse wrote:
> The last one would miss floating point registers (no 2 platforms use the
> same letter for those, hence my quest for something more generic).
> 
> The goal of the experiment is described in PR59159 (for which "+X" is
> unlikely to be the right answer, in particular because it is meaningless for
> constants). I don't know in what context people use the "X" constraint, or
> even better "=X"...

X constraint has been added mainly for uses in match_scratch like:
(clobber (match_scratch:SI 2 "=X,X,X,&r"))
or when the predicate takes care of everything and it is not needed to
specify anything further:
  [(set (match_operand:SWI12 0 "push_operand" "=X")
        (match_operand:SWI12 1 "nonmemory_no_elim_operand" "rn"))]
Using it in inline asm generally has resulted in lots of issues, including
ICEs etc., so nothing I'd recommend to use.

	Jakub

Re: [PING] [PATCH] Fix asm X constraint (PR inline-asm/59155)

[Bernd Edlinger]

On 06/06/16 20:08, Jakub Jelinek wrote:
> On Mon, Jun 06, 2016 at 12:04:04PM -0600, Jeff Law wrote:
>> On 06/06/2016 12:01 PM, Jakub Jelinek wrote:
>>> On Mon, Jun 06, 2016 at 11:54:04AM -0600, Jeff Law wrote:
>>>>> As for recog.c, I can not approve this as I am not a maintainer of it.
>>>>> I only can say that the code looks questionable to me.
>>>> I think the question on the recog part is a matter of how we choose to
>>>> interpret what the "X" constraint means.
>>>>
>>>> Does it literally mean accept anything, or accept some subset expressions?
>>>>
>>>> I tend to think the former, which means that things like
>>>> reg_overlap_mentioned_p or its callers have to be bullet-proofed.

AFACT this is not the only place where overly complex RTL trees can
cause an ICE.

>>>
>>> I think it is a bad idea to accept really anything, even for debug insns,
>>> which initially accepted arbitrarily large RTL expressions (and still accept
>>> stuff like subregs otherwise considered invalid etc.) we found it is highly
>>> undesirable, as it is not very good idea for the compile time complexity
>>> etc., so now we are trying to limit the complexity of the expressions there
>>> by splitting up more complex expressions into smaller ones using temporaries.
>>> So, even accept anything should always be accept anything reasonable,
>>> because most of the RTL passes don't really expect arbitrarily deep
>>> expressions, or expressions where the same reg can appear thousands of times
>>> etc.
>> The problem is how do you define this subset of expressions you're going to
>> accept and those which you are going to reject.
>>
>> I first pondered accepting RTL leaf nodes (reg, subreg, constants) and
>> rejecting everything else.  But I couldn't convince myself that some port
>> might reasonably expect (plus (x) (y)) to match the "X" constraint.
>
> It is always going to be arbitrary.
> Perhaps RTL leaf nodes (if including MEM, then perhaps with valid address
> only), and unary/binary/ternary RTL expressions with RTL leaf node operands?
> Or union of what is accepted by any other constraint?
> Or "g" plus any constants?

Yes. That is what I think too, "X" is probably not used often in
practice, but if it is allowed, it should at least not result in an ICE.

"X" should allow to feed "whatsoever" valid C expressions to the asm
input, and using the %-expression in the assembler string should not
cause an ICE.

And "X" really means different "whatsoever" things in asm statements
and in .md files, even md.texi has different text for internally used
"X" constraint and assembler "X" constraint, so that should be OK.

According to md.texi
"g" should already mean:

@item @samp{g}
Any register, memory or immediate integer operand is allowed, except for
registers that are not general registers.

Which is the same as general_operand(op, VOIDmode). And in other words,
that should be everything that is in "r", "m" and "i" constraints.


However if I compare commond.md ...

(define_constraint "i"
   "Matches a general integer constant."
   (and (match_test "CONSTANT_P (op)")
        (match_test "!flag_pic || LEGITIMATE_PIC_OPERAND_P (op)")))

... against recog.c, general_operand seems to allow less i.e. checks
targetm.legitimate_constant_p additionally.  So that is something
I don't really understand, I mean if a constant is not a "legitimate"
constant by the target's definition, why should it be safe to use
in later in targetm.asm_out.print_operand?



Bernd.

Re: [PING] [PATCH] Fix asm X constraint (PR inline-asm/59155)

[Jeff Law]

On 06/07/2016 11:58 AM, Bernd Edlinger wrote:
>
> AFACT this is not the only place where overly complex RTL trees can
> cause an ICE.
That wouldn't surprise me at all -- but the design of RTL is such that 
it can be arbitrarily complex.  Essentially, routines can not make 
assumptions about the complexity of the RTL they're given -- except for 
any guards/pre-conditions set up in the caller.

>
> Yes. That is what I think too, "X" is probably not used often in
> practice, but if it is allowed, it should at least not result in an ICE.
There's no disagreement about "X" should never result in an ICE.

The question is whether or not "X" truly means an arbitrary piece of RTL 
or some tighter subset of RTL.   I guess one could also raise the 
question of whether or not "X" should ever be exposed to asms.

"X"'s original intent IIRC was to specify that for a particular 
alternative the operand didn't matter and would never be used.  This 
comes up with scratch registers for example.


>
> "X" should allow to feed "whatsoever" valid C expressions to the asm
> input, and using the %-expression in the assembler string should not
> cause an ICE.
Actually it doesn't even have to be a valid C expression.  It might have 
started as a C expression from an ASM statement, but been modified by 
various RTL optimization passes into something that isn't necessarily a 
valid C expression.



> However if I compare commond.md ...
>
> (define_constraint "i"
>    "Matches a general integer constant."
>    (and (match_test "CONSTANT_P (op)")
>         (match_test "!flag_pic || LEGITIMATE_PIC_OPERAND_P (op)")))
>
> ... against recog.c, general_operand seems to allow less i.e. checks
> targetm.legitimate_constant_p additionally.  So that is something
> I don't really understand, I mean if a constant is not a "legitimate"
> constant by the target's definition, why should it be safe to use
> in later in targetm.asm_out.print_operand?
Legitimacy in this context is usually something related to operands that 
have to be reloaded for PIC code generation.  They may print just fine, 
but from a code generation standpoint they require special handling. 
One could argue that this is inconsistent and it ought to be cleaned up, 
but that's separate from the "X" issue.

jeff

Re: [PING] [PATCH] Fix asm X constraint (PR inline-asm/59155)

[Jeff Law]

On 06/06/2016 01:40 PM, Jakub Jelinek wrote:
> On Mon, Jun 06, 2016 at 09:27:56PM +0200, Marc Glisse wrote:
>> The last one would miss floating point registers (no 2 platforms use the
>> same letter for those, hence my quest for something more generic).
>>
>> The goal of the experiment is described in PR59159 (for which "+X" is
>> unlikely to be the right answer, in particular because it is meaningless for
>> constants). I don't know in what context people use the "X" constraint, or
>> even better "=X"...
>
> X constraint has been added mainly for uses in match_scratch like:
> (clobber (match_scratch:SI 2 "=X,X,X,&r"))
> or when the predicate takes care of everything and it is not needed to
> specify anything further:
>   [(set (match_operand:SWI12 0 "push_operand" "=X")
>         (match_operand:SWI12 1 "nonmemory_no_elim_operand" "rn"))]
> Using it in inline asm generally has resulted in lots of issues, including
> ICEs etc., so nothing I'd recommend to use.
So would it make sense to define it as not available for use in ASMs?  I 
realize that's potentially a user-visible change, but it might be a 
reasonable one to make.

jeff

Re: [PING] [PATCH] Fix asm X constraint (PR inline-asm/59155)

[Jakub Jelinek]

On Thu, Jun 09, 2016 at 10:30:13AM -0600, Jeff Law wrote:
> On 06/06/2016 01:40 PM, Jakub Jelinek wrote:
> >On Mon, Jun 06, 2016 at 09:27:56PM +0200, Marc Glisse wrote:
> >>The last one would miss floating point registers (no 2 platforms use the
> >>same letter for those, hence my quest for something more generic).
> >>
> >>The goal of the experiment is described in PR59159 (for which "+X" is
> >>unlikely to be the right answer, in particular because it is meaningless for
> >>constants). I don't know in what context people use the "X" constraint, or
> >>even better "=X"...
> >
> >X constraint has been added mainly for uses in match_scratch like:
> >(clobber (match_scratch:SI 2 "=X,X,X,&r"))
> >or when the predicate takes care of everything and it is not needed to
> >specify anything further:
> >  [(set (match_operand:SWI12 0 "push_operand" "=X")
> >        (match_operand:SWI12 1 "nonmemory_no_elim_operand" "rn"))]
> >Using it in inline asm generally has resulted in lots of issues, including
> >ICEs etc., so nothing I'd recommend to use.
> So would it make sense to define it as not available for use in ASMs?  I
> realize that's potentially a user-visible change, but it might be a
> reasonable one to make.

Yes, I'm all in favor in disabling X constraint for inline asm.
Especially if people actually try to print it as well, rather than make it
unused.  That is a sure path to ICEs.

	Jakub

Re: [PING] [PATCH] Fix asm X constraint (PR inline-asm/59155)

[Jakub Jelinek]

On Thu, Jun 09, 2016 at 06:43:04PM +0200, Jakub Jelinek wrote:
> Yes, I'm all in favor in disabling X constraint for inline asm.
> Especially if people actually try to print it as well, rather than make it
> unused.  That is a sure path to ICEs.

Though, on the other side, even our documentation mentions
asm volatile ("mtfsf 255,%1" : "=X"(sum): "f"(fpenv));
So perhaps we need to error just in case such an argument is printed?

	Jakub

Re: [PING] [PATCH] Fix asm X constraint (PR inline-asm/59155)

[Bernd Edlinger]

On 06/09/16 18:45, Jakub Jelinek wrote:
> On Thu, Jun 09, 2016 at 06:43:04PM +0200, Jakub Jelinek wrote:
>> Yes, I'm all in favor in disabling X constraint for inline asm.
>> Especially if people actually try to print it as well, rather than make it
>> unused.  That is a sure path to ICEs.
>
> Though, on the other side, even our documentation mentions
> asm volatile ("mtfsf 255,%1" : "=X"(sum): "f"(fpenv));
> So perhaps we need to error just in case such an argument is printed?

note that "=X" is also introduced internally in this asm statement:

asm ("cmpl  %2, 0" : "=@ccz"(z), "=@ccb"(b): "r"(i));

see i386.c, ix86_md_asm_adjust.

The first =@cc is replaced by "=Bf" constraint but any
further =@cc are replaced by "=X" and scratch operand.

Printing the "=X" scratch is harmless, but printing the "=Bf" causes
another ICE, I shall submit a follow up patch for that:
asm ("# %0" : "=@ccz"(z));

test.c:6:1: internal compiler error: in print_reg, at 
config/i386/i386.c:17193
  }
  ^
0xedfc30 print_reg(rtx_def*, int, _IO_FILE*)
	../../gcc-trunk/gcc/config/i386/i386.c:17189
0xf048a4 ix86_print_operand(_IO_FILE*, rtx_def*, int)
	../../gcc-trunk/gcc/config/i386/i386.c:17867
0x8bf87c output_operand(rtx_def*, int)
	../../gcc-trunk/gcc/final.c:3847
0x8c00ee output_asm_insn(char const*, rtx_def**)
	../../gcc-trunk/gcc/final.c:3763
0x8c1f9c final_scan_insn(rtx_insn*, _IO_FILE*, int, int, int*)
	../../gcc-trunk/gcc/final.c:2628
0x8c25c9 final(rtx_insn*, _IO_FILE*, int)
	../../gcc-trunk/gcc/final.c:2045
0x8c2da9 rest_of_handle_final
	../../gcc-trunk/gcc/final.c:4445
0x8c2da9 execute
	../../gcc-trunk/gcc/final.c:4520


Well, regarding the X constraint, I do think that
it's definitely OK to use different rules if it is
used in asms vs. when if it is used internally in .md files.

The patch handles X in asms to be just a synonym to the g constraint,
except that g allows only GENERAL_REGS and X allows ALL_REGS.

What I am not sure of, is if X should allow more than g in terms of
CONSTANT_P.  I think it should not, because probably the CONSTANT_P
handling in general_operand is likely smarter than that of the i
constraint.


Bernd.

[PING**2] [PATCH] Fix asm X constraint (PR inline-asm/59155)

[Bernd Edlinger]

[-- Attachment #1: Type: text/plain, Size: 2650 bytes --]

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?


Thanks
Bernd.


On 06/10/16 16:13, Bernd Edlinger wrote:
> On 06/09/16 18:45, Jakub Jelinek wrote:
>> On Thu, Jun 09, 2016 at 06:43:04PM +0200, Jakub Jelinek wrote:
>>> Yes, I'm all in favor in disabling X constraint for inline asm.
>>> Especially if people actually try to print it as well, rather than
>>> make it
>>> unused.  That is a sure path to ICEs.
>>
>> Though, on the other side, even our documentation mentions
>> asm volatile ("mtfsf 255,%1" : "=X"(sum): "f"(fpenv));
>> So perhaps we need to error just in case such an argument is printed?
>
> note that "=X" is also introduced internally in this asm statement:
>
> asm ("cmpl  %2, 0" : "=@ccz"(z), "=@ccb"(b): "r"(i));
>
> see i386.c, ix86_md_asm_adjust.
>
> The first =@cc is replaced by "=Bf" constraint but any
> further =@cc are replaced by "=X" and scratch operand.
>
> Printing the "=X" scratch is harmless, but printing the "=Bf" causes
> another ICE, I shall submit a follow up patch for that:
> asm ("# %0" : "=@ccz"(z));
>
> test.c:6:1: internal compiler error: in print_reg, at
> config/i386/i386.c:17193
>   }
>   ^
> 0xedfc30 print_reg(rtx_def*, int, _IO_FILE*)
>      ../../gcc-trunk/gcc/config/i386/i386.c:17189
> 0xf048a4 ix86_print_operand(_IO_FILE*, rtx_def*, int)
>      ../../gcc-trunk/gcc/config/i386/i386.c:17867
> 0x8bf87c output_operand(rtx_def*, int)
>      ../../gcc-trunk/gcc/final.c:3847
> 0x8c00ee output_asm_insn(char const*, rtx_def**)
>      ../../gcc-trunk/gcc/final.c:3763
> 0x8c1f9c final_scan_insn(rtx_insn*, _IO_FILE*, int, int, int*)
>      ../../gcc-trunk/gcc/final.c:2628
> 0x8c25c9 final(rtx_insn*, _IO_FILE*, int)
>      ../../gcc-trunk/gcc/final.c:2045
> 0x8c2da9 rest_of_handle_final
>      ../../gcc-trunk/gcc/final.c:4445
> 0x8c2da9 execute
>      ../../gcc-trunk/gcc/final.c:4520
>
>
> Well, regarding the X constraint, I do think that
> it's definitely OK to use different rules if it is
> used in asms vs. when if it is used internally in .md files.
>
> The patch handles X in asms to be just a synonym to the g constraint,
> except that g allows only GENERAL_REGS and X allows ALL_REGS.
>
> What I am not sure of, is if X should allow more than g in terms of
> CONSTANT_P.  I think it should not, because probably the CONSTANT_P
> handling in general_operand is likely smarter than that of the i
> constraint.
>
>
> Bernd.

[-- Attachment #2: changelog-pr59155.txt --]
[-- Type: text/plain, Size: 421 bytes --]

gcc:
2016-05-23  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR inline-asm/59155
	* lra-constraints.c (process_alt_operands): Allow ALL_REGS.
	* recog.c (asm_operand_ok): Handle X constraint.

testsuite:
2016-05-23  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR inline-asm/59155
	* gcc.dg/torture/pr59155-1.c: New test.
	* gcc.dg/torture/pr59155-2.c: New test.
	* gcc.dg/torture/pr59155-3.c: New test.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: patch-pr59155.diff --]
[-- Type: text/x-patch; name="patch-pr59155.diff", Size: 2660 bytes --]

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));
+}

Re: [PING] [PATCH] Fix asm X constraint (PR inline-asm/59155)

[Jeff Law]

On 06/09/2016 10:45 AM, Jakub Jelinek wrote:
> On Thu, Jun 09, 2016 at 06:43:04PM +0200, Jakub Jelinek wrote:
>> Yes, I'm all in favor in disabling X constraint for inline asm.
>> Especially if people actually try to print it as well, rather than make it
>> unused.  That is a sure path to ICEs.
>
> Though, on the other side, even our documentation mentions
> asm volatile ("mtfsf 255,%1" : "=X"(sum): "f"(fpenv));
> So perhaps we need to error just in case such an argument is printed?
Are you thinking to scan the output string for %<N> for the appropriate 
<N>?  That shouldn't be too hard.  But that's not sufficient to address 
the problem Bernd is trying to tackle AFAICT.


Jeff

Re: [PING] [PATCH] Fix asm X constraint (PR inline-asm/59155)

[Bernd Edlinger]

On 06/21/16 00:06, Jeff Law wrote:
> On 06/09/2016 10:45 AM, Jakub Jelinek wrote:
>> On Thu, Jun 09, 2016 at 06:43:04PM +0200, Jakub Jelinek wrote:
>>> Yes, I'm all in favor in disabling X constraint for inline asm.
>>> Especially if people actually try to print it as well, rather than
>>> make it
>>> unused.  That is a sure path to ICEs.
>>
>> Though, on the other side, even our documentation mentions
>> asm volatile ("mtfsf 255,%1" : "=X"(sum): "f"(fpenv));
>> So perhaps we need to error just in case such an argument is printed?
> Are you thinking to scan the output string for %<N> for the appropriate
> <N>?  That shouldn't be too hard.  But that's not sufficient to address
> the problem Bernd is trying to tackle AFAICT.

Correct.

And furthermore, the use case with matching X input & output that Marc
wanted to use, seems to be a valid one.  Because "+X" allows more
registers than "+g" or "+r", it should create less register pressure.
And although it is probably unpredictable if a register of the
class "ALL_REGISTERS" will work for an assembler instruction, it is
still interesting to print it in an assembler comment.


Bernd.

Re: [PING**2] [PATCH] Fix asm X constraint (PR inline-asm/59155)

[Jeff Law]

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

Re: [PING**2] [PATCH] Fix asm X constraint (PR inline-asm/59155)

[Bernd Edlinger]

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.

Re: [PING**2] [PATCH] Fix asm X constraint (PR inline-asm/59155)

[Jeff Law]

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

Re: [PING**2] [PATCH] Fix asm X constraint (PR inline-asm/59155)

[Bernd Edlinger]

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.

Re: [PING**2] [PATCH] Fix asm X constraint (PR inline-asm/59155)

[Jeff Law]

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

Re: [PING**2] [PATCH] Fix asm X constraint (PR inline-asm/59155)

[Bernd Edlinger]

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.