| Message ID | 87zjhr9kup.fsf@talisman.default |
|---|---|
| State | New |
| Headers | show |
On 2014-06-05, 4:12 PM, Richard Sandiford wrote: > > It looks like there's a missing break after the 'G' and 'H' handling. > > Tested on x86_64-linux-gnu. I also did an assembly comparison for > a range of targets and Alpha and MIPS seemed to be the only ones > affected. OK to install? > > Sorry for the long write-up... > It is long but make me to know new things about Alpha and MIPS insn cases. By the way, there are a lot of controversy about the changing cost for particular hard reg. Sometimes it creates a lot of mess in allocation, when one pseudo involves several hard regs. Also a lot of alternatives can gives different hard reg preferencing or wrong ones as we don't know what alternative will be used. Therefore I think about earlier alternative selections and reg class cost calculations based on the selected alternatives. But it will be a long project probably involving code selection too. I don't know how will it work until it is implemented which will not happen soon. As I remember I added the code as I see small improvements on x86/x86-64 on benchmarks. As for the patch, it is ok for me. Thanks, Richard. > > gcc/ > * ira-lives.c (single_reg_class): Add missing break. Return NO_REGS > for extra address and memory constraints, not just 'p' and > TARGET_MEM_CONSTRAINT. Likewise if the operand is a constant or > equivalent to a constant and if it matches an extra constraint. > Ignore other non-register operands. > > Index: gcc/ira-lives.c > =================================================================== > --- gcc/ira-lives.c 2014-06-04 22:15:23.527995920 +0100 > +++ gcc/ira-lives.c 2014-06-04 22:15:50.765243138 +0100 > @@ -839,7 +839,8 @@ single_reg_class (const char *constraint > && CONST_DOUBLE_OK_FOR_CONSTRAINT_P (equiv_const, > c, constraints))) > return NO_REGS; > - /* ??? what about memory */ > + break; > + > case 'r': > case 'a': case 'b': case 'c': case 'd': case 'e': case 'f': > case 'h': case 'j': case 'k': case 'l': > @@ -848,9 +849,22 @@ single_reg_class (const char *constraint > case 'A': case 'B': case 'C': case 'D': > case 'Q': case 'R': case 'S': case 'T': case 'U': > case 'W': case 'Y': case 'Z': > +#ifdef EXTRA_CONSTRAINT_STR > + /* ??? Is rejecting memory the best thing to do? */ > + if (EXTRA_MEMORY_CONSTRAINT (c, constraints) > + || EXTRA_ADDRESS_CONSTRAINT (c, constraints)) > + return NO_REGS; > + if (EXTRA_CONSTRAINT_STR (op, c, constraints) > + || (equiv_const != NULL_RTX > + && CONSTANT_P (equiv_const) > + && EXTRA_CONSTRAINT_STR (equiv_const, c, constraints))) > + return NO_REGS; > +#endif > next_cl = (c == 'r' > ? GENERAL_REGS > : REG_CLASS_FROM_CONSTRAINT (c, constraints)); > + if (next_cl == NO_REGS) > + break; > if (cl == NO_REGS > ? ira_class_singleton[next_cl][GET_MODE (op)] < 0 > : (ira_class_singleton[cl][GET_MODE (op)] >
Index: gcc/ira-lives.c =================================================================== --- gcc/ira-lives.c 2014-06-04 22:15:23.527995920 +0100 +++ gcc/ira-lives.c 2014-06-04 22:15:50.765243138 +0100 @@ -839,7 +839,8 @@ single_reg_class (const char *constraint && CONST_DOUBLE_OK_FOR_CONSTRAINT_P (equiv_const, c, constraints))) return NO_REGS; - /* ??? what about memory */ + break; + case 'r': case 'a': case 'b': case 'c': case 'd': case 'e': case 'f': case 'h': case 'j': case 'k': case 'l': @@ -848,9 +849,22 @@ single_reg_class (const char *constraint case 'A': case 'B': case 'C': case 'D': case 'Q': case 'R': case 'S': case 'T': case 'U': case 'W': case 'Y': case 'Z': +#ifdef EXTRA_CONSTRAINT_STR + /* ??? Is rejecting memory the best thing to do? */ + if (EXTRA_MEMORY_CONSTRAINT (c, constraints) + || EXTRA_ADDRESS_CONSTRAINT (c, constraints)) + return NO_REGS; + if (EXTRA_CONSTRAINT_STR (op, c, constraints) + || (equiv_const != NULL_RTX + && CONSTANT_P (equiv_const) + && EXTRA_CONSTRAINT_STR (equiv_const, c, constraints))) + return NO_REGS; +#endif next_cl = (c == 'r' ? GENERAL_REGS : REG_CLASS_FROM_CONSTRAINT (c, constraints)); + if (next_cl == NO_REGS) + break; if (cl == NO_REGS ? ira_class_singleton[next_cl][GET_MODE (op)] < 0 : (ira_class_singleton[cl][GET_MODE (op)]
I'm about to post a series of patches that reworks the handling of standard constraints. As part of that I needed to make single_reg_class handle "extra" constraints in a similar way to the standard ones. It's not a particularly worthwhile change in itself -- not enough to justify this long essay -- but I split it out because it's the only part of the series that changes codegen. The function looks like this: case 'i': if (CONSTANT_P (op) || (equiv_const != NULL_RTX && CONSTANT_P (equiv_const))) return NO_REGS; break; case 'n': if (CONST_SCALAR_INT_P (op) || (equiv_const != NULL_RTX && CONST_SCALAR_INT_P (equiv_const))) return NO_REGS; break; case 's': if ((CONSTANT_P (op) && !CONST_SCALAR_INT_P (op)) || (equiv_const != NULL_RTX && CONSTANT_P (equiv_const) && !CONST_SCALAR_INT_P (equiv_const))) return NO_REGS; break; case 'I': case 'J': case 'K': case 'L': case 'M': case 'N': case 'O': case 'P': if ((CONST_INT_P (op) && CONST_OK_FOR_CONSTRAINT_P (INTVAL (op), c, constraints)) || (equiv_const != NULL_RTX && CONST_INT_P (equiv_const) && CONST_OK_FOR_CONSTRAINT_P (INTVAL (equiv_const), c, constraints))) return NO_REGS; break; case 'E': case 'F': if (CONST_DOUBLE_AS_FLOAT_P (op) || (GET_CODE (op) == CONST_VECTOR && GET_MODE_CLASS (GET_MODE (op)) == MODE_VECTOR_FLOAT) || (equiv_const != NULL_RTX && (CONST_DOUBLE_AS_FLOAT_P (equiv_const) || (GET_CODE (equiv_const) == CONST_VECTOR && (GET_MODE_CLASS (GET_MODE (equiv_const)) == MODE_VECTOR_FLOAT))))) return NO_REGS; break; case 'G': case 'H': if ((CONST_DOUBLE_AS_FLOAT_P (op) && CONST_DOUBLE_OK_FOR_CONSTRAINT_P (op, c, constraints)) || (equiv_const != NULL_RTX && CONST_DOUBLE_AS_FLOAT_P (equiv_const) && CONST_DOUBLE_OK_FOR_CONSTRAINT_P (equiv_const, c, constraints))) return NO_REGS; /* ??? what about memory */ case 'r': case 'a': case 'b': case 'c': case 'd': case 'e': case 'f': case 'h': case 'j': case 'k': case 'l': case 'q': case 't': case 'u': case 'v': case 'w': case 'x': case 'y': case 'z': case 'A': case 'B': case 'C': case 'D': case 'Q': case 'R': case 'S': case 'T': case 'U': case 'W': case 'Y': case 'Z': next_cl = (c == 'r' ? GENERAL_REGS : REG_CLASS_FROM_CONSTRAINT (c, constraints)); if (cl == NO_REGS ? ira_class_singleton[next_cl][GET_MODE (op)] < 0 : (ira_class_singleton[cl][GET_MODE (op)] != ira_class_singleton[next_cl][GET_MODE (op)])) return NO_REGS; cl = next_cl; break; [...] default: return NO_REGS; So for known constant contraints we check whether OP or its equivalent constant satisfies the constraint and return NO_REGS if so. I'd like to extend this behaviour to the extra constraints, since some targets match constants (often symbolic or unspec-based constants) there too. The code that checks next_cl effectively assumes that the constraint is always a register constraint. If it's something else, next_cl will be NO_REGS, which isn't a singleton class, so we'll return NO_REGS regardless of what type of constraint we're matching or what OP is. (In principle this includes register constraints that are disabled on the current subtarget, since they'll have a next_cl of NO_REGS too.) In order to handle extra constant constraints as described above, we'd need to ignore cases where next_cl itself is NO_REGS. This brings me on to memory and address constraints... The comment says: /* ??? what about memory */ At the moment we return NO_REGS for target-independent memory constraints like "m", "o" and "g", because of the default case. The handling of next_cl means that we effectively do the same for extra memory constraints, since next_cl is always NO_REGS for them. That seems reasonable to me and I'm not trying to change it here. The patch just makes the current choice explicit by checking for extra memory constraints. Likewise we return NO_REGS for 'p' and (indirectly) for extra address constraints. This too makes sense, since I don't think we support a singleton BASE_REG_CLASS. Again the patch makes that explicit. So all in all, the patch only really affects two cases: extra constant constraints and the (probably rare) situation that there is a singleton register constraint alongside disabled/NO_REGS register constraints. The former does occur on Alpha and MIPS. E.g.: (define_insn "*call_osf_1" [(call (mem:DI (match_operand:DI 0 "call_operand" "c,R,s")) (match_operand 1)) (use (reg:DI 29)) (clobber (reg:DI 26))] "! TARGET_EXPLICIT_RELOCS && TARGET_ABI_OSF" "@ jsr $26,($27),0\;ldgp $29,0($26) bsr $26,$%0..ng jsr $26,%0\;ldgp $29,0($26)" [(set_attr "type" "jsr") (set_attr "length" "12,*,16")]) Here "c" is a singleton register class (the ABI-defined indirect call register). "s" is the standard constraint for symbolic addresses and "R" is an extra constant constraint. If the call address is a register, single_reg_class currently returns NO_REGS because of the way that "R" is handled via next_cl==NO_REGS. After the patch it returns the singleton register class for "c" instead. I think this is the right behaviour, since indirect calls really do require a single register; the other constraints are for direct calls instead. It looks like there's a missing break after the 'G' and 'H' handling. Tested on x86_64-linux-gnu. I also did an assembly comparison for a range of targets and Alpha and MIPS seemed to be the only ones affected. OK to install? Sorry for the long write-up... Thanks, Richard gcc/ * ira-lives.c (single_reg_class): Add missing break. Return NO_REGS for extra address and memory constraints, not just 'p' and TARGET_MEM_CONSTRAINT. Likewise if the operand is a constant or equivalent to a constant and if it matches an extra constraint. Ignore other non-register operands.