Re: [PATCH] vect: Fix integer overflow calculating mask
On Mon, Mar 04, 2024 at 03:30:01PM +, 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
Re: [PATCH] vect: Fix integer overflow calculating mask
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 +, Andrew Stubbs wrote: On 23/02/2024 13:02, Jakub Jelinek wrote: On Fri, Feb 23, 2024 at 12:58:53PM +, 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 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
On Fri, 23 Feb 2024, Jakub Jelinek wrote: > On Fri, Feb 23, 2024 at 02:22:19PM +, Andrew Stubbs wrote: > > On 23/02/2024 13:02, Jakub Jelinek wrote: > > > On Fri, Feb 23, 2024 at 12:58:53PM +, 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
On Fri, Feb 23, 2024 at 02:22:19PM +, Andrew Stubbs wrote: > On 23/02/2024 13:02, Jakub Jelinek wrote: > > On Fri, Feb 23, 2024 at 12:58:53PM +, 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
On 23/02/2024 13:02, Jakub Jelinek wrote: On Fri, Feb 23, 2024 at 12:58:53PM +, 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 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 e23880e..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
> Am 23.02.2024 um 14:03 schrieb Jakub Jelinek : > > On Fri, Feb 23, 2024 at 12:58:53PM +, 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
On Fri, Feb 23, 2024 at 12:58:53PM +, 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
[PATCH] vect: Fix integer overflow calculating mask
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 e23880e..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