[PATCH] Don't allow combination of read/write and earlyclobber constraint modifier

[PATCH] Don't allow combination of read/write and earlyclobber constraint modifier

[Tom de Vries]

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

On 01-07-14 21:47, Jeff Law wrote:
> On 07/01/14 13:27, Tom de Vries wrote:
>> So my question is: is the combination of '&' and '+' supported ? If so,
>> what is the exact semantics ? If not, should we warn or give an error ?
 >
> I don't think we can define any reasonable semantics for &+.  My recommendation
> would be for this to be considered a hard error.
>

[ move discussion from gcc ml to gcc-patches ml ]

Attached patch detects the combination of + and & constrains during genrecog, 
and generates an error like this:
...
/home/vries/gcc_versions/devel/src/gcc/config/aarch64/aarch64-simd.md:1020: 
operand 0 has in-out reload, incompatible with earlyclobber
/home/vries/gcc_versions/devel/src/gcc/config/aarch64/aarch64-simd.md:1020: 
operand 0 has in-out reload, incompatible with earlyclobber
/home/vries/gcc_versions/devel/src/gcc/config/aarch64/aarch64-simd.md:1020: 
operand 0 has in-out reload, incompatible with earlyclobber
make[2]: *** [s-recog] Error 1
...
The error triggers three times, once for each mode iterator element.

OK if x86_64 bootstrap succeeds ?

Thanks,
- Tom

[-- Attachment #2: 0004-Don-t-allow-earlyclobber-modifier-with-read-write-mo.patch --]
[-- Type: text/x-patch, Size: 788 bytes --]

2014-07-02  Tom de Vries  <tom@codesourcery.com>

	* genrecog.c (validate_pattern): Don't allow earlyclobber constraint
	modifier with read/write constraint modifier.

diff --git a/gcc/genrecog.c b/gcc/genrecog.c
index 457b59c..ad709ee 100644
--- a/gcc/genrecog.c
+++ b/gcc/genrecog.c
@@ -481,6 +481,13 @@ validate_pattern (rtx pattern, rtx insn, rtx set, int set_code)
 				   rtx_name[GET_CODE (insn)]);
 	      }
 
+	    if (constraints0 == '+'
+		&& strchr (XSTR (pattern, 2), '&') != NULL)
+	      error_with_line (pattern_lineno,
+			       "operand %d has in-out reload, incompatible with"
+			       " earlyclobber",
+			       XINT (pattern, 0));
+
 	    /* A MATCH_OPERAND that is a SET should have an output reload.  */
 	    else if (set && constraints0)
 	      {
-- 
1.9.1

Re: combination of read/write and earlyclobber constraint modifier

[Tom de Vries]

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

On 02-07-14 08:23, Marc Glisse wrote:
> In the first example you gave, looking at the pattern (no match_dup, setting the
> full register), it seems that it may have wanted "=&" instead of "+&".

[ move discussion from gcc ml to gcc-patches ml ]

Marcus,

The +& constraint on operand 0 of vec_unpack_trunc_<mode> seems wrong, since the 
template does not use the operand as input.

This patch fixes that.

OK for trunk if aarch64 build & regtest succeeds ?

Thanks,
- Tom



[-- Attachment #2: 0006-Fix-constraint-in-vec_unpack_trunc_-mode.patch --]
[-- Type: text/x-patch, Size: 729 bytes --]

2014-07-02  Tom de Vries  <tom@codesourcery.com>

	* config/aarch64/aarch64-simd.md
	(define_insn "vec_unpack_trunc_<mode>"): Fix constraint.

diff --git a/gcc/config/aarch64/aarch64-simd.md b/gcc/config/aarch64/aarch64-simd.md
index 1c32f0c..0377de4 100644
--- a/gcc/config/aarch64/aarch64-simd.md
+++ b/gcc/config/aarch64/aarch64-simd.md
@@ -1018,7 +1018,7 @@
 ;; For quads.
 
 (define_insn "vec_pack_trunc_<mode>"
- [(set (match_operand:<VNARROWQ2> 0 "register_operand" "+&w")
+ [(set (match_operand:<VNARROWQ2> 0 "register_operand" "=&w")
        (vec_concat:<VNARROWQ2>
 	 (truncate:<VNARROWQ> (match_operand:VQN 1 "register_operand" "w"))
 	 (truncate:<VNARROWQ> (match_operand:VQN 2 "register_operand" "w"))))]
-- 
1.9.1

Re: [PATCH] Don't allow combination of read/write and earlyclobber constraint modifier

[Richard Earnshaw]

On 02/07/14 08:52, Tom de Vries wrote:
> On 01-07-14 21:47, Jeff Law wrote:
>> On 07/01/14 13:27, Tom de Vries wrote:
>>> So my question is: is the combination of '&' and '+' supported ? If so,
>>> what is the exact semantics ? If not, should we warn or give an error ?
>  >
>> I don't think we can define any reasonable semantics for &+.  My recommendation
>> would be for this to be considered a hard error.
>>
> 

Why would this be any different in behaviour from use of operand tie
constraints to early-clobber?

In my view, it says that this operand is safe from the early-clobber
limitation, but other operands are not and need to be in other registers.

Eg op0 ("=&r") op1("0") op2("r") says that op1 must be the same register
as op0, but op2 must be a different register, so A = A <op> B is ok, but
A = A <op> A is not.

R.

> [ move discussion from gcc ml to gcc-patches ml ]
> 
> Attached patch detects the combination of + and & constrains during genrecog, 
> and generates an error like this:
> ...
> /home/vries/gcc_versions/devel/src/gcc/config/aarch64/aarch64-simd.md:1020: 
> operand 0 has in-out reload, incompatible with earlyclobber
> /home/vries/gcc_versions/devel/src/gcc/config/aarch64/aarch64-simd.md:1020: 
> operand 0 has in-out reload, incompatible with earlyclobber
> /home/vries/gcc_versions/devel/src/gcc/config/aarch64/aarch64-simd.md:1020: 
> operand 0 has in-out reload, incompatible with earlyclobber
> make[2]: *** [s-recog] Error 1
> ...
> The error triggers three times, once for each mode iterator element.
> 
> OK if x86_64 bootstrap succeeds ?
> 
> Thanks,
> - Tom
> 
> 
> 0004-Don-t-allow-earlyclobber-modifier-with-read-write-mo.patch
> 
> 
> 2014-07-02  Tom de Vries  <tom@codesourcery.com>
> 
> 	* genrecog.c (validate_pattern): Don't allow earlyclobber constraint
> 	modifier with read/write constraint modifier.
> 
> diff --git a/gcc/genrecog.c b/gcc/genrecog.c
> index 457b59c..ad709ee 100644
> --- a/gcc/genrecog.c
> +++ b/gcc/genrecog.c
> @@ -481,6 +481,13 @@ validate_pattern (rtx pattern, rtx insn, rtx set, int set_code)
>  				   rtx_name[GET_CODE (insn)]);
>  	      }
>  
> +	    if (constraints0 == '+'
> +		&& strchr (XSTR (pattern, 2), '&') != NULL)
> +	      error_with_line (pattern_lineno,
> +			       "operand %d has in-out reload, incompatible with"
> +			       " earlyclobber",
> +			       XINT (pattern, 0));
> +
>  	    /* A MATCH_OPERAND that is a SET should have an output reload.  */
>  	    else if (set && constraints0)
>  	      {
> 

Re: combination of read/write and earlyclobber constraint modifier

[Marcus Shawcroft]

On 2 July 2014 09:02, Tom de Vries <Tom_deVries@mentor.com> wrote:
> On 02-07-14 08:23, Marc Glisse wrote:
>>
>> In the first example you gave, looking at the pattern (no match_dup,
>> setting the
>> full register), it seems that it may have wanted "=&" instead of "+&".
>
>
> [ move discussion from gcc ml to gcc-patches ml ]
>
> Marcus,
>
> The +& constraint on operand 0 of vec_unpack_trunc_<mode> seems wrong, since
> the template does not use the operand as input.
>
> This patch fixes that.
>
> OK for trunk if aarch64 build & regtest succeeds ?

Your patch looks fine, operand 0 isn't used for input.  OK assuming no
regression.   Did you find this by inspection or is this the cause of
some bug?

/Marcus

Re: combination of read/write and earlyclobber constraint modifier

[Tom de Vries]

On 03-07-14 10:20, Marcus Shawcroft wrote:
> On 2 July 2014 09:02, Tom de Vries <Tom_deVries@mentor.com> wrote:
>> On 02-07-14 08:23, Marc Glisse wrote:
>>>
>>> In the first example you gave, looking at the pattern (no match_dup,
>>> setting the
>>> full register), it seems that it may have wanted "=&" instead of "+&".
>>
>>
>> [ move discussion from gcc ml to gcc-patches ml ]
>>
>> Marcus,
>>
>> The +& constraint on operand 0 of vec_unpack_trunc_<mode> seems wrong, since
>> the template does not use the operand as input.
>>
>> This patch fixes that.
>>
>> OK for trunk if aarch64 build & regtest succeeds ?
>
> Your patch looks fine, operand 0 isn't used for input.  OK assuming no
> regression.   Did you find this by inspection or is this the cause of
> some bug?
>

Marcus,

I found this by inspection: https://gcc.gnu.org/ml/gcc/2014-07/msg00007.html .

Thanks,
- Tom

[PATCH, COMMITTED] Update earlyclobber documentation

[Tom de Vries]

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

[ was: Re: combination of read/write and earlyclobber constraint modifier on gcc@ ]
On 03-07-14 00:42, Jeff Law wrote:
> Based on various followups (public & private), let's go with the clarification
> above.  Richard E. explicitly added support for this in the mid/late 90s as an
> optimization for the ARM.

Jeff,

Committed as attached.

Thanks to all for the helpful comments.

- Tom

[-- Attachment #2: 0002-Improve-documentation-of-earlyclobber.patch --]
[-- Type: text/x-patch, Size: 816 bytes --]

2014-07-04  Tom de Vries  <tom@codesourcery.com>

	* doc/md.texi (@subsection Constraint Modifier Characters): Clarify
	combination of earlyclobber and read/write modifiers.

diff --git a/gcc/doc/md.texi b/gcc/doc/md.texi
index 539865e..fde67d7 100644
--- a/gcc/doc/md.texi
+++ b/gcc/doc/md.texi
@@ -1582,7 +1582,10 @@ alternatives of this form often allows GCC to produce better code
 when only some of the inputs can be affected by the earlyclobber.
 See, for example, the @samp{mulsi3} insn of the ARM@.
 
-@samp{&} does not obviate the need to write @samp{=}.
+Furthermore, if the @dfn{earlyclobber} operand is also read/write operand, then
+that operand is modified only after it's used.
+
+@samp{&} does not obviate the need to write @samp{=} or @samp{+}.
 
 @cindex @samp{%} in constraint
 @item %
-- 
1.9.1

Re: combination of read/write and earlyclobber constraint modifier

[Christophe Lyon]

On 3 July 2014 10:34, Tom de Vries <Tom_deVries@mentor.com> wrote:
> On 03-07-14 10:20, Marcus Shawcroft wrote:
>>
>> On 2 July 2014 09:02, Tom de Vries <Tom_deVries@mentor.com> wrote:
>>>
>>> On 02-07-14 08:23, Marc Glisse wrote:
>>>>
>>>>
>>>> In the first example you gave, looking at the pattern (no match_dup,
>>>> setting the
>>>> full register), it seems that it may have wanted "=&" instead of "+&".
>>>
>>>
>>>
>>> [ move discussion from gcc ml to gcc-patches ml ]
>>>
>>> Marcus,
>>>
>>> The +& constraint on operand 0 of vec_unpack_trunc_<mode> seems wrong,
>>> since
>>> the template does not use the operand as input.
>>>
>>> This patch fixes that.
>>>
>>> OK for trunk if aarch64 build & regtest succeeds ?
>>
>>
>> Your patch looks fine, operand 0 isn't used for input.  OK assuming no
>> regression.   Did you find this by inspection or is this the cause of
>> some bug?
>>
>
> Marcus,
>
> I found this by inspection: https://gcc.gnu.org/ml/gcc/2014-07/msg00007.html
> .
>
> Thanks,
> - Tom
>

Hi,

This patch causes gcc.target/aarch64/vmlsq_laneq.c to FAIL on
aarch64_be-none-elf.

Christophe.

Re: combination of read/write and earlyclobber constraint modifier

[Christophe Lyon]

On 7 July 2014 11:29, Christophe Lyon <christophe.lyon@linaro.org> wrote:
> On 3 July 2014 10:34, Tom de Vries <Tom_deVries@mentor.com> wrote:
>> On 03-07-14 10:20, Marcus Shawcroft wrote:
>>>
>>> On 2 July 2014 09:02, Tom de Vries <Tom_deVries@mentor.com> wrote:
>>>>
>>>> On 02-07-14 08:23, Marc Glisse wrote:
>>>>>
>>>>>
>>>>> In the first example you gave, looking at the pattern (no match_dup,
>>>>> setting the
>>>>> full register), it seems that it may have wanted "=&" instead of "+&".
>>>>
>>>>
>>>>
>>>> [ move discussion from gcc ml to gcc-patches ml ]
>>>>
>>>> Marcus,
>>>>
>>>> The +& constraint on operand 0 of vec_unpack_trunc_<mode> seems wrong,
>>>> since
>>>> the template does not use the operand as input.
>>>>
>>>> This patch fixes that.
>>>>
>>>> OK for trunk if aarch64 build & regtest succeeds ?
>>>
>>>
>>> Your patch looks fine, operand 0 isn't used for input.  OK assuming no
>>> regression.   Did you find this by inspection or is this the cause of
>>> some bug?
>>>
>>
>> Marcus,
>>
>> I found this by inspection: https://gcc.gnu.org/ml/gcc/2014-07/msg00007.html
>> .
>>
>> Thanks,
>> - Tom
>>
>
> Hi,
>
> This patch causes gcc.target/aarch64/vmlsq_laneq.c to FAIL on
> aarch64_be-none-elf.
>
> Christophe.

... which was fixed by James' commit 212298

Sorry for the noise

Christophe.