| Message ID | 87wqy0cuzt.fsf@talisman.home |
|---|---|
| State | New |
| Headers | show |
On Mon, Nov 5, 2012 at 9:30 AM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > This is a case where we had: > > (set (reg:HI foo) (plus:HI (reg:HI sp) (const_int X))) > (clobber (reg:CC FLAGS_REG)) > > and the splitters decided to convert it to an LEA: > > (set (reg:SI foo) (plus:SI (subreg:SI (reg:HI sp) 0) (const_int X))) > > But this fails to match, because ix86_address_subreg_operand > doesn't allow subregs of the stack pointer. > > This shows up an inconsistency in the way the generic code handles > subregs of the stack pointer. Normally we refuse to fold them, even > after reload, but the final_p case of alter_subreg folds them anyway. > That's how we ended up with the rather odd 16-bit sp. > > However, even if the special alter_subreg case was removed > (so that we continued to use stack_pointer_rtx itself), we'd have: > > (set (reg:HI foo) (plus:HI (subreg:HI (reg:DI sp) 0) (const_int X))) > (clobber (reg:CC FLAGS_REG)) > > which would get converted to: > > (set (reg:SI foo) (plus:SI (subreg:SI (reg:DI sp) 0) (const_int X))) > > and we'd ICE in the same way. > > The reason x86 rejects subregs of the stack pointer is this same refusal > to fold. ix86_print_operand_address tries to simplify a SUBREG to a REG > and simplify_subreg wouldn't do anything for sp. > > simplify_subreg isn't a lot of help at the output stage though. > If the insn stream contains a subreg that could be simplified but > hasn't been, then IMO that's a bug. The cases we have to handle here > are those that can't be simplified (unless we decide at some point that > all registers must be simplifiable after reload, in which case we shouldn't > need to handle SUBREGs at all). > > As things stand, I think we should be using true_regnum in this case instead. > > Tested on x86_64-linux-gnu. OK to install? Let's ask H.J. to test this change on x32. > gcc/ > PR target/55204 > * config/i386/i386.c (ix86_address_subreg_operand): Remove stack > pointer check. > (print_reg): Use true_regnum rather than REGNO. > (ix86_print_operand_address): Remove SUBREG handling. The patch is OK for mainline and 4.7, if it passes H.J.'s tests with -maddress-mode={short,long} on x32. > + unsigned int regno = true_regnum (x); I'd rather see the declaration at the beginning of the function. Thanks, Uros.
On Mon, Nov 5, 2012 at 1:01 AM, Uros Bizjak <ubizjak@gmail.com> wrote: > On Mon, Nov 5, 2012 at 9:30 AM, Richard Sandiford > <rdsandiford@googlemail.com> wrote: >> This is a case where we had: >> >> (set (reg:HI foo) (plus:HI (reg:HI sp) (const_int X))) >> (clobber (reg:CC FLAGS_REG)) >> >> and the splitters decided to convert it to an LEA: >> >> (set (reg:SI foo) (plus:SI (subreg:SI (reg:HI sp) 0) (const_int X))) >> >> But this fails to match, because ix86_address_subreg_operand >> doesn't allow subregs of the stack pointer. >> >> This shows up an inconsistency in the way the generic code handles >> subregs of the stack pointer. Normally we refuse to fold them, even >> after reload, but the final_p case of alter_subreg folds them anyway. >> That's how we ended up with the rather odd 16-bit sp. >> >> However, even if the special alter_subreg case was removed >> (so that we continued to use stack_pointer_rtx itself), we'd have: >> >> (set (reg:HI foo) (plus:HI (subreg:HI (reg:DI sp) 0) (const_int X))) >> (clobber (reg:CC FLAGS_REG)) >> >> which would get converted to: >> >> (set (reg:SI foo) (plus:SI (subreg:SI (reg:DI sp) 0) (const_int X))) >> >> and we'd ICE in the same way. >> >> The reason x86 rejects subregs of the stack pointer is this same refusal >> to fold. ix86_print_operand_address tries to simplify a SUBREG to a REG >> and simplify_subreg wouldn't do anything for sp. >> >> simplify_subreg isn't a lot of help at the output stage though. >> If the insn stream contains a subreg that could be simplified but >> hasn't been, then IMO that's a bug. The cases we have to handle here >> are those that can't be simplified (unless we decide at some point that >> all registers must be simplifiable after reload, in which case we shouldn't >> need to handle SUBREGs at all). >> >> As things stand, I think we should be using true_regnum in this case instead. >> >> Tested on x86_64-linux-gnu. OK to install? > > Let's ask H.J. to test this change on x32. > >> gcc/ >> PR target/55204 >> * config/i386/i386.c (ix86_address_subreg_operand): Remove stack >> pointer check. >> (print_reg): Use true_regnum rather than REGNO. >> (ix86_print_operand_address): Remove SUBREG handling. > > The patch is OK for mainline and 4.7, if it passes H.J.'s tests with > -maddress-mode={short,long} on x32. > I am on it. It will take a while. H.J.
On Mon, Nov 5, 2012 at 2:21 AM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Mon, Nov 5, 2012 at 1:01 AM, Uros Bizjak <ubizjak@gmail.com> wrote: >> On Mon, Nov 5, 2012 at 9:30 AM, Richard Sandiford >> <rdsandiford@googlemail.com> wrote: >>> This is a case where we had: >>> >>> (set (reg:HI foo) (plus:HI (reg:HI sp) (const_int X))) >>> (clobber (reg:CC FLAGS_REG)) >>> >>> and the splitters decided to convert it to an LEA: >>> >>> (set (reg:SI foo) (plus:SI (subreg:SI (reg:HI sp) 0) (const_int X))) >>> >>> But this fails to match, because ix86_address_subreg_operand >>> doesn't allow subregs of the stack pointer. >>> >>> This shows up an inconsistency in the way the generic code handles >>> subregs of the stack pointer. Normally we refuse to fold them, even >>> after reload, but the final_p case of alter_subreg folds them anyway. >>> That's how we ended up with the rather odd 16-bit sp. >>> >>> However, even if the special alter_subreg case was removed >>> (so that we continued to use stack_pointer_rtx itself), we'd have: >>> >>> (set (reg:HI foo) (plus:HI (subreg:HI (reg:DI sp) 0) (const_int X))) >>> (clobber (reg:CC FLAGS_REG)) >>> >>> which would get converted to: >>> >>> (set (reg:SI foo) (plus:SI (subreg:SI (reg:DI sp) 0) (const_int X))) >>> >>> and we'd ICE in the same way. >>> >>> The reason x86 rejects subregs of the stack pointer is this same refusal >>> to fold. ix86_print_operand_address tries to simplify a SUBREG to a REG >>> and simplify_subreg wouldn't do anything for sp. >>> >>> simplify_subreg isn't a lot of help at the output stage though. >>> If the insn stream contains a subreg that could be simplified but >>> hasn't been, then IMO that's a bug. The cases we have to handle here >>> are those that can't be simplified (unless we decide at some point that >>> all registers must be simplifiable after reload, in which case we shouldn't >>> need to handle SUBREGs at all). >>> >>> As things stand, I think we should be using true_regnum in this case instead. >>> >>> Tested on x86_64-linux-gnu. OK to install? >> >> Let's ask H.J. to test this change on x32. >> >>> gcc/ >>> PR target/55204 >>> * config/i386/i386.c (ix86_address_subreg_operand): Remove stack >>> pointer check. >>> (print_reg): Use true_regnum rather than REGNO. >>> (ix86_print_operand_address): Remove SUBREG handling. >> >> The patch is OK for mainline and 4.7, if it passes H.J.'s tests with >> -maddress-mode={short,long} on x32. >> > > I am on it. It will take a while. > I tested it with -maddress-mode={short,long} on x32. There are no regressions in GCC nor glibc testsuite. Thanks.
Uros Bizjak <ubizjak@gmail.com> writes: > On Mon, Nov 5, 2012 at 9:30 AM, Richard Sandiford > <rdsandiford@googlemail.com> wrote: >> gcc/ >> PR target/55204 >> * config/i386/i386.c (ix86_address_subreg_operand): Remove stack >> pointer check. >> (print_reg): Use true_regnum rather than REGNO. >> (ix86_print_operand_address): Remove SUBREG handling. > > The patch is OK for mainline and 4.7, if it passes H.J.'s tests with > -maddress-mode={short,long} on x32. > >> + unsigned int regno = true_regnum (x); > > I'd rather see the declaration at the beginning of the function. OK, thanks, applied to both with that change. Richard
On Mon, Nov 5, 2012 at 10:56 AM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > Uros Bizjak <ubizjak@gmail.com> writes: >> On Mon, Nov 5, 2012 at 9:30 AM, Richard Sandiford >> <rdsandiford@googlemail.com> wrote: >>> gcc/ >>> PR target/55204 >>> * config/i386/i386.c (ix86_address_subreg_operand): Remove stack >>> pointer check. >>> (print_reg): Use true_regnum rather than REGNO. >>> (ix86_print_operand_address): Remove SUBREG handling. >> >> The patch is OK for mainline and 4.7, if it passes H.J.'s tests with >> -maddress-mode={short,long} on x32. >> >>> + unsigned int regno = true_regnum (x); >> >> I'd rather see the declaration at the beginning of the function. > > OK, thanks, applied to both with that change. > > Richard On 4.7 branch, this failed to build x32 run-time library: /tmp/ccID3vr6.s: Assembler messages: /tmp/ccID3vr6.s:2788: Error: bad register name `%rr15' There is an extra 'r' in register name.
Index: gcc/config/i386/i386.c =================================================================== --- gcc/config/i386/i386.c 2012-11-04 21:53:46.000000000 +0000 +++ gcc/config/i386/i386.c 2012-11-04 21:53:46.821356709 +0000 @@ -11784,10 +11784,6 @@ ix86_address_subreg_operand (rtx op) if (GET_MODE_SIZE (mode) > UNITS_PER_WORD) return false; - /* simplify_subreg does not handle stack pointer. */ - if (REGNO (op) == STACK_POINTER_REGNUM) - return false; - /* Allow only SUBREGs of non-eliminable hard registers. */ return register_no_elim_operand (op, mode); } @@ -14084,13 +14080,6 @@ print_reg (rtx x, int code, FILE *file) const char *reg; bool duplicated = code == 'd' && TARGET_AVX; - gcc_assert (x == pc_rtx - || (REGNO (x) != ARG_POINTER_REGNUM - && REGNO (x) != FRAME_POINTER_REGNUM - && REGNO (x) != FLAGS_REG - && REGNO (x) != FPSR_REG - && REGNO (x) != FPCR_REG)); - if (ASSEMBLER_DIALECT == ASM_ATT) putc ('%', file); @@ -14101,6 +14090,13 @@ print_reg (rtx x, int code, FILE *file) return; } + unsigned int regno = true_regnum (x); + gcc_assert (regno != ARG_POINTER_REGNUM + && regno != FRAME_POINTER_REGNUM + && regno != FLAGS_REG + && regno != FPSR_REG + && regno != FPCR_REG); + if (code == 'w' || MMX_REG_P (x)) code = 2; else if (code == 'b') @@ -14126,7 +14122,7 @@ print_reg (rtx x, int code, FILE *file) { gcc_assert (TARGET_64BIT); putc ('r', file); - fprint_ul (file, REGNO (x) - FIRST_REX_INT_REG + 8); + fprint_ul (file, regno - FIRST_REX_INT_REG + 8); switch (code) { case 0: @@ -14170,24 +14166,24 @@ print_reg (rtx x, int code, FILE *file) case 16: case 2: normal: - reg = hi_reg_name[REGNO (x)]; + reg = hi_reg_name[regno]; break; case 1: - if (REGNO (x) >= ARRAY_SIZE (qi_reg_name)) + if (regno >= ARRAY_SIZE (qi_reg_name)) goto normal; - reg = qi_reg_name[REGNO (x)]; + reg = qi_reg_name[regno]; break; case 0: - if (REGNO (x) >= ARRAY_SIZE (qi_high_reg_name)) + if (regno >= ARRAY_SIZE (qi_high_reg_name)) goto normal; - reg = qi_high_reg_name[REGNO (x)]; + reg = qi_high_reg_name[regno]; break; case 32: if (SSE_REG_P (x)) { gcc_assert (!duplicated); putc ('y', file); - fputs (hi_reg_name[REGNO (x)] + 1, file); + fputs (hi_reg_name[regno] + 1, file); return; } break; @@ -14943,22 +14939,6 @@ ix86_print_operand_address (FILE *file, gcc_assert (ok); - if (parts.base && GET_CODE (parts.base) == SUBREG) - { - rtx tmp = SUBREG_REG (parts.base); - parts.base = simplify_subreg (GET_MODE (parts.base), - tmp, GET_MODE (tmp), 0); - gcc_assert (parts.base != NULL_RTX); - } - - if (parts.index && GET_CODE (parts.index) == SUBREG) - { - rtx tmp = SUBREG_REG (parts.index); - parts.index = simplify_subreg (GET_MODE (parts.index), - tmp, GET_MODE (tmp), 0); - gcc_assert (parts.index != NULL_RTX); - } - base = parts.base; index = parts.index; disp = parts.disp;