| Message ID | a90d0b0e-c605-747e-1a47-572a59fb2977@vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [rs6000] Fix PR83969: ICE in final_scan_insn, at final.c:2997 | expand |
On 3/7/18 6:50 PM, Peter Bergner wrote: > This passed bootstrap and regtesting on powerpc64-linux, running the > testsuite in both 32-bit and 64-bit modes with no regressions. > Ok for trunk? FYI, this also passed bootstrap and regtesting on powerpc64le-linux without regressions also. Peter
On Wed, Mar 07, 2018 at 06:50:41PM -0600, Peter Bergner wrote: > PR83969 shows another bug in mem_operand_gpr() (which implements the "Y" > constraint) accepting reg+reg addresses. This was fixed by adding a call > to rs6000_offsettable_memref_p() to verify the address is a valid offsettable > address. Fixing that exposed a problem in the *movdi_internal64 pattern > which was using the "Y" constraint for the integer load/store alternatives, > which allow either reg+offset or reg+reg addresses. This worked before > only because of the buggy mem_operand_gpr. The fix for that was to use > the "YZ" constraint which allows both reg+offset and reg+reg addresses. > > This passed bootstrap and regtesting on powerpc64-linux, running the > testsuite in both 32-bit and 64-bit modes with no regressions. > Ok for trunk? Sorry this took a while to review. It is okay for trunk. Does this need backports? Thanks! Segher > gcc/ > PR target/83969 > * config/rs6000/rs6000.c (rs6000_offsettable_memref_p): New prototype. > Add strict argument and use it. > (rs6000_split_multireg_move): Update for new strict argument. > (mem_operand_gpr): Disallow all non-offsettable addresses. > * config/rs6000/rs6000.md (*movdi_internal64): Use YZ constraint. > > gcc/testsuite/ > PR target/83969 > * gcc.target/powerpc/pr83969.c: New test.
On 3/9/18 1:31 PM, Segher Boessenkool wrote: > On Wed, Mar 07, 2018 at 06:50:41PM -0600, Peter Bergner wrote: >> This passed bootstrap and regtesting on powerpc64-linux, running the >> testsuite in both 32-bit and 64-bit modes with no regressions. >> Ok for trunk? > > Sorry this took a while to review. It is okay for trunk. Does this > need backports? Technically, it is broken there too, but until trunk, we never really generated the altivec mems that trigger this bug, so I think I would lean towards just having this on trunk and if someone, somehow hits it, then we can back port it then. Peter
On 3/9/18 4:25 PM, Peter Bergner wrote: > On 3/9/18 1:31 PM, Segher Boessenkool wrote: >> On Wed, Mar 07, 2018 at 06:50:41PM -0600, Peter Bergner wrote: >>> This passed bootstrap and regtesting on powerpc64-linux, running the >>> testsuite in both 32-bit and 64-bit modes with no regressions. >>> Ok for trunk? >> >> Sorry this took a while to review. It is okay for trunk. Does this >> need backports? > > Technically, it is broken there too, but until trunk, we never really > generated the altivec mems that trigger this bug, so I think I would > lean towards just having this on trunk and if someone, somehow hits > it, then we can back port it then. So as we talked offline, the go test case in PR85436 is fixed by this patch, so I have back ported it and am bootstrapping and regtesting it. Is it ok for GCC 7 if the testing comes back clean? Peter
On Thu, Apr 19, 2018 at 01:23:51PM -0500, Peter Bergner wrote: > On 3/9/18 4:25 PM, Peter Bergner wrote: > > On 3/9/18 1:31 PM, Segher Boessenkool wrote: > >> On Wed, Mar 07, 2018 at 06:50:41PM -0600, Peter Bergner wrote: > >>> This passed bootstrap and regtesting on powerpc64-linux, running the > >>> testsuite in both 32-bit and 64-bit modes with no regressions. > >>> Ok for trunk? > >> > >> Sorry this took a while to review. It is okay for trunk. Does this > >> need backports? > > > > Technically, it is broken there too, but until trunk, we never really > > generated the altivec mems that trigger this bug, so I think I would > > lean towards just having this on trunk and if someone, somehow hits > > it, then we can back port it then. > > So as we talked offline, the go test case in PR85436 is fixed by this > patch, so I have back ported it and am bootstrapping and regtesting it. > Is it ok for GCC 7 if the testing comes back clean? Certainly, thanks. Is it needed for GCC 6 as well? Okay for that too, if so. Segher
On 4/19/18 5:40 PM, Segher Boessenkool wrote: > On Thu, Apr 19, 2018 at 01:23:51PM -0500, Peter Bergner wrote: >> On 3/9/18 4:25 PM, Peter Bergner wrote: >>> Technically, it is broken there too, but until trunk, we never really >>> generated the altivec mems that trigger this bug, so I think I would >>> lean towards just having this on trunk and if someone, somehow hits >>> it, then we can back port it then. >> >> So as we talked offline, the go test case in PR85436 is fixed by this >> patch, so I have back ported it and am bootstrapping and regtesting it. >> Is it ok for GCC 7 if the testing comes back clean? > > Certainly, thanks. Is it needed for GCC 6 as well? Okay for that too, > if so. Ok, the following is what I ended up committing. Thanks. Neither test case fails on GCC 6, but I haven't tested on BE which is where the PR83969 test case was failing (-m32 BE). I'll do a build on BE and verify whether the tests compile there or not. If they PASS, I'd probably just leave it alone until someone comes up with a test case that FAILs on GCC 6 before back porting it. Peter gcc/ Backport from mainline 2018-03-09 Peter Bergner <bergner@vnet.ibm.com> PR target/83969 * config/rs6000/rs6000.c (rs6000_offsettable_memref_p): New prototype. Add strict argument and use it. (rs6000_split_multireg_move): Update for new strict argument. (mem_operand_gpr): Disallow all non-offsettable addresses. * config/rs6000/rs6000.md (*movdi_internal64): Use YZ constraint. gcc/testsuite/ PR target/85436 * go.dg/pr85436.go: New test. Backport from mainline 2018-03-09 Peter Bergner <bergner@vnet.ibm.com> PR target/83969 * gcc.target/powerpc/pr83969.c: New test. Index: gcc/config/rs6000/rs6000.c =================================================================== --- gcc/config/rs6000/rs6000.c (revision 259519) +++ gcc/config/rs6000/rs6000.c (working copy) @@ -1372,6 +1372,7 @@ static rtx rs6000_debug_legitimize_reloa int, int, int *); static bool rs6000_mode_dependent_address (const_rtx); static bool rs6000_debug_mode_dependent_address (const_rtx); +static bool rs6000_offsettable_memref_p (rtx, machine_mode, bool); static enum reg_class rs6000_secondary_reload_class (enum reg_class, machine_mode, rtx); static enum reg_class rs6000_debug_secondary_reload_class (enum reg_class, @@ -8564,10 +8565,8 @@ mem_operand_gpr (rtx op, machine_mode mo int extra; rtx addr = XEXP (op, 0); - /* Don't allow altivec type addresses like (mem (and (plus ...))). - See PR target/84279. */ - - if (GET_CODE (addr) == AND) + /* Don't allow non-offsettable addresses. See PRs 83969 and 84279. */ + if (!rs6000_offsettable_memref_p (op, mode, false)) return false; op = address_offset (addr); @@ -10340,7 +10339,7 @@ rs6000_find_base_term (rtx op) in 32-bit mode, that the recog predicate rejects. */ static bool -rs6000_offsettable_memref_p (rtx op, machine_mode reg_mode) +rs6000_offsettable_memref_p (rtx op, machine_mode reg_mode, bool strict) { bool worst_case; @@ -10348,7 +10347,7 @@ rs6000_offsettable_memref_p (rtx op, mac return false; /* First mimic offsettable_memref_p. */ - if (offsettable_address_p (true, GET_MODE (op), XEXP (op, 0))) + if (offsettable_address_p (strict, GET_MODE (op), XEXP (op, 0))) return true; /* offsettable_address_p invokes rs6000_mode_dependent_address, but @@ -10362,7 +10361,7 @@ rs6000_offsettable_memref_p (rtx op, mac worst_case = ((TARGET_POWERPC64 && GET_MODE_CLASS (reg_mode) == MODE_INT) || GET_MODE_SIZE (reg_mode) == 4); return rs6000_legitimate_offset_address_p (GET_MODE (op), XEXP (op, 0), - true, worst_case); + strict, worst_case); } /* Determine the reassociation width to be used in reassociate_bb. @@ -26559,7 +26558,7 @@ rs6000_split_multireg_move (rtx dst, rtx emit_insn (gen_add3_insn (breg, breg, delta_rtx)); src = replace_equiv_address (src, breg); } - else if (! rs6000_offsettable_memref_p (src, reg_mode)) + else if (! rs6000_offsettable_memref_p (src, reg_mode, true)) { if (GET_CODE (XEXP (src, 0)) == PRE_MODIFY) { @@ -26626,7 +26625,7 @@ rs6000_split_multireg_move (rtx dst, rtx emit_insn (gen_add3_insn (breg, breg, delta_rtx)); dst = replace_equiv_address (dst, breg); } - else if (!rs6000_offsettable_memref_p (dst, reg_mode) + else if (!rs6000_offsettable_memref_p (dst, reg_mode, true) && GET_CODE (XEXP (dst, 0)) != LO_SUM) { if (GET_CODE (XEXP (dst, 0)) == PRE_MODIFY) @@ -26665,7 +26664,7 @@ rs6000_split_multireg_move (rtx dst, rtx } } else if (GET_CODE (XEXP (dst, 0)) != LO_SUM) - gcc_assert (rs6000_offsettable_memref_p (dst, reg_mode)); + gcc_assert (rs6000_offsettable_memref_p (dst, reg_mode, true)); } for (i = 0; i < nregs; i++) Index: gcc/config/rs6000/rs6000.md =================================================================== --- gcc/config/rs6000/rs6000.md (revision 259519) +++ gcc/config/rs6000/rs6000.md (working copy) @@ -8640,14 +8640,14 @@ (define_split ;; FPR->GPR GPR->FPR VSX->GPR GPR->VSX (define_insn "*movdi_internal64" [(set (match_operand:DI 0 "nonimmediate_operand" - "=Y, r, r, r, r, r, + "=YZ, r, r, r, r, r, ^m, ^d, ^d, ^wY, $Z, $wb, $wv, ^wi, *wo, *wo, *wv, *wi, *wi, *wv, *wv, r, *h, *h, ?*r, ?*wg, ?*r, ?*wj") (match_operand:DI 1 "input_operand" - "r, Y, r, I, L, nF, + "r, YZ, r, I, L, nF, d, m, d, wb, wv, wY, Z, wi, Oj, wM, OjwM, Oj, wM, wS, wB, *h, r, 0, Index: gcc/testsuite/gcc.target/powerpc/pr83969.c =================================================================== --- gcc/testsuite/gcc.target/powerpc/pr83969.c (nonexistent) +++ gcc/testsuite/gcc.target/powerpc/pr83969.c (working copy) @@ -0,0 +1,14 @@ +/* { dg-do compile { target { powerpc*-*-* } } } */ +/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=G5" } } */ +/* { dg-options "-O1 -mcpu=G5 -fno-split-wide-types -ftree-loop-vectorize" } */ + +long long int +n7 (int po, long long int r4) +{ + while (po < 1) + { + r4 |= 1; + ++po; + } + return r4; +} Index: gcc/testsuite/go.dg/pr85436.go =================================================================== --- gcc/testsuite/go.dg/pr85436.go (nonexistent) +++ gcc/testsuite/go.dg/pr85436.go (working copy) @@ -0,0 +1,21 @@ +/* { dg-do compile } */ +/* { dg-options "-O3 -mcpu=power9" { target { powerpc*-*-* } } } */ + +package main +import ( + "go/ast" + "go/parser" + "go/token" +) +type testFuncs struct { } +func (t *testFuncs) load(filename, pkg string, doImport, seen *bool) { + var testFileSet = token.NewFileSet() + f, err := parser.ParseFile(testFileSet, filename, nil, parser.ParseComments) + if err != nil { } + for _, d := range f.Decls { + n, ok := d.(*ast.FuncDecl) + if !ok { } + ptr := n.Type.Params.List[0].Type.(*ast.StarExpr) + if sel := ptr.X.(*ast.SelectorExpr); sel.Sel.Name == "M" { } + } +}
On 4/20/18 9:34 AM, Peter Bergner wrote: > Neither test case fails on GCC 6, but I haven't tested on BE which is > where the PR83969 test case was failing (-m32 BE). I'll do a build on > BE and verify whether the tests compile there or not. If they PASS, > I'd probably just leave it alone until someone comes up with a test > case that FAILs on GCC 6 before back porting it. OK, GCC 6 on BE doesn't ICE on either test case, so I'm going to just leave things as they are there. We can always back port the fixes later if someone comes up with yet another test case that fails there. Peter
Index: gcc/config/rs6000/rs6000.c =================================================================== --- gcc/config/rs6000/rs6000.c (revision 258268) +++ gcc/config/rs6000/rs6000.c (working copy) @@ -1378,6 +1378,7 @@ static rtx rs6000_debug_legitimize_reloa int, int, int *); static bool rs6000_mode_dependent_address (const_rtx); static bool rs6000_debug_mode_dependent_address (const_rtx); +static bool rs6000_offsettable_memref_p (rtx, machine_mode, bool); static enum reg_class rs6000_secondary_reload_class (enum reg_class, machine_mode, rtx); static enum reg_class rs6000_debug_secondary_reload_class (enum reg_class, @@ -8207,10 +8208,8 @@ mem_operand_gpr (rtx op, machine_mode mo int extra; rtx addr = XEXP (op, 0); - /* Don't allow altivec type addresses like (mem (and (plus ...))). - See PR target/84279. */ - - if (GET_CODE (addr) == AND) + /* Don't allow non-offsettable addresses. See PRs 83969 and 84279. */ + if (!rs6000_offsettable_memref_p (op, mode, false)) return false; op = address_offset (addr); @@ -9956,7 +9955,7 @@ rs6000_find_base_term (rtx op) in 32-bit mode, that the recog predicate rejects. */ static bool -rs6000_offsettable_memref_p (rtx op, machine_mode reg_mode) +rs6000_offsettable_memref_p (rtx op, machine_mode reg_mode, bool strict) { bool worst_case; @@ -9964,7 +9963,7 @@ rs6000_offsettable_memref_p (rtx op, mac return false; /* First mimic offsettable_memref_p. */ - if (offsettable_address_p (true, GET_MODE (op), XEXP (op, 0))) + if (offsettable_address_p (strict, GET_MODE (op), XEXP (op, 0))) return true; /* offsettable_address_p invokes rs6000_mode_dependent_address, but @@ -9978,7 +9977,7 @@ rs6000_offsettable_memref_p (rtx op, mac worst_case = ((TARGET_POWERPC64 && GET_MODE_CLASS (reg_mode) == MODE_INT) || GET_MODE_SIZE (reg_mode) == 4); return rs6000_legitimate_offset_address_p (GET_MODE (op), XEXP (op, 0), - true, worst_case); + strict, worst_case); } /* Determine the reassociation width to be used in reassociate_bb. @@ -24063,7 +24062,7 @@ rs6000_split_multireg_move (rtx dst, rtx emit_insn (gen_add3_insn (breg, breg, delta_rtx)); src = replace_equiv_address (src, breg); } - else if (! rs6000_offsettable_memref_p (src, reg_mode)) + else if (! rs6000_offsettable_memref_p (src, reg_mode, true)) { if (GET_CODE (XEXP (src, 0)) == PRE_MODIFY) { @@ -24130,7 +24129,7 @@ rs6000_split_multireg_move (rtx dst, rtx emit_insn (gen_add3_insn (breg, breg, delta_rtx)); dst = replace_equiv_address (dst, breg); } - else if (!rs6000_offsettable_memref_p (dst, reg_mode) + else if (!rs6000_offsettable_memref_p (dst, reg_mode, true) && GET_CODE (XEXP (dst, 0)) != LO_SUM) { if (GET_CODE (XEXP (dst, 0)) == PRE_MODIFY) @@ -24169,7 +24168,7 @@ rs6000_split_multireg_move (rtx dst, rtx } } else if (GET_CODE (XEXP (dst, 0)) != LO_SUM) - gcc_assert (rs6000_offsettable_memref_p (dst, reg_mode)); + gcc_assert (rs6000_offsettable_memref_p (dst, reg_mode, true)); } for (i = 0; i < nregs; i++) Index: gcc/config/rs6000/rs6000.md =================================================================== --- gcc/config/rs6000/rs6000.md (revision 258268) +++ gcc/config/rs6000/rs6000.md (working copy) @@ -8549,14 +8549,14 @@ (define_split ;; FPR->GPR GPR->FPR VSX->GPR GPR->VSX (define_insn "*movdi_internal64" [(set (match_operand:DI 0 "nonimmediate_operand" - "=Y, r, r, r, r, r, + "=YZ, r, r, r, r, r, ^m, ^d, ^d, ^wY, $Z, $wb, $wv, ^wi, *wo, *wo, *wv, *wi, *wi, *wv, *wv, r, *h, *h, ?*r, ?*wg, ?*r, ?*wj") (match_operand:DI 1 "input_operand" - "r, Y, r, I, L, nF, + "r, YZ, r, I, L, nF, d, m, d, wb, wv, wY, Z, wi, Oj, wM, OjwM, Oj, wM, wS, wB, *h, r, 0, Index: gcc/testsuite/gcc.target/powerpc/pr83969.c =================================================================== --- gcc/testsuite/gcc.target/powerpc/pr83969.c (revision 0) +++ gcc/testsuite/gcc.target/powerpc/pr83969.c (working copy) @@ -0,0 +1,14 @@ +/* { dg-do compile { target { powerpc*-*-* } } } */ +/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=G5" } } */ +/* { dg-options "-O1 -mcpu=G5 -fno-split-wide-types -ftree-loop-vectorize" } */ + +long long int +n7 (int po, long long int r4) +{ + while (po < 1) + { + r4 |= 1; + ++po; + } + return r4;