Re: [PATCH 8/8] Add a common .md file and define standard constraints there
On Sat, 2014-06-14 at 08:49 +0100, Richard Sandiford wrote: The patch below fixes the testcase. Please could you give it a spin and see whether there's any other fallout? I assume this would have shown up in a testsuite run if you'd been able to get that far. Thanks, Richard The patch fixed my build and the testsuite results looked good too. Steve Ellcey sell...@mips.com
Re: [PATCH 8/8] Add a common .md file and define standard constraints there
Steve Ellcey sell...@mips.com writes: Richard, Something in these constraint patches broke my mips16 build (I cannot build glibc in mips16 mode). I have cut down a test case and verified that the problem started with this checkin: 2014-06-11 Richard Sandiford rdsandif...@googlemail.com * common.md: New file. * doc/md.texi: Update description of generic, machine-independent constraints. * config/s390/constraints.md (e): Delete. * Makefile.in (md_file): Include common.md. * config/m32c/t-m32c (md_file): Likewise. * genpreds.c (general_mem): New array. (etc) Attached is a small test case (its ugly but it comes from vfscanf in glibc) that fails to compile for me with these options: -mips32r2 -mips16 -mabi=32 -std=gnu99 -fgnu89-inline -O2 -c x.c Error message: /tmp/ccAltddb.s: Assembler messages: /tmp/ccAltddb.s:23: Error: invalid operands `sb $3,24($sp)' The problem here is that mips_regno_mode_ok_for_base_p allows invalid hard registers as bases if !strict_p. It was always a bit of a hack and the reason it was added no longer applies. Robert's LRA patch already included a fix, so maybe we should just apply that part now. The patch below fixes the testcase. Please could you give it a spin and see whether there's any other fallout? I assume this would have shown up in a testsuite run if you'd been able to get that far. Thanks, Richard 2014-03-26 Robert Suchanek robert.sucha...@imgtec.com * config/mips/mips.c (mips_regno_mode_ok_for_base_p): Remove use !strict_p for MIPS16. diff --git gcc/config/mips/mips.c gcc/config/mips/mips.c index 45256e9..81b6c26 100644 --- gcc/config/mips/mips.c +++ gcc/config/mips/mips.c @@ -2241,22 +2241,9 @@ mips_regno_mode_ok_for_base_p (int regno, enum machine_mode mode, return true; /* In MIPS16 mode, the stack pointer can only address word and doubleword - values, nothing smaller. There are two problems here: - - (a) Instantiating virtual registers can introduce new uses of the - stack pointer. If these virtual registers are valid addresses, - the stack pointer should be too. - - (b) Most uses of the stack pointer are not made explicit until - FRAME_POINTER_REGNUM and ARG_POINTER_REGNUM have been eliminated. - We don't know until that stage whether we'll be eliminating to the - stack pointer (which needs the restriction) or the hard frame - pointer (which doesn't). - - All in all, it seems more consistent to only enforce this restriction - during and after reload. */ + values, nothing smaller. */ if (TARGET_MIPS16 regno == STACK_POINTER_REGNUM) -return !strict_p || GET_MODE_SIZE (mode) == 4 || GET_MODE_SIZE (mode) == 8; +return GET_MODE_SIZE (mode) == 4 || GET_MODE_SIZE (mode) == 8; return TARGET_MIPS16 ? M16_REG_P (regno) : GP_REG_P (regno); } l
Re: [PATCH 8/8] Add a common .md file and define standard constraints there
Segher Boessenkool seg...@kernel.crashing.org writes: * cris, m68k, pdp11, and vax actually use g. So it won't be all that much work to completely get rid of g. Do we want that? Is it simply a matter of replacing “g” by “mri”? That’s what the doc suggests. Or is there more to the story than that? As far as I know g and rmi are equivalent, yes. g is easier to type and read if you use it a lot (only ancient targets really); the compiler will probably become somewhat slower for those targets, and perhaps somewhat faster for all others. Hard to say without doing the work and measuring the result :-) FWIW, I had a follow-on patch that created the recog_op_alt data at build time and made the constraints field of that structure point to CONSTRAINT_* bytes rather than raw strings. That involved converting g to rmi like you say and also meant adding CONSTRAINT_*s for # and ?. (Other non-operand characters can be dropped since the information is given directly in the recog_op_alt.) It didn't really seem to be much of a win though. Richard
Re: [PATCH 8/8] Add a common .md file and define standard constraints there
Richard,
Something in these constraint patches broke my mips16 build (I cannot
build glibc in mips16 mode). I have cut down a test case and verified
that the problem started with this checkin:
2014-06-11 Richard Sandiford rdsandif...@googlemail.com
* common.md: New file.
* doc/md.texi: Update description of generic, machine-independent
constraints.
* config/s390/constraints.md (e): Delete.
* Makefile.in (md_file): Include common.md.
* config/m32c/t-m32c (md_file): Likewise.
* genpreds.c (general_mem): New array.
(etc)
Attached is a small test case (its ugly but it comes from vfscanf in glibc) that
fails to compile for me with these options:
-mips32r2 -mips16 -mabi=32 -std=gnu99 -fgnu89-inline -O2 -c x.c
Error message:
/tmp/ccAltddb.s: Assembler messages:
/tmp/ccAltddb.s:23: Error: invalid operands `sb $3,24($sp)'
Steve Ellcey
sell...@mips.com
extern long long __mips16_syscall4(long, long, long, long, long);
typedef unsigned int size_t;
union __mips16_syscall_return {
long long val;
};
typedef struct {
int lock;
}
_IO_lock_t;
struct _IO_FILE {
_IO_lock_t *_lock;
};
typedef struct _IO_FILE _IO_FILE;
int _IO_vfscanf_internal (_IO_FILE *s) {
int c = 0;
char *wp = ((void *)0);
size_t wpsize;
wp = (char *) (__typeof (wp)) ({
size_t __newlen = 32; /* (((newsize * sizeof (char)) + 15) -16); */
char *__newbuf = __builtin_alloca (__newlen);
__newbuf;
}
);
wp[wpsize++] = (c);
((void) ({
int *__futex = (((*(s)-_lock).lock));
int __val = ({
__typeof (*__futex) __atg1_result;
__atg1_result;
}
);
if (__builtin_expect (__val 1, 0)) ({
long int __ret;
__ret = ({
union __mips16_syscall_return ret;
ret.val = __mips16_syscall4 ((long) ((long) (__futex)), (long) (((1) | 128)), (long) ((1)), (long) (0), (long) ((4000 + 238)));
}
);
}
);
}
));
}
Re: [PATCH 8/8] Add a common .md file and define standard constraints there
On Thu, Jun 05, 2014 at 10:43:25PM +0100, Richard Sandiford wrote: This final patch uses a common .md file to define all standard constraints except 'g'. I had a look at what targets still use g. Note: there can be errors in this, it's all based on \g[,] :-) * frv and mcore use g in commented-out patterns; * cr16, mcore, picochip, rl78, and sh use g where they mean rm or m; * m68k uses it (in a dbne pattern) where the C template splits the r, m, i cases again; * bfin, fr30, h8300, m68k, rs6000, and v850 use it as the second operand (# bytes pushed) of the call patterns; that operand is unused in all these cases, could just be ; * cris, m68k, pdp11, and vax actually use g. So it won't be all that much work to completely get rid of g. Do we want that? Segher
Re: [PATCH 8/8] Add a common .md file and define standard constraints there
On Jun 12, 2014, at 3:24 PM, Segher Boessenkool seg...@kernel.crashing.org wrote: On Thu, Jun 05, 2014 at 10:43:25PM +0100, Richard Sandiford wrote: This final patch uses a common .md file to define all standard constraints except 'g'. I had a look at what targets still use g. Note: there can be errors in this, it's all based on \g[,] :-) * frv and mcore use g in commented-out patterns; * cr16, mcore, picochip, rl78, and sh use g where they mean rm or m; * m68k uses it (in a dbne pattern) where the C template splits the r, m, i cases again; * bfin, fr30, h8300, m68k, rs6000, and v850 use it as the second operand (# bytes pushed) of the call patterns; that operand is unused in all these cases, could just be ; * cris, m68k, pdp11, and vax actually use g. So it won't be all that much work to completely get rid of g. Do we want that? Is it simply a matter of replacing “g” by “mri”? That’s what the doc suggests. Or is there more to the story than that? paul
Re: [PATCH 8/8] Add a common .md file and define standard constraints there
* cris, m68k, pdp11, and vax actually use g. So it won't be all that much work to completely get rid of g. Do we want that? Is it simply a matter of replacing “g” by “mri”? That’s what the doc suggests. Or is there more to the story than that? As far as I know g and rmi are equivalent, yes. g is easier to type and read if you use it a lot (only ancient targets really); the compiler will probably become somewhat slower for those targets, and perhaps somewhat faster for all others. Hard to say without doing the work and measuring the result :-) Segher
Re: [PATCH 8/8] Add a common .md file and define standard constraints there
Thanks for the reviews. Jeff Law l...@redhat.com writes: Does the comment before indep_constraints in genoutput need updating? The constraints in common.md are machine independent, but aren't listed in indep_constraints in genoutput.c Yeah, good catch. I changed it to: /* All machine-independent constraint characters (except digits) that are handled outside the define*_constraint mechanism. */ static const char indep_constraints[] = ,=+%*?!#g; Also in genpreds.c: /* Contraint letters that have a special meaning and that cannot be used in define*_constraints. */ static const char generic_constraint_letters[] = g; Richard
Re: [PATCH 8/8] Add a common .md file and define standard constraints there
On 06/05/14 15:43, Richard Sandiford wrote: This final patch uses a common .md file to define all standard constraints except 'g'. It then gets rid of explicit case statements for the standard constraints, except in two cases: (1) recog.c:asm_operand_ok still needs to handle 'o' specially for targets like ia64 that don't have offsettable addresses. See the comment there for justification. (2) the trickier cases in reload. I'm not changing those more than I have to. Can't argue with #2 ;-) reload gets less and less important every day, so I see less and less value hacking too much on it. I did wonder about defining a new rtl construct that could be used for 'g', so that even that special case goes away. In the end I think it would be a false abstraction though. No other constraint allows (or IMO should allow) all three of a register class, a base-reloadable memory and a constant, so handling it in the lookup_constraint paths would make things more complicated rather than less. OK. Note that the s390 'e' constraint is TARGET_MEM_CONSTRAINT, which is now defined in the common file. I put the common .md file in the main gcc/ directory by analogy with defaults.h and common.opt. It could instead go in config/ or config/common/, if those sound better. Seems fine to me, I don't feel a need to bikeshed here. Does the comment before indep_constraints in genoutput need updating? The constraints in common.md are machine independent, but aren't listed in indep_constraints in genoutput.c Approved with whatever language you want to use for that comment. Jeff
[PATCH 8/8] Add a common .md file and define standard constraints there
This final patch uses a common .md file to define all standard constraints except 'g'. It then gets rid of explicit case statements for the standard constraints, except in two cases: (1) recog.c:asm_operand_ok still needs to handle 'o' specially for targets like ia64 that don't have offsettable addresses. See the comment there for justification. (2) the trickier cases in reload. I'm not changing those more than I have to. I did wonder about defining a new rtl construct that could be used for 'g', so that even that special case goes away. In the end I think it would be a false abstraction though. No other constraint allows (or IMO should allow) all three of a register class, a base-reloadable memory and a constant, so handling it in the lookup_constraint paths would make things more complicated rather than less. Note that the s390 'e' constraint is TARGET_MEM_CONSTRAINT, which is now defined in the common file. I put the common .md file in the main gcc/ directory by analogy with defaults.h and common.opt. It could instead go in config/ or config/common/, if those sound better. Richard gcc/ * common.md: New file. * doc/md.texi: Update description of generic, machine-independent constraints. * config/s390/constraints.md (e): Delete. * Makefile.in (md_file): Include common.md. * config/m32c/t-m32c (md_file): Likewise. * genpreds.c (general_mem): New array. (generic_constraint_letters): Remove constraints now defined by common.md. (add_constraint): Map TARGET_MEM_CONSTRAINT to general_mem. Allow the first character to be '' or '' as well. * genoutput.c (general_mem): New array. (indep_constraints): Remove constraints now defined by common.md. (note_constraint): Map TARGET_MEM_CONSTRAINT to general_mem. Remove special handling of 'm'. * ira-costs.c (record_reg_classes): Remove special handling of constraints now defined by common.md. * ira.c (ira_setup_alts, ira_get_dup_out_num): Likewise. * ira-lives.c (single_reg_class): Likewise. (ira_implicitly_set_insn_hard_regs): Likewise. * lra-constraints.c (reg_class_from_constraints): Likewise. (process_alt_operands, process_address, curr_insn_transform): Likewise. * postreload.c (reload_cse_simplify_operands): Likewise. * reload.c (push_secondary_reload, scratch_reload_class) (find_reloads, alternative_allows_const_pool_ref): Likewise. * reload1.c (maybe_fix_stack_asms): Likewise. * targhooks.c (default_secondary_reload): Likewise. * stmt.c (parse_output_constraint): Likewise. * recog.c (preprocess_constraints): Likewise. (constrain_operands, peep2_find_free_register): Likewise. (asm_operand_ok): Likewise, but add a comment saying why 'o' must be handled specially. Index: gcc/common.md === --- /dev/null 2014-05-18 17:42:44.871287828 +0100 +++ gcc/common.md 2014-06-05 22:40:45.977752252 +0100 @@ -0,0 +1,95 @@ +;; Common GCC machine description file, shared by all targets. +;; Copyright (C) 2014 Free Software Foundation, Inc. +;; +;; This file is part of GCC. +;; +;; GCC is free software; you can redistribute it and/or modify +;; it under the terms of the GNU General Public License as published by +;; the Free Software Foundation; either version 3, or (at your option) +;; any later version. +;; +;; GCC is distributed in the hope that it will be useful, +;; but WITHOUT ANY WARRANTY; without even the implied warranty of +;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +;; GNU General Public License for more details. +;; +;; You should have received a copy of the GNU General Public License +;; along with GCC; see the file COPYING3. If not see +;; http://www.gnu.org/licenses/. */ + +(define_register_constraint r GENERAL_REGS + Matches any general register.) + +(define_memory_constraint TARGET_MEM_CONSTRAINT + Matches any valid memory. + (and (match_code mem) + (match_test memory_address_addr_space_p (GET_MODE (op), XEXP (op, 0), +MEM_ADDR_SPACE (op) + +(define_memory_constraint o + Matches an offsettable memory reference. + (and (match_code mem) + (match_test offsettable_nonstrict_memref_p (op + +;; V matches TARGET_MEM_CONSTRAINTs that are rejected by o. +;; This means that it is not a memory constraint in the usual sense, +;; since reloading the address into a base register would make the +;; address offsettable. +(define_constraint V + Matches a non-offsettable memory reference. + (and (match_code mem) + (match_test memory_address_addr_space_p (GET_MODE (op), XEXP (op, 0), +MEM_ADDR_SPACE (op))) + (not (match_test offsettable_nonstrict_memref_p (op) + +;; Like V, this is
