[PATCH] vect: Fix integer overflow calculating mask

[PATCH] vect: Fix integer overflow calculating mask

[Andrew Stubbs]

This is a follow-up to the previous patch to ensure that integer vector
bit-masks do not have excess bits set. It fixes a bug, observed on
amdgcn, in which the mask could be incorrectly set to zero, resulting in
wrong-code.

The mask was broken when nunits==32. The patched version will probably
be broken for nunits==64, but I don't think any current targets have
masks with more than 64 bits.

OK for mainline?

Andrew

gcc/ChangeLog:

	* expr.cc (store_constructor): Use 64-bit shifts.
---
 gcc/expr.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/expr.cc b/gcc/expr.cc
index e238811110e..90de5decee3 100644
--- a/gcc/expr.cc
+++ b/gcc/expr.cc
@@ -7879,7 +7879,7 @@ store_constructor (tree exp, rtx target, int cleared, poly_int64 size,
 	    auto nunits = TYPE_VECTOR_SUBPARTS (type).to_constant ();
 	    if (maybe_ne (GET_MODE_PRECISION (mode), nunits))
 	      tmp = expand_binop (mode, and_optab, tmp,
-				  GEN_INT ((1 << nunits) - 1), target,
+				  GEN_INT ((1UL << nunits) - 1), target,
 				  true, OPTAB_WIDEN);
 	    if (tmp != target)
 	      emit_move_insn (target, tmp);
-- 
2.41.0

Re: [PATCH] vect: Fix integer overflow calculating mask

[Jakub Jelinek]

On Fri, Feb 23, 2024 at 12:58:53PM +0000, Andrew Stubbs wrote:
> This is a follow-up to the previous patch to ensure that integer vector
> bit-masks do not have excess bits set. It fixes a bug, observed on
> amdgcn, in which the mask could be incorrectly set to zero, resulting in
> wrong-code.
> 
> The mask was broken when nunits==32. The patched version will probably
> be broken for nunits==64, but I don't think any current targets have
> masks with more than 64 bits.
> 
> OK for mainline?
> 
> Andrew
> 
> gcc/ChangeLog:
> 
> 	* expr.cc (store_constructor): Use 64-bit shifts.

No, this isn't 64-bit shift on all hosts.
Use HOST_WIDE_INT_1U instead.

> --- a/gcc/expr.cc
> +++ b/gcc/expr.cc
> @@ -7879,7 +7879,7 @@ store_constructor (tree exp, rtx target, int cleared, poly_int64 size,
>  	    auto nunits = TYPE_VECTOR_SUBPARTS (type).to_constant ();
>  	    if (maybe_ne (GET_MODE_PRECISION (mode), nunits))
>  	      tmp = expand_binop (mode, and_optab, tmp,
> -				  GEN_INT ((1 << nunits) - 1), target,
> +				  GEN_INT ((1UL << nunits) - 1), target,
>  				  true, OPTAB_WIDEN);
>  	    if (tmp != target)
>  	      emit_move_insn (target, tmp);
> -- 
> 2.41.0

	Jakub

Re: [PATCH] vect: Fix integer overflow calculating mask

[Richard Biener]

> Am 23.02.2024 um 14:03 schrieb Jakub Jelinek <jakub@redhat.com>:
> 
> On Fri, Feb 23, 2024 at 12:58:53PM +0000, Andrew Stubbs wrote:
>> This is a follow-up to the previous patch to ensure that integer vector
>> bit-masks do not have excess bits set. It fixes a bug, observed on
>> amdgcn, in which the mask could be incorrectly set to zero, resulting in
>> wrong-code.
>> 
>> The mask was broken when nunits==32. The patched version will probably
>> be broken for nunits==64, but I don't think any current targets have
>> masks with more than 64 bits.
>> 
>> OK for mainline?
>> 
>> Andrew
>> 
>> gcc/ChangeLog:
>> 
>>    * expr.cc (store_constructor): Use 64-bit shifts.
> 
> No, this isn't 64-bit shift on all hosts.
> Use HOST_WIDE_INT_1U instead.

I think there are now two other similar places recently added that need adjustment as well.

Richard 

>> --- a/gcc/expr.cc
>> +++ b/gcc/expr.cc
>> @@ -7879,7 +7879,7 @@ store_constructor (tree exp, rtx target, int cleared, poly_int64 size,
>>        auto nunits = TYPE_VECTOR_SUBPARTS (type).to_constant ();
>>        if (maybe_ne (GET_MODE_PRECISION (mode), nunits))
>>          tmp = expand_binop (mode, and_optab, tmp,
>> -                  GEN_INT ((1 << nunits) - 1), target,
>> +                  GEN_INT ((1UL << nunits) - 1), target,
>>                  true, OPTAB_WIDEN);
>>        if (tmp != target)
>>          emit_move_insn (target, tmp);
>> --
>> 2.41.0
> 
>    Jakub
> 

Re: [PATCH] vect: Fix integer overflow calculating mask

[Andrew Stubbs]

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

On 23/02/2024 13:02, Jakub Jelinek wrote:
> On Fri, Feb 23, 2024 at 12:58:53PM +0000, Andrew Stubbs wrote:
>> This is a follow-up to the previous patch to ensure that integer vector
>> bit-masks do not have excess bits set. It fixes a bug, observed on
>> amdgcn, in which the mask could be incorrectly set to zero, resulting in
>> wrong-code.
>>
>> The mask was broken when nunits==32. The patched version will probably
>> be broken for nunits==64, but I don't think any current targets have
>> masks with more than 64 bits.
>>
>> OK for mainline?
>>
>> Andrew
>>
>> gcc/ChangeLog:
>>
>> 	* expr.cc (store_constructor): Use 64-bit shifts.
> 
> No, this isn't 64-bit shift on all hosts.
> Use HOST_WIDE_INT_1U instead.

OK, I did wonder if there was a proper way to do it. :)

How about this?

Andrew

[-- Attachment #2: gcc.patch --]
[-- Type: text/plain, Size: 789 bytes --]

vect: Fix integer overflow calculating mask

The mask was broken when nunits==32 on hosts where int is 32-bit.

gcc/ChangeLog:

	* expr.cc (store_constructor): Use 64-bit shifts.

diff --git a/gcc/expr.cc b/gcc/expr.cc
index e238811110e..6bd16ac7f49 100644
--- a/gcc/expr.cc
+++ b/gcc/expr.cc
@@ -7879,8 +7879,8 @@ store_constructor (tree exp, rtx target, int cleared, poly_int64 size,
 	    auto nunits = TYPE_VECTOR_SUBPARTS (type).to_constant ();
 	    if (maybe_ne (GET_MODE_PRECISION (mode), nunits))
 	      tmp = expand_binop (mode, and_optab, tmp,
-				  GEN_INT ((1 << nunits) - 1), target,
-				  true, OPTAB_WIDEN);
+				  GEN_INT ((HOST_WIDE_INT_1U << nunits) - 1),
+				  target, true, OPTAB_WIDEN);
 	    if (tmp != target)
 	      emit_move_insn (target, tmp);
 	    break;

Re: [PATCH] vect: Fix integer overflow calculating mask

[Jakub Jelinek]

On Fri, Feb 23, 2024 at 02:22:19PM +0000, Andrew Stubbs wrote:
> On 23/02/2024 13:02, Jakub Jelinek wrote:
> > On Fri, Feb 23, 2024 at 12:58:53PM +0000, Andrew Stubbs wrote:
> > > This is a follow-up to the previous patch to ensure that integer vector
> > > bit-masks do not have excess bits set. It fixes a bug, observed on
> > > amdgcn, in which the mask could be incorrectly set to zero, resulting in
> > > wrong-code.
> > > 
> > > The mask was broken when nunits==32. The patched version will probably
> > > be broken for nunits==64, but I don't think any current targets have
> > > masks with more than 64 bits.
> > > 
> > > OK for mainline?
> > > 
> > > Andrew
> > > 
> > > gcc/ChangeLog:
> > > 
> > > 	* expr.cc (store_constructor): Use 64-bit shifts.
> > 
> > No, this isn't 64-bit shift on all hosts.
> > Use HOST_WIDE_INT_1U instead.
> 
> OK, I did wonder if there was a proper way to do it. :)
> 
> How about this?

If you change the other two GEN_INT ((1 << nunits) - 1) occurrences in
expr.cc the same way, then LGTM.

	Jakub

Re: [PATCH] vect: Fix integer overflow calculating mask

[Richard Biener]

On Fri, 23 Feb 2024, Jakub Jelinek wrote:

> On Fri, Feb 23, 2024 at 02:22:19PM +0000, Andrew Stubbs wrote:
> > On 23/02/2024 13:02, Jakub Jelinek wrote:
> > > On Fri, Feb 23, 2024 at 12:58:53PM +0000, Andrew Stubbs wrote:
> > > > This is a follow-up to the previous patch to ensure that integer vector
> > > > bit-masks do not have excess bits set. It fixes a bug, observed on
> > > > amdgcn, in which the mask could be incorrectly set to zero, resulting in
> > > > wrong-code.
> > > > 
> > > > The mask was broken when nunits==32. The patched version will probably
> > > > be broken for nunits==64, but I don't think any current targets have
> > > > masks with more than 64 bits.
> > > > 
> > > > OK for mainline?
> > > > 
> > > > Andrew
> > > > 
> > > > gcc/ChangeLog:
> > > > 
> > > > 	* expr.cc (store_constructor): Use 64-bit shifts.
> > > 
> > > No, this isn't 64-bit shift on all hosts.
> > > Use HOST_WIDE_INT_1U instead.
> > 
> > OK, I did wonder if there was a proper way to do it. :)
> > 
> > How about this?
> 
> If you change the other two GEN_INT ((1 << nunits) - 1) occurrences in
> expr.cc the same way, then LGTM.

There's also two in dojump.cc

Richard.

Re: [PATCH] vect: Fix integer overflow calculating mask

[Andrew Stubbs]

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

On 23/02/2024 15:13, Richard Biener wrote:
> On Fri, 23 Feb 2024, Jakub Jelinek wrote:
> 
>> On Fri, Feb 23, 2024 at 02:22:19PM +0000, Andrew Stubbs wrote:
>>> On 23/02/2024 13:02, Jakub Jelinek wrote:
>>>> On Fri, Feb 23, 2024 at 12:58:53PM +0000, Andrew Stubbs wrote:
>>>>> This is a follow-up to the previous patch to ensure that integer vector
>>>>> bit-masks do not have excess bits set. It fixes a bug, observed on
>>>>> amdgcn, in which the mask could be incorrectly set to zero, resulting in
>>>>> wrong-code.
>>>>>
>>>>> The mask was broken when nunits==32. The patched version will probably
>>>>> be broken for nunits==64, but I don't think any current targets have
>>>>> masks with more than 64 bits.
>>>>>
>>>>> OK for mainline?
>>>>>
>>>>> Andrew
>>>>>
>>>>> gcc/ChangeLog:
>>>>>
>>>>> 	* expr.cc (store_constructor): Use 64-bit shifts.
>>>>
>>>> No, this isn't 64-bit shift on all hosts.
>>>> Use HOST_WIDE_INT_1U instead.
>>>
>>> OK, I did wonder if there was a proper way to do it. :)
>>>
>>> How about this?
>>
>> If you change the other two GEN_INT ((1 << nunits) - 1) occurrences in
>> expr.cc the same way, then LGTM.
> 
> There's also two in dojump.cc

This patch should fix all the cases, I think.

I have not observed any further test result changes.

OK?

Andrew

[-- Attachment #2: gcc.patch --]
[-- Type: text/plain, Size: 2118 bytes --]

vect: Fix integer overflow calculating mask

The masks and bitvectors were broken when nunits==32 on hosts where int is
32-bit.

gcc/ChangeLog:

	* dojump.cc (do_compare_and_jump): Use full-width integers for shifts.
	* expr.cc (store_constructor): Likewise.
	(do_store_flag): Likewise.

diff --git a/gcc/dojump.cc b/gcc/dojump.cc
index ac744e54cf8..88600cb42d3 100644
--- a/gcc/dojump.cc
+++ b/gcc/dojump.cc
@@ -1318,10 +1318,10 @@ do_compare_and_jump (tree treeop0, tree treeop1, enum rtx_code signed_code,
     {
       gcc_assert (code == EQ || code == NE);
       op0 = expand_binop (mode, and_optab, op0,
-			  GEN_INT ((1 << nunits) - 1), NULL_RTX,
+			  GEN_INT ((HOST_WIDE_INT_1U << nunits) - 1), NULL_RTX,
 			  true, OPTAB_WIDEN);
       op1 = expand_binop (mode, and_optab, op1,
-			  GEN_INT ((1 << nunits) - 1), NULL_RTX,
+			  GEN_INT ((HOST_WIDE_INT_1U << nunits) - 1), NULL_RTX,
 			  true, OPTAB_WIDEN);
     }
 
diff --git a/gcc/expr.cc b/gcc/expr.cc
index 8d34d024c9c..f7d74525c15 100644
--- a/gcc/expr.cc
+++ b/gcc/expr.cc
@@ -7879,8 +7879,8 @@ store_constructor (tree exp, rtx target, int cleared, poly_int64 size,
 	    auto nunits = TYPE_VECTOR_SUBPARTS (type).to_constant ();
 	    if (maybe_ne (GET_MODE_PRECISION (mode), nunits))
 	      tmp = expand_binop (mode, and_optab, tmp,
-				  GEN_INT ((1 << nunits) - 1), target,
-				  true, OPTAB_WIDEN);
+				  GEN_INT ((HOST_WIDE_INT_1U << nunits) - 1),
+				  target, true, OPTAB_WIDEN);
 	    if (tmp != target)
 	      emit_move_insn (target, tmp);
 	    break;
@@ -13707,11 +13707,11 @@ do_store_flag (sepops ops, rtx target, machine_mode mode)
     {
       gcc_assert (code == EQ || code == NE);
       op0 = expand_binop (mode, and_optab, op0,
-			  GEN_INT ((1 << nunits) - 1), NULL_RTX,
-			  true, OPTAB_WIDEN);
+			  GEN_INT ((HOST_WIDE_INT_1U << nunits) - 1),
+			  NULL_RTX, true, OPTAB_WIDEN);
       op1 = expand_binop (mode, and_optab, op1,
-			  GEN_INT ((1 << nunits) - 1), NULL_RTX,
-			  true, OPTAB_WIDEN);
+			  GEN_INT ((HOST_WIDE_INT_1U << nunits) - 1),
+			  NULL_RTX, true, OPTAB_WIDEN);
     }
 
   if (target == 0)

Re: [PATCH] vect: Fix integer overflow calculating mask

[Jakub Jelinek]

On Mon, Mar 04, 2024 at 03:30:01PM +0000, Andrew Stubbs wrote:
> vect: Fix integer overflow calculating mask
> 
> The masks and bitvectors were broken when nunits==32 on hosts where int is
> 32-bit.
> 
> gcc/ChangeLog:
> 
> 	* dojump.cc (do_compare_and_jump): Use full-width integers for shifts.
> 	* expr.cc (store_constructor): Likewise.
> 	(do_store_flag): Likewise.

LGTM, thanks.

	Jakub