[07/25,pr82089] Don't sign-extend SFV 1 in BImode

Message ID e2dbc848a3d62df872695b8289b0a034e0c8c03d.1536144068.git.ams@codesourcery.com
State New
Headers show
Series
  • AMD GCN Port
Related show

Commit Message

Andrew Stubbs Sept. 5, 2018, 11:49 a.m.
This is an update of the patch posted to PR82089 long ago.  We ran into the
same bug on GCN, so we need this fixed as part of this series.

2018-09-05  Andrew Stubbs  <ams@codesourcery.com>
            Tom de Vries  <tom@codesourcery.com>

	PR82089

	gcc/
	* expmed.c (emit_cstore): Fix handling of result_mode == BImode and
	STORE_FLAG_VALUE == 1.
---
 gcc/expmed.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Comments

Richard Sandiford Sept. 17, 2018, 8:40 a.m. | #1
<ams@codesourcery.com> writes:
> This is an update of the patch posted to PR82089 long ago.  We ran into the

> same bug on GCN, so we need this fixed as part of this series.

>

> 2018-09-05  Andrew Stubbs  <ams@codesourcery.com>

>             Tom de Vries  <tom@codesourcery.com>

>

> 	PR82089

>

> 	gcc/

> 	* expmed.c (emit_cstore): Fix handling of result_mode == BImode and

> 	STORE_FLAG_VALUE == 1.

> ---

>  gcc/expmed.c | 15 +++++++++++----

>  1 file changed, 11 insertions(+), 4 deletions(-)

>

> diff --git a/gcc/expmed.c b/gcc/expmed.c

> index 29ce10b..0b87fdc 100644

> --- a/gcc/expmed.c

> +++ b/gcc/expmed.c

> @@ -5464,11 +5464,18 @@ emit_cstore (rtx target, enum insn_code icode, enum rtx_code code,

>       If STORE_FLAG_VALUE does not have the sign bit set when

>       interpreted in MODE, we can do this conversion as unsigned, which

>       is usually more efficient.  */

> -  if (GET_MODE_SIZE (int_target_mode) > GET_MODE_SIZE (result_mode))

> +  if (GET_MODE_SIZE (int_target_mode) > GET_MODE_SIZE (result_mode)

> +      || (result_mode == BImode && int_target_mode != BImode))


Would be better to test GET_MODE_PRECISION instead of GET_MODE_SIZE,
if that works, instead of treating BImode as a special case.

>      {

> -      convert_move (target, subtarget,

> -		    val_signbit_known_clear_p (result_mode,

> -					       STORE_FLAG_VALUE));

> +      gcc_assert (GET_MODE_SIZE (result_mode) != 1

> +		  || STORE_FLAG_VALUE == 1 || STORE_FLAG_VALUE == -1);

> +      bool unsignedp

> +	= (GET_MODE_SIZE (result_mode) == 1

> +	   ? STORE_FLAG_VALUE == 1

> +	   : val_signbit_known_clear_p (result_mode, STORE_FLAG_VALUE));

> +

> +      convert_move (target, subtarget, unsignedp);

> +


GET_MODE_SIZE == 1 would also trigger for QImode, which shouldn't be treated
differently from HImode etc.

The original val_signbit_known_clear_p test seems like it might be an
abstraction too far.  In practice STORE_FLAG_VALUE has to fit within
the mode of a natural (unextended) condition result, so I think we can
simply test STORE_FLAG_VALUE >= 0 for all modes to see whether the target
wants the result to be treated as signed or unsigned.

Thanks,
Richard
Andrew Stubbs Sept. 26, 2018, 3:52 p.m. | #2
On 17/09/18 09:40, Richard Sandiford wrote:
> <ams@codesourcery.com> writes:

>> This is an update of the patch posted to PR82089 long ago.  We ran into the

>> same bug on GCN, so we need this fixed as part of this series.

>>

>> 2018-09-05  Andrew Stubbs  <ams@codesourcery.com>

>>              Tom de Vries  <tom@codesourcery.com>

>>

>> 	PR82089

>>

>> 	gcc/

>> 	* expmed.c (emit_cstore): Fix handling of result_mode == BImode and

>> 	STORE_FLAG_VALUE == 1.

>> ---

>>   gcc/expmed.c | 15 +++++++++++----

>>   1 file changed, 11 insertions(+), 4 deletions(-)

>>

>> diff --git a/gcc/expmed.c b/gcc/expmed.c

>> index 29ce10b..0b87fdc 100644

>> --- a/gcc/expmed.c

>> +++ b/gcc/expmed.c

>> @@ -5464,11 +5464,18 @@ emit_cstore (rtx target, enum insn_code icode, enum rtx_code code,

>>        If STORE_FLAG_VALUE does not have the sign bit set when

>>        interpreted in MODE, we can do this conversion as unsigned, which

>>        is usually more efficient.  */

>> -  if (GET_MODE_SIZE (int_target_mode) > GET_MODE_SIZE (result_mode))

>> +  if (GET_MODE_SIZE (int_target_mode) > GET_MODE_SIZE (result_mode)

>> +      || (result_mode == BImode && int_target_mode != BImode))

> 

> Would be better to test GET_MODE_PRECISION instead of GET_MODE_SIZE,

> if that works, instead of treating BImode as a special case.

> 

>>       {

>> -      convert_move (target, subtarget,

>> -		    val_signbit_known_clear_p (result_mode,

>> -					       STORE_FLAG_VALUE));

>> +      gcc_assert (GET_MODE_SIZE (result_mode) != 1

>> +		  || STORE_FLAG_VALUE == 1 || STORE_FLAG_VALUE == -1);

>> +      bool unsignedp

>> +	= (GET_MODE_SIZE (result_mode) == 1

>> +	   ? STORE_FLAG_VALUE == 1

>> +	   : val_signbit_known_clear_p (result_mode, STORE_FLAG_VALUE));

>> +

>> +      convert_move (target, subtarget, unsignedp);

>> +

> 

> GET_MODE_SIZE == 1 would also trigger for QImode, which shouldn't be treated

> differently from HImode etc.

> 

> The original val_signbit_known_clear_p test seems like it might be an

> abstraction too far.  In practice STORE_FLAG_VALUE has to fit within

> the mode of a natural (unextended) condition result, so I think we can

> simply test STORE_FLAG_VALUE >= 0 for all modes to see whether the target

> wants the result to be treated as signed or unsigned.


How about the attached?

I think I addressed all your comments, and it tests fine on GCN with no 
regressions.

Andrew
[pr82089] Don't sign-extend SFV 1 in BImode

This is an update of the patch posted to PR82089 long ago.  We ran into the
same bug on GCN, so we need this fixed as part of this series.

2018-09-26  Andrew Stubbs  <ams@codesourcery.com>
            Tom de Vries  <tom@codesourcery.com>

	PR82089

	gcc/
	* expmed.c (emit_cstore): Fix handling of result_mode == BImode and
	STORE_FLAG_VALUE == 1.

diff --git a/gcc/expmed.c b/gcc/expmed.c
index 29ce10b..444d6a8 100644
--- a/gcc/expmed.c
+++ b/gcc/expmed.c
@@ -5464,11 +5464,14 @@ emit_cstore (rtx target, enum insn_code icode, enum rtx_code code,
      If STORE_FLAG_VALUE does not have the sign bit set when
      interpreted in MODE, we can do this conversion as unsigned, which
      is usually more efficient.  */
-  if (GET_MODE_SIZE (int_target_mode) > GET_MODE_SIZE (result_mode))
+  if (GET_MODE_PRECISION (int_target_mode) > GET_MODE_PRECISION (result_mode))
     {
-      convert_move (target, subtarget,
-		    val_signbit_known_clear_p (result_mode,
-					       STORE_FLAG_VALUE));
+      gcc_assert (GET_MODE_PRECISION (result_mode) != 1
+		  || STORE_FLAG_VALUE == 1 || STORE_FLAG_VALUE == -1);
+
+      bool unsignedp = (STORE_FLAG_VALUE >= 0);
+      convert_move (target, subtarget, unsignedp);
+
       op0 = target;
       result_mode = int_target_mode;
     }
Richard Sandiford Sept. 26, 2018, 4:25 p.m. | #3
Andrew Stubbs <ams@codesourcery.com> writes:
> On 17/09/18 09:40, Richard Sandiford wrote:

>> <ams@codesourcery.com> writes:

>>> This is an update of the patch posted to PR82089 long ago.  We ran into the

>>> same bug on GCN, so we need this fixed as part of this series.

>>>

>>> 2018-09-05  Andrew Stubbs  <ams@codesourcery.com>

>>>              Tom de Vries  <tom@codesourcery.com>

>>>

>>> 	PR82089

>>>

>>> 	gcc/

>>> 	* expmed.c (emit_cstore): Fix handling of result_mode == BImode and

>>> 	STORE_FLAG_VALUE == 1.

>>> ---

>>>   gcc/expmed.c | 15 +++++++++++----

>>>   1 file changed, 11 insertions(+), 4 deletions(-)

>>>

>>> diff --git a/gcc/expmed.c b/gcc/expmed.c

>>> index 29ce10b..0b87fdc 100644

>>> --- a/gcc/expmed.c

>>> +++ b/gcc/expmed.c

>>> @@ -5464,11 +5464,18 @@ emit_cstore (rtx target, enum insn_code icode, enum rtx_code code,

>>>        If STORE_FLAG_VALUE does not have the sign bit set when

>>>        interpreted in MODE, we can do this conversion as unsigned, which

>>>        is usually more efficient.  */

>>> -  if (GET_MODE_SIZE (int_target_mode) > GET_MODE_SIZE (result_mode))

>>> +  if (GET_MODE_SIZE (int_target_mode) > GET_MODE_SIZE (result_mode)

>>> +      || (result_mode == BImode && int_target_mode != BImode))

>> 

>> Would be better to test GET_MODE_PRECISION instead of GET_MODE_SIZE,

>> if that works, instead of treating BImode as a special case.

>> 

>>>       {

>>> -      convert_move (target, subtarget,

>>> -		    val_signbit_known_clear_p (result_mode,

>>> -					       STORE_FLAG_VALUE));

>>> +      gcc_assert (GET_MODE_SIZE (result_mode) != 1

>>> +		  || STORE_FLAG_VALUE == 1 || STORE_FLAG_VALUE == -1);

>>> +      bool unsignedp

>>> +	= (GET_MODE_SIZE (result_mode) == 1

>>> +	   ? STORE_FLAG_VALUE == 1

>>> +	   : val_signbit_known_clear_p (result_mode, STORE_FLAG_VALUE));

>>> +

>>> +      convert_move (target, subtarget, unsignedp);

>>> +

>> 

>> GET_MODE_SIZE == 1 would also trigger for QImode, which shouldn't be treated

>> differently from HImode etc.

>> 

>> The original val_signbit_known_clear_p test seems like it might be an

>> abstraction too far.  In practice STORE_FLAG_VALUE has to fit within

>> the mode of a natural (unextended) condition result, so I think we can

>> simply test STORE_FLAG_VALUE >= 0 for all modes to see whether the target

>> wants the result to be treated as signed or unsigned.

>

> How about the attached?

>

> I think I addressed all your comments, and it tests fine on GCN with no 

> regressions.

>

> Andrew

>

> [pr82089] Don't sign-extend SFV 1 in BImode

>

> This is an update of the patch posted to PR82089 long ago.  We ran into the

> same bug on GCN, so we need this fixed as part of this series.

>

> 2018-09-26  Andrew Stubbs  <ams@codesourcery.com>

>             Tom de Vries  <tom@codesourcery.com>

>

> 	PR82089

>

> 	gcc/

> 	* expmed.c (emit_cstore): Fix handling of result_mode == BImode and

> 	STORE_FLAG_VALUE == 1.


OK, thanks.

Richard
Andrew Stubbs Sept. 27, 2018, 11:29 a.m. | #4
On 26/09/18 17:25, Richard Sandiford wrote:
> OK, thanks.


Committed, thanks.

Andrew

Patch

diff --git a/gcc/expmed.c b/gcc/expmed.c
index 29ce10b..0b87fdc 100644
--- a/gcc/expmed.c
+++ b/gcc/expmed.c
@@ -5464,11 +5464,18 @@  emit_cstore (rtx target, enum insn_code icode, enum rtx_code code,
      If STORE_FLAG_VALUE does not have the sign bit set when
      interpreted in MODE, we can do this conversion as unsigned, which
      is usually more efficient.  */
-  if (GET_MODE_SIZE (int_target_mode) > GET_MODE_SIZE (result_mode))
+  if (GET_MODE_SIZE (int_target_mode) > GET_MODE_SIZE (result_mode)
+      || (result_mode == BImode && int_target_mode != BImode))
     {
-      convert_move (target, subtarget,
-		    val_signbit_known_clear_p (result_mode,
-					       STORE_FLAG_VALUE));
+      gcc_assert (GET_MODE_SIZE (result_mode) != 1
+		  || STORE_FLAG_VALUE == 1 || STORE_FLAG_VALUE == -1);
+      bool unsignedp
+	= (GET_MODE_SIZE (result_mode) == 1
+	   ? STORE_FLAG_VALUE == 1
+	   : val_signbit_known_clear_p (result_mode, STORE_FLAG_VALUE));
+
+      convert_move (target, subtarget, unsignedp);
+
       op0 = target;
       result_mode = int_target_mode;
     }