Re: [PATCH resend] rs6000, pr 94833: fix vec_first_match_index for nulls
Hi Carl, On Wed, Jun 10, 2020 at 09:05:36AM -0700, Carl Love wrote: > I committed this patch to mainline and backported to GCC 9. > > I have looked at GCC 8. The functional issue is there, i.e. the > vcmpnez is used instead of vcmpne. However the test case > builtins-8-p9-runnable.c does not exist in GCC 8. The patch consists > of the functional fix: > > --- a/gcc/config/rs6000/vsx.md > +++ b/gcc/config/rs6000/vsx.md > @@ -4803,8 +4803,8 @@ >rtx cmp_result = gen_reg_rtx (mode); >rtx not_result = gen_reg_rtx (mode); > > - emit_insn (gen_vcmpnez (cmp_result, operands[1], > -operands[2])); > + emit_insn (gen_vcmpne (cmp_result, operands[1], > + operands[2])); >emit_insn (gen_one_cmpl2 (not_result, cmp_result)); > >sh = GET_MODE_SIZE (GET_MODE_INNER (mode)) / 2; > > So, I am a bit unsure how to proceed. I think we need the functional > change. But without applying the test cases fixes I don't feel that I > am really backporting the patch as approved for backporting. > > I think we could reference the mainline commit, as we always do when > backporting, but then note the test case fix was not included since the > testcase does not exist. Would that be OK? Yes, that is fine, this fix is obvious enough :-) You could also include the whole testcase from trunk (or 9), but that can be quite a bit of (testing) work, and is it worth it at all. > Please let me know how you would like me to handle this issue. Either way is okay (I'd go for just the 2-line patch). Thanks, Segher
RE: [PATCH resend] rs6000, pr 94833: fix vec_first_match_index for nulls
Segher, Bill: I committed this patch to mainline and backported to GCC 9. I have looked at GCC 8. The functional issue is there, i.e. the vcmpnez is used instead of vcmpne. However the test case builtins-8-p9-runnable.c does not exist in GCC 8. The patch consists of the functional fix: --- a/gcc/config/rs6000/vsx.md +++ b/gcc/config/rs6000/vsx.md @@ -4803,8 +4803,8 @@ rtx cmp_result = gen_reg_rtx (mode); rtx not_result = gen_reg_rtx (mode); - emit_insn (gen_vcmpnez (cmp_result, operands[1], -operands[2])); + emit_insn (gen_vcmpne (cmp_result, operands[1], + operands[2])); emit_insn (gen_one_cmpl2 (not_result, cmp_result)); sh = GET_MODE_SIZE (GET_MODE_INNER (mode)) / 2; So, I am a bit unsure how to proceed. I think we need the functional change. But without applying the test cases fixes I don't feel that I am really backporting the patch as approved for backporting. I think we could reference the mainline commit, as we always do when backporting, but then note the test case fix was not included since the testcase does not exist. Would that be OK? Please let me know how you would like me to handle this issue. Carl Love On Thu, 2020-05-14 at 11:53 -0500, Segher Boessenkool wrote: > Hi! > > On Wed, May 13, 2020 at 10:14:24AM -0700, Carl Love wrote: > > The following patch fixes PR94833, vec_first_match_index does not > > function as described in its description. > > > > The builtin does not handle vector elements which are zero > > correctly. > > The following patch fixes the issue and adds additional test cases > > to > > verify the vec_first_match_index builtin and related builtins work > > correctly with elements that are zero. > > 2020-04-30 Carl Love > > > > PR target/94833 > > * config/rs6000/vsx.md (define_expand): Fix instruction > > generation for > > first_match_index_. > > * testsuite/gcc.target/powerpc/builtins-8-p9-runnable.c (main): > > Add > > additional test cases with zero vector elements. > > Okay for trunk and all backports needed after a while. Thanks! > > > Segher
Re: [PATCH resend] rs6000, pr 94833: fix vec_first_match_index for nulls
Hi! On Wed, May 13, 2020 at 10:14:24AM -0700, Carl Love wrote: > The following patch fixes PR94833, vec_first_match_index does not > function as described in its description. > > The builtin does not handle vector elements which are zero correctly. > The following patch fixes the issue and adds additional test cases to > verify the vec_first_match_index builtin and related builtins work > correctly with elements that are zero. > 2020-04-30 Carl Love > > PR target/94833 > * config/rs6000/vsx.md (define_expand): Fix instruction generation for > first_match_index_. > * testsuite/gcc.target/powerpc/builtins-8-p9-runnable.c (main): Add > additional test cases with zero vector elements. Okay for trunk and all backports needed after a while. Thanks! Segher
[PATCH resend] rs6000, pr 94833: fix vec_first_match_index for nulls
GCC maintainers:
This is a resend of "[PATCH]rs6000, fix vec_first_match_index for
nulls".
Per the received comments the pr number was added to the subject line.
I also tweaked the message to make it clear that the patch fixed issues
with vectors whose elements contain zeros rather then a zero length
vector.
-
The following patch fixes PR94833, vec_first_match_index does not
function as described in its description.
The builtin does not handle vector elements which are zero correctly.
The following patch fixes the issue and adds additional test cases to
verify the vec_first_match_index builtin and related builtins work
correctly with elements that are zero.
The patch has been compiled and tested on
powerpc64le-unknown-linux-gnu (Power 9 LE)
with no regression errors.
Please let me know if the patch is acceptable for mainline and for
backporting as needed.
Thanks.
Carl Love
--
gcc/ChangeLog
2020-04-30 Carl Love
PR target/94833
* config/rs6000/vsx.md (define_expand): Fix instruction generation for
first_match_index_.
* testsuite/gcc.target/powerpc/builtins-8-p9-runnable.c (main): Add
additional test cases with zero vector elements.
---
gcc/config/rs6000/vsx.md | 4 +-
.../powerpc/builtins-8-p9-runnable.c | 118 ++
2 files changed, 120 insertions(+), 2 deletions(-)
diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md
index 1fcc1b03096..12a0d5e668c 100644
--- a/gcc/config/rs6000/vsx.md
+++ b/gcc/config/rs6000/vsx.md
@@ -4803,8 +4803,8 @@
rtx cmp_result = gen_reg_rtx (mode);
rtx not_result = gen_reg_rtx (mode);
- emit_insn (gen_vcmpnez (cmp_result, operands[1],
-operands[2]));
+ emit_insn (gen_vcmpne (cmp_result, operands[1],
+ operands[2]));
emit_insn (gen_one_cmpl2 (not_result, cmp_result));
sh = GET_MODE_SIZE (GET_MODE_INNER (mode)) / 2;
diff --git a/gcc/testsuite/gcc.target/powerpc/builtins-8-p9-runnable.c
b/gcc/testsuite/gcc.target/powerpc/builtins-8-p9-runnable.c
index b2f7dc855e8..19457eebfc4 100644
--- a/gcc/testsuite/gcc.target/powerpc/builtins-8-p9-runnable.c
+++ b/gcc/testsuite/gcc.target/powerpc/builtins-8-p9-runnable.c
@@ -103,6 +103,31 @@ int main() {
The element index in natural element order is returned for the
first match or the number of elements if there is no match. */
/* char */
+ char_src1 = (vector signed char) { 0x40, 0, 0x40, 0x40,
+0x40, 0x40, 0x40, 0x40,
+0x40, 0x40, 0x40, 0x40,
+0x40, 0x40, 0x40, 0x40 };
+
+ char_src2 = (vector signed char) {0, 0, 0, 0, 0, 0, 0, 0,
+ 0, 0, 0, 0, 0, 0, 0, 0};
+ expected_result = 1;
+
+ result = vec_first_match_index (char_src1, char_src2);
+
+#ifdef DEBUG2
+ print_signed_char("src1", char_src1);
+ print_signed_char("src2", char_src2);
+ printf(" vec_first_match_index = %d\n\n", result);
+#endif
+
+ if (result != expected_result)
+#ifdef DEBUG
+printf("Error: char first match result (%d) does not match expected result
(%d)\n",
+ result, expected_result);
+#else
+abort();
+#endif
+
char_src1 = (vector signed char) {-1, 2, 3, 4, -5, 6, 7, 8,
9, 10, 11, 12, 13, 14, 15, 16};
char_src2 = (vector signed char) {-1, 2, 3, 20, -5, 6, 7, 8,
@@ -367,6 +392,50 @@ int main() {
The element index in BE order is returned for the first mismatch
or the number of elements if there is no match. */
/* char */
+ char_src1 = (vector signed char) {1, 2, 0, 4, -5, 6, 7, 8,
+ 9, 10, 11, 12, 13, 14, 15, 16};
+ char_src2 = (vector signed char) {1, 2, 0, 20, -5, 6, 7, 8,
+ 9, 10, 11, 12, 13, 14, 15, 16};
+ expected_result = 3;
+
+ result = vec_first_mismatch_index (char_src1, char_src2);
+
+#ifdef DEBUG2
+ print_signed_char("src1", char_src1);
+ print_signed_char("src2", char_src2);
+ printf("vec_first_mismatch_index = %d\n\n", result);
+#endif
+
+ if (result != expected_result)
+#ifdef DEBUG
+printf("Error: char first mismatch result (%d) does not match expected
result (%d)\n",
+ result, expected_result);
+#else
+abort();
+#endif
+
+ char_src1 = (vector signed char) {0, 2, 3, 4, -5, 6, 7, 8,
+ 9, 10, 11, 12, 13, 14, 15, 16};
+ char_src2 = (vector signed char) {0, 2, 3, 20, -5, 6, 7, 8,
+ 9, 10, 11, 12, 13, 14, 15, 16};
+ expected_result = 3;
+
+ result = vec_first_mismatch_index (char_src1, char_src2);
+
+#ifdef DEBUG2
+ print_signed_char("src1", char_src1);
+ print_signed_char("src2", char_src2);
+
