Using GCC 4.8.0 as of 20130203 : $ xgcc -v Using built-in specs. COLLECT_GCC=/home/merkil/gcc/dist/bin/gcc COLLECT_LTO_WRAPPER=/home/merkil/gcc/dist/libexec/gcc/x86_64-unknown-linux-gnu/4.8.0/lto-wrapper Target: x86_64-unknown-linux-gnu Configured with: ../srcdir/configure CFLAGS='-O2 -pipe' CXXFLAGS='-O2 -pipe' --enable-languages=c --prefix=/home/merkil/gcc/dist Thread model: posix gcc version 4.8.0 20130203 (experimental) (GCC) $ cat reg.c int i, a, t, b, c, d, e, f, j, *h; void fn(void) { if(b) { int *p, *q; char g; if(f++) for(;; e++); lbl: for(b = 0; b < 2; b++) t /= d + 1 ? : i || a < c < (d = f) ? : 1 | (j = 2); *p = g >= *q ^ c != a ^ *p; if(!e) { q = p; goto lbl; } } if(h++) goto lbl; } $ xgcc -O2 reg.c {standard input}: Assembler messages: {standard input}:112: Error: incorrect register `%rdi' used with `l' suffix
Started with LRA merge ( http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=192719 ). (define_insn "*cmp<mode>_1" [(set (reg FLAGS_REG) (compare (match_operand:SWI 0 "nonimmediate_operand" "<r>m,<r>") (match_operand:SWI 1 "<general_operand>" "<r><i>,<r>m")))] "ix86_match_ccmode (insn, CCmode)" "cmp{<imodesuffix>}\t{%1, %0|%0, %1}" [(set_attr "type" "icmp") (set_attr "mode" "<MODE>")]) In *.ira dump the insn looks still correct: (insn 90 87 91 22 (set (reg:CCGC 17 flags) (compare:CCGC (reg:SI 118 [ D.1797 ]) (mem:SI (reg/v/f:DI 59 [ q ]) [2 *q_1+0 S4 A32]))) pr56195.c:17 7 {*cmpsi_1} (nil)) but in *.reload dump: (insn 90 256 91 22 (set (reg:CCGC 17 flags) (compare:CCGC (reg:DI 5 di [137]) (mem:SI (reg:DI 5 di [137]) [2 *q_1+0 S4 A32]))) pr56195.c:17 7 {*cmpsi_1} (expr_list:REG_DEAD (reg:DI 5 di [137]) (nil))) so I wonder how could it match that way and survive till end of compilation.
Seems this is heavily related to the uninitialized uses, in (insn 90 87 91 22 (set (reg:CCGC 17 flags) (compare:CCGC (reg:SI 118 [ D.1797 ]) (mem:SI (reg/v/f:DI 59 [ q ]) [2 *q_1+0 S4 A32]))) pr56195.c:17 7 {*cmpsi_1} (nil)) both SI 118 and DI 59 pseudos are preinitialized to const0_rtx because both the address to read from and the other value are uninitialized. Guess LRA figures out that both operands are equal to const0_rtx, just doesn't handle the different modes properly (it can be cmpl (%rdi), %edi ).
I'd say the bug is in get_reload_reg. Changing pseudo 118 in operand 0 of insn 90 on equiv 0 Changing address in insn 90 r59:DI -- no change Changing pseudo 59 in address of insn 90 on equiv 0 Creating newreg=137, assigning class GENERAL_REGS to address r137 Choosing alt 1 in insn 90: (0) r (1) rm Reuse r137 for reload 0, change to class INDEX_REGS for r137 90: flags:CCGC=cmp(r137:DI,[r137:DI]) Inserting insn reload before: 256: r137:DI=0 3065 if (get_reload_reg (type, mode, old, goal_alt[i], "", &new_reg) 3066 && type != OP_OUT) calls it with type=OP_IN, mode=SImode, original=const0_rtx, rclass=GENERAL_REGS but returns new_reg = (reg:DI 137). That is because: if (rtx_equal_p (curr_insn_input_reloads[i].input, original) && in_class_p (curr_insn_input_reloads[i].reg, rclass, &new_class)) doesn't check any mode if original (and curr_insn_input_reloads[i].input) are VOIDmode as in this case. So, either this can be fixed by doing: if (rtx_equal_p (curr_insn_input_reloads[i].input, original) - && in_class_p (curr_insn_input_reloads[i].reg, rclass, &new_class)) + && in_class_p (curr_insn_input_reloads[i].reg, rclass, &new_class) + && GET_MODE (curr_insn_input_reloads[i].reg) == mode) , or we could try better, if the GET_MODE (curr_insn_input_reloads[i].reg) is wider than mode, see if we can create a lowpart subreg thereof and return that, and only give up (i.e. continue looping) if creation of the lowpart subreg for some reason failed. Vlad, what do you think?
(In reply to comment #3) > I'd say the bug is in get_reload_reg. > Changing pseudo 118 in operand 0 of insn 90 on equiv 0 > Changing address in insn 90 r59:DI -- no change > Changing pseudo 59 in address of insn 90 on equiv 0 > Creating newreg=137, assigning class GENERAL_REGS to address r137 > Choosing alt 1 in insn 90: (0) r (1) rm > Reuse r137 for reload 0, change to class INDEX_REGS for r137 > 90: flags:CCGC=cmp(r137:DI,[r137:DI]) > Inserting insn reload before: > 256: r137:DI=0 > > > 3065 if (get_reload_reg (type, mode, old, goal_alt[i], "", &new_reg) > 3066 && type != OP_OUT) > > calls it with > type=OP_IN, mode=SImode, original=const0_rtx, rclass=GENERAL_REGS > but returns new_reg = (reg:DI 137). > That is because: > if (rtx_equal_p (curr_insn_input_reloads[i].input, original) > && in_class_p (curr_insn_input_reloads[i].reg, rclass, &new_class)) > doesn't check any mode if original (and curr_insn_input_reloads[i].input) are > VOIDmode as in this case. So, either this can be fixed by doing: > if (rtx_equal_p (curr_insn_input_reloads[i].input, original) > - && in_class_p (curr_insn_input_reloads[i].reg, rclass, &new_class)) > + && in_class_p (curr_insn_input_reloads[i].reg, rclass, &new_class) > + && GET_MODE (curr_insn_input_reloads[i].reg) == mode) > , or we could try better, if the GET_MODE (curr_insn_input_reloads[i].reg) > is wider than mode, see if we can create a lowpart subreg thereof and return > that, and only give up (i.e. continue looping) if creation of the lowpart > subreg for some reason failed. > > Vlad, what do you think? I think, the second solution with lowpart is better. Would you like to make a patch or may be you prefer that I work on it?
So something like: --- lra-constraints.c.jj 2013-02-07 18:34:39.000000000 +0100 +++ lra-constraints.c 2013-02-07 20:41:17.051986353 +0100 @@ -421,7 +421,21 @@ get_reload_reg (enum op_type type, enum if (rtx_equal_p (curr_insn_input_reloads[i].input, original) && in_class_p (curr_insn_input_reloads[i].reg, rclass, &new_class)) { - *result_reg = curr_insn_input_reloads[i].reg; + rtx reg = curr_insn_input_reloads[i].reg; + /* If input is equal to original and both are VOIDmode, + GET_MODE (reg) might be still different from mode. + Ensure we don't return *result_reg with wrong mode. */ + if (GET_MODE (reg) != mode) + { + if (GET_MODE_SIZE (GET_MODE (reg)) < GET_MODE_SIZE (mode)) + continue; + reg = simplify_subreg (mode, reg, GET_MODE (reg), + subreg_lowpart_offset (mode, + GET_MODE (reg))); + if (reg == NULL_RTX || !REG_P (reg)) + continue; + } + *result_reg = reg; regno = REGNO (*result_reg); if (lra_dump_file != NULL) { ?
Actually, that one doesn't really work, because we have pseudo rather than hard reg at that point, which will never simplify. With this: --- lra-constraints.c.jj 2013-02-07 18:34:39.000000000 +0100 +++ lra-constraints.c 2013-02-07 20:58:25.558920536 +0100 @@ -421,8 +421,20 @@ get_reload_reg (enum op_type type, enum if (rtx_equal_p (curr_insn_input_reloads[i].input, original) && in_class_p (curr_insn_input_reloads[i].reg, rclass, &new_class)) { - *result_reg = curr_insn_input_reloads[i].reg; - regno = REGNO (*result_reg); + rtx reg = curr_insn_input_reloads[i].reg; + regno = REGNO (reg); + /* If input is equal to original and both are VOIDmode, + GET_MODE (reg) might be still different from mode. + Ensure we don't return *result_reg with wrong mode. */ + if (GET_MODE (reg) != mode) + { + if (GET_MODE_SIZE (GET_MODE (reg)) < GET_MODE_SIZE (mode)) + continue; + reg = lowpart_subreg (mode, reg, GET_MODE (reg)); + if (reg == NULL_RTX || GET_CODE (reg) != SUBREG) + continue; + } + *result_reg = reg; if (lra_dump_file != NULL) { fprintf (lra_dump_file, " Reuse r%d for reload ", regno); the assembly difference is: - cmpl (%rdi), %rdi + cmpl (%rdi), %edi which is desirable in this case, but not sure if all get_reload_reg callers will grok a SUBREG instead of REG returned in *result_reg.
(In reply to comment #6) > Actually, that one doesn't really work, because we have pseudo rather than hard > reg at that point, which will never simplify. > > With this: > > --- lra-constraints.c.jj 2013-02-07 18:34:39.000000000 +0100 > +++ lra-constraints.c 2013-02-07 20:58:25.558920536 +0100 > @@ -421,8 +421,20 @@ get_reload_reg (enum op_type type, enum > if (rtx_equal_p (curr_insn_input_reloads[i].input, original) > && in_class_p (curr_insn_input_reloads[i].reg, rclass, &new_class)) > { > - *result_reg = curr_insn_input_reloads[i].reg; > - regno = REGNO (*result_reg); > + rtx reg = curr_insn_input_reloads[i].reg; > + regno = REGNO (reg); > + /* If input is equal to original and both are VOIDmode, > + GET_MODE (reg) might be still different from mode. > + Ensure we don't return *result_reg with wrong mode. */ > + if (GET_MODE (reg) != mode) > + { > + if (GET_MODE_SIZE (GET_MODE (reg)) < GET_MODE_SIZE (mode)) > + continue; > + reg = lowpart_subreg (mode, reg, GET_MODE (reg)); > + if (reg == NULL_RTX || GET_CODE (reg) != SUBREG) > + continue; > + } > + *result_reg = reg; > if (lra_dump_file != NULL) > { > fprintf (lra_dump_file, " Reuse r%d for reload ", regno); > > the assembly difference is: > > - cmpl (%rdi), %rdi > + cmpl (%rdi), %edi > > which is desirable in this case, but not sure if all get_reload_reg callers > will grok a SUBREG instead of REG returned in *result_reg. This version of patch looks ok for me. I have no worry about get_reload_reg callers. It should work fine (that is a difference from reload pass when you should care about secondary reloads etc). Thanks for working on this, Jakub,
Author: jakub Date: Fri Feb 8 15:19:02 2013 New Revision: 195891 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=195891 Log: PR rtl-optimization/56195 * lra-constraints.c (get_reload_reg): Don't reuse regs if they have smaller mode than requested, if they have wider mode than requested, try to return a SUBREG. * gcc.dg/torture/pr56195.c: New test. Added: trunk/gcc/testsuite/gcc.dg/torture/pr56195.c Modified: trunk/gcc/ChangeLog trunk/gcc/lra-constraints.c trunk/gcc/testsuite/ChangeLog
Fixed.