| Message ID | 20210426113855.3291810-1-cmuellner@gcc.gnu.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] REE: PR rtl-optimization/100264: Handle more PARALLEL SET expressions | expand |
On 4/26/2021 5:38 AM, Christoph Muellner via Gcc-patches wrote: > [ree] PR rtl-optimization/100264: Handle more PARALLEL SET expressions > > PR rtl-optimization/100264 > * ree.c (get_sub_rtx): Ignore SET expressions without register > destinations. > (merge_def_and_ext): Eliminate destination check for register > as such SET expressions can't occur anymore. > (combine_reaching_defs): Likewise. This is pretty sensible. Do you have commit privs for GCC? Any interest in further REE work? I've stumbled across a case where we could/should eliminate a useless extension, but don't. It shows up with an internal port, but I bet I could show it with riscv as well. Interested? jeff
On Sat, May 1, 2021 at 12:48 AM Jeff Law <jeffreyalaw@gmail.com> wrote: > > > On 4/26/2021 5:38 AM, Christoph Muellner via Gcc-patches wrote: > > [ree] PR rtl-optimization/100264: Handle more PARALLEL SET expressions > > > > PR rtl-optimization/100264 > > * ree.c (get_sub_rtx): Ignore SET expressions without register > > destinations. > > (merge_def_and_ext): Eliminate destination check for register > > as such SET expressions can't occur anymore. > > (combine_reaching_defs): Likewise. > > This is pretty sensible. Do you have commit privs for GCC? No, I don't have write access yet. > Any interest in further REE work? I've stumbled across a case where we > could/should eliminate a useless extension, but don't. It shows up with > an internal port, but I bet I could show it with riscv as well. Interested? If I am interested in eliminating useless extensions? Always! Is there a PR for it? I'm curious if my skill level is high enough to fix it...
On Fri, Apr 30, 2021 at 4:10 PM Christoph Müllner via Gcc-patches < gcc-patches@gcc.gnu.org> wrote: > On Sat, May 1, 2021 at 12:48 AM Jeff Law <jeffreyalaw@gmail.com> wrote: > > On 4/26/2021 5:38 AM, Christoph Muellner via Gcc-patches wrote: > > > [ree] PR rtl-optimization/100264: Handle more PARALLEL SET expressions > > > > > > PR rtl-optimization/100264 > > > * ree.c (get_sub_rtx): Ignore SET expressions without register > > > destinations. > > > (merge_def_and_ext): Eliminate destination check for register > > > as such SET expressions can't occur anymore. > > > (combine_reaching_defs): Likewise. > > > > This is pretty sensible. Do you have commit privs for GCC? > This looks reasonable to me also. But I tried a build and check with an rv64gc/lp64d linux toolchain built from riscv-gnu-toolchain and I get two extra failures in the gfortran testsuite. /scratch/jimw/fsf-testing/patched/riscv-gcc/gcc/testsuite/gfortran.dg/typebound\ _operator_3.f03:93:21: internal compiler error: in get_sub_rtx, at ree.c:705^M 0x15664f8 get_sub_rtx^M ../../../patched/riscv-gcc/gcc/ree.c:705^M 0x15672ce merge_def_and_ext^M ../../../patched/riscv-gcc/gcc/ree.c:719^M 0x15672ce combine_reaching_defs^M ../../../patched/riscv-gcc/gcc/ree.c:1020^M 0x1568308 find_and_remove_re^M ../../../patched/riscv-gcc/gcc/ree.c:1319^M 0x1568308 rest_of_handle_ree^M ../../../patched/riscv-gcc/gcc/ree.c:1390^M 0x1568308 execute^M ../../../patched/riscv-gcc/gcc/ree.c:1418^M Please submit a full bug report,^M with preprocessed source if appropriate.^M Please include the complete backtrace with any bug report.^M See <https://gcc.gnu.org/bugs/> for instructions.^M compiler exited with status 1 FAIL: gfortran.dg/typebound_operator_3.f03 -Os (internal compiler error) FAIL: gfortran.dg/typebound_operator_3.f03 -Os (test for excess errors) Jim
On Thu, May 6, 2021 at 5:29 AM Jim Wilson <jimw@sifive.com> wrote: > > On Fri, Apr 30, 2021 at 4:10 PM Christoph Müllner via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: >> >> On Sat, May 1, 2021 at 12:48 AM Jeff Law <jeffreyalaw@gmail.com> wrote: >> > On 4/26/2021 5:38 AM, Christoph Muellner via Gcc-patches wrote: >> > > [ree] PR rtl-optimization/100264: Handle more PARALLEL SET expressions >> > > >> > > PR rtl-optimization/100264 >> > > * ree.c (get_sub_rtx): Ignore SET expressions without register >> > > destinations. >> > > (merge_def_and_ext): Eliminate destination check for register >> > > as such SET expressions can't occur anymore. >> > > (combine_reaching_defs): Likewise. >> > >> > This is pretty sensible. Do you have commit privs for GCC? > > > This looks reasonable to me also. But I tried a build and check with an rv64gc/lp64d linux toolchain built from riscv-gnu-toolchain and I get two extra failures in the gfortran testsuite. > > /scratch/jimw/fsf-testing/patched/riscv-gcc/gcc/testsuite/gfortran.dg/typebound\ > _operator_3.f03:93:21: internal compiler error: in get_sub_rtx, at ree.c:705^M > 0x15664f8 get_sub_rtx^M > ../../../patched/riscv-gcc/gcc/ree.c:705^M > 0x15672ce merge_def_and_ext^M > ../../../patched/riscv-gcc/gcc/ree.c:719^M > 0x15672ce combine_reaching_defs^M > ../../../patched/riscv-gcc/gcc/ree.c:1020^M > 0x1568308 find_and_remove_re^M > ../../../patched/riscv-gcc/gcc/ree.c:1319^M > 0x1568308 rest_of_handle_ree^M > ../../../patched/riscv-gcc/gcc/ree.c:1390^M > 0x1568308 execute^M > ../../../patched/riscv-gcc/gcc/ree.c:1418^M > Please submit a full bug report,^M > with preprocessed source if appropriate.^M > Please include the complete backtrace with any bug report.^M > See <https://gcc.gnu.org/bugs/> for instructions.^M > compiler exited with status 1 > FAIL: gfortran.dg/typebound_operator_3.f03 -Os (internal compiler error) > FAIL: gfortran.dg/typebound_operator_3.f03 -Os (test for excess errors) The issue comes from the assertion at the end of get_sub_rtx(). It is not valid anymore, because it is possible to have PARALLEL expressions without a single SET expression for a register. Without my patch, we would trigger this assertion if there would be a PARALLEL expression without any SET. My solution is to eliminate the assertion, as the function is supposed to return NULL in case no matching SET is found (even before my patch). This also shortens the whole function a bit because the logic for non-PARALLEL SETs can be simplified. Thanks, Christoph
diff --git a/gcc/ree.c b/gcc/ree.c index 65457c582c6a..1eaaaf9e1eb5 100644 --- a/gcc/ree.c +++ b/gcc/ree.c @@ -658,10 +658,11 @@ make_defs_and_copies_lists (rtx_insn *extend_insn, const_rtx set_pat, return ret; } -/* If DEF_INSN has single SET expression, possibly buried inside - a PARALLEL, return the address of the SET expression, else - return NULL. This is similar to single_set, except that - single_set allows multiple SETs when all but one is dead. */ +/* If DEF_INSN has single SET expression with a register + destination, possibly buried inside a PARALLEL, return + the address of the SET expression, else return NULL. + This is similar to single_set, except that single_set + allows multiple SETs when all but one is dead. */ static rtx * get_sub_rtx (rtx_insn *def_insn) { @@ -675,6 +676,8 @@ get_sub_rtx (rtx_insn *def_insn) rtx s_expr = XVECEXP (PATTERN (def_insn), 0, i); if (GET_CODE (s_expr) != SET) continue; + if (!REG_P (SET_DEST (s_expr))) + continue; if (sub_rtx == NULL) sub_rtx = &XVECEXP (PATTERN (def_insn), 0, i); @@ -686,7 +689,13 @@ get_sub_rtx (rtx_insn *def_insn) } } else if (code == SET) - sub_rtx = &PATTERN (def_insn); + { + rtx s_expr = PATTERN (def_insn); + if (!REG_P (SET_DEST (s_expr))) + return NULL; + + sub_rtx = &PATTERN (def_insn); + } else { /* It is not a PARALLEL or a SET, what could it be ? */ @@ -712,13 +721,12 @@ merge_def_and_ext (ext_cand *cand, rtx_insn *def_insn, ext_state *state) if (sub_rtx == NULL) return false; - if (REG_P (SET_DEST (*sub_rtx)) - && (GET_MODE (SET_DEST (*sub_rtx)) == ext_src_mode + if (GET_MODE (SET_DEST (*sub_rtx)) == ext_src_mode || ((state->modified[INSN_UID (def_insn)].kind == (cand->code == ZERO_EXTEND ? EXT_MODIFIED_ZEXT : EXT_MODIFIED_SEXT)) && state->modified[INSN_UID (def_insn)].mode - == ext_src_mode))) + == ext_src_mode)) { if (GET_MODE_UNIT_SIZE (GET_MODE (SET_DEST (*sub_rtx))) >= GET_MODE_UNIT_SIZE (cand->mode)) @@ -853,8 +861,7 @@ combine_reaching_defs (ext_cand *cand, const_rtx set_pat, ext_state *state) CAND->insn, then this transformation is not safe. Note we have to test in the widened mode. */ rtx *dest_sub_rtx = get_sub_rtx (def_insn); - if (dest_sub_rtx == NULL - || !REG_P (SET_DEST (*dest_sub_rtx))) + if (dest_sub_rtx == NULL) return false; rtx tmp_reg = gen_rtx_REG (GET_MODE (SET_DEST (set)), @@ -947,8 +954,7 @@ combine_reaching_defs (ext_cand *cand, const_rtx set_pat, ext_state *state) break; rtx *dest_sub_rtx2 = get_sub_rtx (def_insn2); - if (dest_sub_rtx2 == NULL - || !REG_P (SET_DEST (*dest_sub_rtx2))) + if (dest_sub_rtx2 == NULL) break; /* On RISC machines we must make sure that changing the mode of
[ree] PR rtl-optimization/100264: Handle more PARALLEL SET expressions PR rtl-optimization/100264 * ree.c (get_sub_rtx): Ignore SET expressions without register destinations. (merge_def_and_ext): Eliminate destination check for register as such SET expressions can't occur anymore. (combine_reaching_defs): Likewise. --- gcc/ree.c | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-)