Message ID | 5175D919.1070704@redhat.com |
---|---|
State | New |
Headers | show |
On Mon, Apr 22, 2013 at 8:43 PM, Vladimir Makarov <vmakarov@redhat.com> wrote: > * config/rs6000/rs6000.c (legitimate_lo_sum_address_p): Remove > lra_in_progress guard for addressing something bigger than word. That will work, but I'm worried that it is too fragile. Previously the code uniformly returned consistent values from the various legitimate address functions. Changing the response based on lra_in_progress for various modes seems like a problem waiting to happen. Thanks, David
On 04/22/2013 09:55 PM, David Edelsohn wrote: > On Mon, Apr 22, 2013 at 8:43 PM, Vladimir Makarov <vmakarov@redhat.com> wrote: > >> * config/rs6000/rs6000.c (legitimate_lo_sum_address_p): Remove >> lra_in_progress guard for addressing something bigger than word. > That will work, but I'm worried that it is too fragile. Previously > the code uniformly returned consistent values from the various > legitimate address functions. Changing the response based on > lra_in_progress for various modes seems like a problem waiting to > happen. > LRA approach is different from reload one. First of all LRA can create pseudos (not assigned yet to anything) and secondly LRA makes changes incrementally opposite to reload which makes all final rewriting code when it decides that a final solution is found. The difference in approaches means that LRA can not use all reload macros and hooks completely without changes. Although I tried to minimize the changes, they are still present. For example, LRA will never use hooks which call push_reload which is completely oriented to reload pass. I'd not say that "the code uniformly returned consistent values". One place where lra_in_progress is needed exactly because of discrepancy between legitimize reload address hook (which can call push_reload and can not be used by LRA) and legitimate address hook. Two other places in rs6000_emit_move for SDmode where lra_in_progress is used are because LRA can create and use spilled pseudos instead of using concrete memory slot as reload does. And two the rest places are in calls of legitimate_const_pool_address_p where LRA actually querying in strict sense. So I think the changes are minimal. David, thanks for expressing the concern. I hope I answered it. The future will show the reality not just our speculations.
On Tue, Apr 23, 2013 at 11:03 AM, Vladimir Makarov <vmakarov@redhat.com> wrote: > LRA approach is different from reload one. First of all LRA can create > pseudos (not assigned yet to anything) and secondly LRA makes changes > incrementally opposite to reload which makes all final rewriting code when > it decides that a final solution is found. The difference in approaches > means that LRA can not use all reload macros and hooks completely without > changes. Although I tried to minimize the changes, they are still present. > For example, LRA will never use hooks which call push_reload which is > completely oriented to reload pass. > > I'd not say that "the code uniformly returned consistent values". One > place where lra_in_progress is needed exactly because of discrepancy between > legitimize reload address hook (which can call push_reload and can not be > used by LRA) and legitimate address hook. Two other places in > rs6000_emit_move for SDmode where lra_in_progress is used are because LRA > can create and use spilled pseudos instead of using concrete memory slot as > reload does. And two the rest places are in calls of > legitimate_const_pool_address_p where LRA actually querying in strict sense. > So I think the changes are minimal. > > David, thanks for expressing the concern. I hope I answered it. The future > will show the reality not just our speculations. Vlad, I don't think that you understood my concern. I am not concerned about adding lra_in_progress. I am concerned about combining lra_in_progress with the mode. I would like an explanation why it is correct for the result of legitimate address functions to vary with lra_in_progress combined with the mode. Not simply that it works. The issue should be if an offsettable address is valid and the size of the offset. Thanks, David
On 04/23/2013 11:33 AM, David Edelsohn wrote: > On Tue, Apr 23, 2013 at 11:03 AM, Vladimir Makarov <vmakarov@redhat.com> wrote: > >> LRA approach is different from reload one. First of all LRA can create >> pseudos (not assigned yet to anything) and secondly LRA makes changes >> incrementally opposite to reload which makes all final rewriting code when >> it decides that a final solution is found. The difference in approaches >> means that LRA can not use all reload macros and hooks completely without >> changes. Although I tried to minimize the changes, they are still present. >> For example, LRA will never use hooks which call push_reload which is >> completely oriented to reload pass. >> >> I'd not say that "the code uniformly returned consistent values". One >> place where lra_in_progress is needed exactly because of discrepancy between >> legitimize reload address hook (which can call push_reload and can not be >> used by LRA) and legitimate address hook. Two other places in >> rs6000_emit_move for SDmode where lra_in_progress is used are because LRA >> can create and use spilled pseudos instead of using concrete memory slot as >> reload does. And two the rest places are in calls of >> legitimate_const_pool_address_p where LRA actually querying in strict sense. >> So I think the changes are minimal. >> >> David, thanks for expressing the concern. I hope I answered it. The future >> will show the reality not just our speculations. > > Vlad, > > I don't think that you understood my concern. I am not concerned > about adding lra_in_progress. I am concerned about combining > lra_in_progress with the mode. I would like an explanation why it is > correct for the result of legitimate address functions to vary with > lra_in_progress combined with the mode. Not simply that it works. > > The issue should be if an offsettable address is valid and the size of > the offset. > Sorry, may be I missed some other places but could you be more specific about the place where combining lra_in_progress and mode happens now for legitimate address querying. I guess I explained in my previous emails why the following change is necessary in legitimate_lo_sum_address_p: toc_ok_p = (lra_in_progress && TARGET_CMODEL != CMODEL_SMALL && small_toc_ref (x, VOIDmode)); lra_in_progress was removed in my latest change which now looks like the original code: if (GET_MODE_SIZE (mode) > UNITS_PER_WORD && !(/* ??? Assume floating point reg based on mode? */ TARGET_HARD_FLOAT && TARGET_FPRS && TARGET_DOUBLE_FLOAT && (mode == DFmode || mode == DDmode))) return false; So I can not see other places.
On Tue, Apr 23, 2013 at 11:37 AM, Vladimir Makarov <vmakarov@redhat.com> wrote: > Sorry, may be I missed some other places but > could you be more specific about the place where combining lra_in_progress > and mode happens now for legitimate address querying. > > I guess I explained in my previous emails why the following change is > necessary in legitimate_lo_sum_address_p: > > toc_ok_p = (lra_in_progress && TARGET_CMODEL != CMODEL_SMALL > && small_toc_ref (x, VOIDmode)); > > lra_in_progress was removed in my latest change which now looks like the > original code: > > if (GET_MODE_SIZE (mode) > UNITS_PER_WORD > > && !(/* ??? Assume floating point reg based on mode? */ > TARGET_HARD_FLOAT && TARGET_FPRS && TARGET_DOUBLE_FLOAT > && (mode == DFmode || mode == DDmode))) > return false; > > So I can not see other places. Okay. I misunderstood the change. I thought that you were adding lra_in_progress tied to the mode. Thanks, David
I'm seeing a lot of failures with these changes in make check. The first two that I noticed on a build that did not use --with-cpu=power7: 1) c-c++-common/dfp/call-by-value.c (and others in the directory) fails with -O0 for all targets before power7 because it can't spill SDmode. Note, in the earlier targets, we need to have a wider spill slot because we don't have 32-bit integer load/store to the FPR registers. FAIL: c-c++-common/dfp/call-by-value.c (internal compiler error) FAIL: c-c++-common/dfp/call-by-value.c (test for excess errors) Excess errors: /home/meissner/fsf-src/meissner-lra/gcc/testsuite/c-c++-common/dfp/call-by-value.c:43:1: internal compiler error: in assign_by_spills, at lra-assigns.c:1268 0x104ceff7 assign_by_spills /home/meissner/fsf-src/meissner-lra/gcc/lra-assigns.c:1268 0x104cfe43 lra_assign() /home/meissner/fsf-src/meissner-lra/gcc/lra-assigns.c:1425 0x104ca837 lra(_IO_FILE*) /home/meissner/fsf-src/meissner-lra/gcc/lra.c:2309 0x1047d6eb do_reload /home/meissner/fsf-src/meissner-lra/gcc/ira.c:4619 0x1047d6eb rest_of_handle_reload /home/meissner/fsf-src/meissner-lra/gcc/ira.c:4731 2) A lot of fortran tests are failing for all optimization levels due to segmentation violations at runtime: AIL: gfortran.dg/advance_1.f90 -O0 execution test Executing on host: /home/meissner/fsf-build-ppc64/meissner-lra/gcc/testsuite/gfortran1/../../gfortran -B/home/meissner/fsf-build-ppc64/meissner-lra/gcc/testsuite/gfortran1/../../ -B/home/meissner/fsf-build-ppc64/meissner-lra/powerpc64-unkno wn-linux-gnu/./libgfortran/ /home/meissner/fsf-src/meissner-lra/gcc/testsuite/gfortran.dg/advance_1.f90 -fno-diagnostics-show-caret -O1 -pedantic-errors -B/home/meissner/fsf-build-ppc64/meissner-lra/powerpc64-unknown-linux-gnu/32/libg fortran/.libs -L/home/meissner/fsf-build-ppc64/meissner-lra/powerpc64-unknown-linux-gnu/32/libgfortran/.libs -L/home/meissner/fsf-build-ppc64/meissner-lra/powerpc64-unknown-linux-gnu/32/libgfortran/.libs -lm -m32 -o ./advance_1.exe (t imeout = 300) spawn /home/meissner/fsf-build-ppc64/meissner-lra/gcc/testsuite/gfortran1/../../gfortran -B/home/meissner/fsf-build-ppc64/meissner-lra/gcc/testsuite/gfortran1/../../ -B/home/meissner/fsf-build-ppc64/meissner-lra/powerpc64-unknown-linux-gnu/ ./libgfortran/ /home/meissner/fsf-src/meissner-lra/gcc/testsuite/gfortran.dg/advance_1.f90 -fno-diagnostics-show-caret -O1 -pedantic-errors -B/home/meissner/fsf-build-ppc64/meissner-lra/powerpc64-unknown-linux-gnu/32/libgfortran/.libs -L/ho me/meissner/fsf-build-ppc64/meissner-lra/powerpc64-unknown-linux-gnu/32/libgfortran/.libs -L/home/meissner/fsf-build-ppc64/meissner-lra/powerpc64-unknown-linux-gnu/32/libgfortran/.libs -lm -m32 -o ./advance_1.exe PASS: gfortran.dg/advance_1.f90 -O1 (test for excess errors) Setting LD_LIBRARY_PATH to .:/home/meissner/fsf-build-ppc64/meissner-lra/powerpc64-unknown-linux-gnu/32/libgfortran/.libs:/home/meissner/fsf-build-ppc64/meissner-lra/powerpc64-unknown-linux-gnu/32/libgfortran/.libs:/home/meissner/fsf-build- ppc64/meissner-lra/gcc:/home/meissner/fsf-build-ppc64/meissner-lra/gcc/32:.:/home/meissner/fsf-build-ppc64/meissner-lra/powerpc64-unknown-linux-gnu/32/libgfortran/.libs:/home/meissner/fsf-build-ppc64/meissner-lra/powerpc64-unknown-linux-gnu /32/libgfortran/.libs:/home/meissner/fsf-build-ppc64/meissner-lra/gcc:/home/meissner/fsf-build-ppc64/meissner-lra/gcc/32:/home/meissner/tools/ppc64/lib:/home/meissner/tools/ppc32/lib:/home/meissner/tools-binutils/ppc64/lib:/home/meissner/to ols-binutils/ppc32/lib spawn [open ...] Program received signal SIGSEGV: Segmentation fault - invalid memory reference.
On 04/24/2013 03:42 PM, Michael Meissner wrote: > I'm seeing a lot of failures with these changes in make check. > I've reproduced it, Mike. I'll work on it. Thanks.
Index: ChangeLog =================================================================== --- ChangeLog (revision 198101) +++ ChangeLog (working copy) @@ -1,3 +1,10 @@ +2013-04-22 Vladimir Makarov <vmakarov@redhat.com> + + * lra-constraints.c (process_address): Try to put lo_sum into + register. + * config/rs6000/rs6000.c (legitimate_lo_sum_address_p): Remove + lra_in_progress guard for addressing something bigger than word. + 2013-04-18 Vladimir Makarov <vmakarov@redhat.com> * lra-constraints.c (check_and_process_move): Move code for move Index: config/rs6000/rs6000.c =================================================================== --- config/rs6000/rs6000.c (revision 198101) +++ config/rs6000/rs6000.c (working copy) @@ -5736,7 +5736,7 @@ legitimate_lo_sum_address_p (enum machin return false; if (GET_MODE_NUNITS (mode) != 1) return false; - if (! lra_in_progress && GET_MODE_SIZE (mode) > UNITS_PER_WORD + if (GET_MODE_SIZE (mode) > UNITS_PER_WORD && !(/* ??? Assume floating point reg based on mode? */ TARGET_HARD_FLOAT && TARGET_FPRS && TARGET_DOUBLE_FLOAT && (mode == DFmode || mode == DDmode))) Index: lra-constraints.c =================================================================== --- lra-constraints.c (revision 198101) +++ lra-constraints.c (working copy) @@ -2504,8 +2504,21 @@ process_address (int nop, rtx *before, r *ad.inner = gen_rtx_LO_SUM (Pmode, new_reg, addr); if (! valid_address_p (ad.mode, *ad.outer, ad.as)) { - *ad.inner = addr; - code = -1; + /* Try to put lo_sum into register. */ + insn = emit_insn (gen_rtx_SET + (VOIDmode, new_reg, + gen_rtx_LO_SUM (Pmode, new_reg, addr))); + code = recog_memoized (insn); + if (code >= 0) + { + *ad.inner = new_reg; + if (! valid_address_p (ad.mode, *ad.outer, ad.as)) + { + *ad.inner = addr; + code = -1; + } + } + } } if (code < 0)