| Message ID | CAFULd4bFt16CtZk6hrZz3p258Bf1szLG+q2iPFXxZKNcPJJjYA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Jul 19, 2011 at 2:16 AM, Uros Bizjak <ubizjak@gmail.com> wrote: > On Tue, Jul 19, 2011 at 10:46 AM, Richard Guenther > <richard.guenther@gmail.com> wrote: > >>>>>>>>> TARGET_MEM_REF only works on ptr_mode. This patch allows 32bit address >>>>>>>>> in x32 mode. OK for trunk? >>>>>>>> >>>>>>>> Do you perhaps have a testcase to help in analyzing the problem? >>>>>>>> >>>>>>> >>>>>>> See: >>>>>>> >>>>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49780 >>>>>> >>>>>> I don't think that tree-ssa-address/addr_for_mem_ref is correct when >>>>>> REALLY_EXPAND is false. It constructs RTX "template" in pointer_mode, >>>>>> which is not necessary valid and is rejected from >>>>>> ix86_validate_address_p. When really expanding the expression, we have >>>>>> a conversion at the end: >>>>>> >>>>>> gen_addr_rtx (pointer_mode, sym, bse, idx, st, off, &address, NULL, NULL); >>>>>> if (pointer_mode != address_mode) >>>>>> address = convert_memory_address (address_mode, address); >>>>>> return address; >>>>>> >>>>>> This is in fact your r175912 change in the fix for PR47383 - you need >>>>>> to do something with template as well... >>>>>> >>>>> >>>>> Since TARGET_MEM_REF only works on ptr_mode, I don't think >>>>> we can change template. We just need to accept TARGET_MEM_REF >>>>> in ptr_mode and fix it up later. >>>> >>>> No, a template is used to get some insight into the supported address >>>> structure. If there is a mismatch, this approach fails, we can as well >>>> give the compiler whatever fake template we want. > > I have investigated other Pmode != ptr_mode targets, and none of them > check the mode of a register in TARGET_LEGITIMATE_ADDRESS_P (or > equivalent GO_IF_LEGITIMATE_ADDRESS). All it matters is only if there > is a register and regno of the register. > > Attached patch simply removes these two checks, as it seems they are > not needed. This also follows how other Pmode != ptr_mode targets. > > 2011-07-19 Uros Bizjak <ubizjak@gmail.com> > > PR target/49780 > * config/i386/i386.c (ix86_legitimate_address_p): Remove checks that > base and index registers are in Pmode. > > Patch was bootstrapped and regression tested on x86_64-pc-linux-gnu > {,-m32}. Can you please re-test it on x32? Comparing with my patch, which only allows DImode and SImode, it caused the following regressions: FAIL: libgomp.fortran/omp_atomic1.f90 -O1 execution test FAIL: libgomp.fortran/omp_atomic1.f90 -O2 execution test FAIL: libgomp.fortran/omp_atomic1.f90 -O3 -fomit-frame-pointer execution test FAIL: libgomp.fortran/omp_atomic1.f90 -O3 -fomit-frame-pointer -funroll-all-loops -finline-functions execution test FAIL: libgomp.fortran/omp_atomic1.f90 -O3 -fomit-frame-pointer -funroll-loops execution test FAIL: libgomp.fortran/omp_atomic1.f90 -O3 -g execution test FAIL: libgomp.fortran/omp_atomic1.f90 -Os execution test > BTW: I still think that template should return the same address > structure as expansion, but this won't crash the compiler anymore. > > Uros. >
On Tue, Jul 19, 2011 at 3:47 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >> Attached patch simply removes these two checks, as it seems they are >> not needed. This also follows how other Pmode != ptr_mode targets. >> >> 2011-07-19 Uros Bizjak <ubizjak@gmail.com> >> >> PR target/49780 >> * config/i386/i386.c (ix86_legitimate_address_p): Remove checks that >> base and index registers are in Pmode. >> >> Patch was bootstrapped and regression tested on x86_64-pc-linux-gnu >> {,-m32}. Can you please re-test it on x32? > > Comparing with my patch, which only allows DImode and SImode, > it caused the following regressions: > > FAIL: libgomp.fortran/omp_atomic1.f90 -O1 execution test > FAIL: libgomp.fortran/omp_atomic1.f90 -O2 execution test > FAIL: libgomp.fortran/omp_atomic1.f90 -O3 -fomit-frame-pointer execution test > FAIL: libgomp.fortran/omp_atomic1.f90 -O3 -fomit-frame-pointer > -funroll-all-loops -finline-functions execution test > FAIL: libgomp.fortran/omp_atomic1.f90 -O3 -fomit-frame-pointer > -funroll-loops execution test > FAIL: libgomp.fortran/omp_atomic1.f90 -O3 -g execution test > FAIL: libgomp.fortran/omp_atomic1.f90 -Os execution test > >> BTW: I still think that template should return the same address >> structure as expansion, but this won't crash the compiler anymore. There is no non-DImode addresses in insn stream, so I doubt the bug is due to my change. Uros.
On Tue, Jul 19, 2011 at 7:04 AM, Uros Bizjak <ubizjak@gmail.com> wrote: > On Tue, Jul 19, 2011 at 3:47 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > >>> Attached patch simply removes these two checks, as it seems they are >>> not needed. This also follows how other Pmode != ptr_mode targets. >>> >>> 2011-07-19 Uros Bizjak <ubizjak@gmail.com> >>> >>> PR target/49780 >>> * config/i386/i386.c (ix86_legitimate_address_p): Remove checks that >>> base and index registers are in Pmode. >>> >>> Patch was bootstrapped and regression tested on x86_64-pc-linux-gnu >>> {,-m32}. Can you please re-test it on x32? >> >> Comparing with my patch, which only allows DImode and SImode, >> it caused the following regressions: >> >> FAIL: libgomp.fortran/omp_atomic1.f90 -O1 execution test >> FAIL: libgomp.fortran/omp_atomic1.f90 -O2 execution test >> FAIL: libgomp.fortran/omp_atomic1.f90 -O3 -fomit-frame-pointer execution test >> FAIL: libgomp.fortran/omp_atomic1.f90 -O3 -fomit-frame-pointer >> -funroll-all-loops -finline-functions execution test >> FAIL: libgomp.fortran/omp_atomic1.f90 -O3 -fomit-frame-pointer >> -funroll-loops execution test >> FAIL: libgomp.fortran/omp_atomic1.f90 -O3 -g execution test >> FAIL: libgomp.fortran/omp_atomic1.f90 -Os execution test >> >>> BTW: I still think that template should return the same address >>> structure as expansion, but this won't crash the compiler anymore. > > There is no non-DImode addresses in insn stream, so I doubt the bug is > due to my change. > I saw the same failures on x86-64: http://gcc.gnu.org/ml/gcc-testresults/2011-07/msg02224.html Can you take a look? Thanks.
On Tue, Jul 19, 2011 at 4:42 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Tue, Jul 19, 2011 at 7:04 AM, Uros Bizjak <ubizjak@gmail.com> wrote: >> On Tue, Jul 19, 2011 at 3:47 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >> >>>> Attached patch simply removes these two checks, as it seems they are >>>> not needed. This also follows how other Pmode != ptr_mode targets. >>>> >>>> 2011-07-19 Uros Bizjak <ubizjak@gmail.com> >>>> >>>> PR target/49780 >>>> * config/i386/i386.c (ix86_legitimate_address_p): Remove checks that >>>> base and index registers are in Pmode. >>>> >>>> Patch was bootstrapped and regression tested on x86_64-pc-linux-gnu >>>> {,-m32}. Can you please re-test it on x32? >>> >>> Comparing with my patch, which only allows DImode and SImode, >>> it caused the following regressions: >>> >>> FAIL: libgomp.fortran/omp_atomic1.f90 -O1 execution test >>> FAIL: libgomp.fortran/omp_atomic1.f90 -O2 execution test >>> FAIL: libgomp.fortran/omp_atomic1.f90 -O3 -fomit-frame-pointer execution test >>> FAIL: libgomp.fortran/omp_atomic1.f90 -O3 -fomit-frame-pointer >>> -funroll-all-loops -finline-functions execution test >>> FAIL: libgomp.fortran/omp_atomic1.f90 -O3 -fomit-frame-pointer >>> -funroll-loops execution test >>> FAIL: libgomp.fortran/omp_atomic1.f90 -O3 -g execution test >>> FAIL: libgomp.fortran/omp_atomic1.f90 -Os execution test >>> >>>> BTW: I still think that template should return the same address >>>> structure as expansion, but this won't crash the compiler anymore. >> >> There is no non-DImode addresses in insn stream, so I doubt the bug is >> due to my change. >> > > I saw the same failures on x86-64: > > http://gcc.gnu.org/ml/gcc-testresults/2011-07/msg02224.html > > Can you take a look? Sometimes, the compiler is really creative in inventing instructions: (insn 47 46 49 7 (set (reg:SI 68 [ D.1686 ]) (subreg:SI (plus:SF (reg:SF 159 [ D.1685 ]) (reg:SF 159 [ D.1685 ])) 0)) omp_atomic1.f90:17 247 {*lea_2} (expr_list:REG_DEAD (reg:SF 159 [ D.1685 ]) (nil))) Really funny. Uros.
On Tue, Jul 19, 2011 at 06:26:33PM +0200, Uros Bizjak wrote: > Sometimes, the compiler is really creative in inventing instructions: > > (insn 47 46 49 7 (set (reg:SI 68 [ D.1686 ]) > (subreg:SI (plus:SF (reg:SF 159 [ D.1685 ]) > (reg:SF 159 [ D.1685 ])) 0)) omp_atomic1.f90:17 247 {*lea_2} > (expr_list:REG_DEAD (reg:SF 159 [ D.1685 ]) > (nil))) > > Really funny. That's the job of combiner to try all kinds of stuff and it is the responsibility of the backend to reject those. I think it would be better to get back to testing Pmode in the legitimate address hook, perhaps allowing ptr_mode too in addition to Pmode (which for -m32/-m64 won't mean any change, just for -mx32). Jakub
On Tue, Jul 19, 2011 at 6:30 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Tue, Jul 19, 2011 at 06:26:33PM +0200, Uros Bizjak wrote: >> Sometimes, the compiler is really creative in inventing instructions: >> >> (insn 47 46 49 7 (set (reg:SI 68 [ D.1686 ]) >> (subreg:SI (plus:SF (reg:SF 159 [ D.1685 ]) >> (reg:SF 159 [ D.1685 ])) 0)) omp_atomic1.f90:17 247 {*lea_2} >> (expr_list:REG_DEAD (reg:SF 159 [ D.1685 ]) >> (nil))) >> >> Really funny. > > That's the job of combiner to try all kinds of stuff and it is the > responsibility of the backend to reject those. I think it would be better > to get back to testing Pmode in the legitimate address hook, perhaps > allowing ptr_mode too in addition to Pmode (which for -m32/-m64 won't mean > any change, just for -mx32). Actually, there is a bypass in ix86_decompose_address, and this RTX squeezed through. IMO constructs like this should be rejected in i_d_a, which effectively only moves Pmode/ptr_mode check here. I'm looking into it. Uros.
Index: config/i386/i386.c =================================================================== --- config/i386/i386.c (revision 176433) +++ config/i386/i386.c (working copy) @@ -11653,10 +11653,6 @@ ix86_legitimate_address_p (enum machine_ /* Base is not a register. */ return false; - if (GET_MODE (base) != Pmode) - /* Base is not in Pmode. */ - return false; - if ((strict && ! REG_OK_FOR_BASE_STRICT_P (reg)) || (! strict && ! REG_OK_FOR_BASE_NONSTRICT_P (reg))) /* Base is not valid. */ @@ -11682,10 +11678,6 @@ ix86_legitimate_address_p (enum machine_ /* Index is not a register. */ return false; - if (GET_MODE (index) != Pmode) - /* Index is not in Pmode. */ - return false; - if ((strict && ! REG_OK_FOR_INDEX_STRICT_P (reg)) || (! strict && ! REG_OK_FOR_INDEX_NONSTRICT_P (reg))) /* Index is not valid. */