| Message ID | 1465807102-8941-1-git-send-email-krebbel@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Jun 13, 2016 at 10:38:22AM +0200, Andreas Krebbel wrote: > Committed to GCC 5 and mainline branches. What about gcc-6-branch? It also has MAX_ARGS 5, and case for arity 6. > gcc/ChangeLog: > > 2016-06-13 Andreas Krebbel <krebbel@linux.vnet.ibm.com> > > PR target/71379 > * config/s390/s390.c (s390_expand_builtin): Increase MAX_ARGS by > one. > --- > gcc/config/s390/s390.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/gcc/config/s390/s390.c b/gcc/config/s390/s390.c > index 48b8222..ee0187c 100644 > --- a/gcc/config/s390/s390.c > +++ b/gcc/config/s390/s390.c > @@ -791,7 +791,7 @@ s390_expand_builtin (tree exp, rtx target, rtx subtarget ATTRIBUTE_UNUSED, > machine_mode mode ATTRIBUTE_UNUSED, > int ignore ATTRIBUTE_UNUSED) > { > -#define MAX_ARGS 5 > +#define MAX_ARGS 6 > > tree fndecl = TREE_OPERAND (CALL_EXPR_FN (exp), 0); > unsigned int fcode = DECL_FUNCTION_CODE (fndecl); > -- > 1.9.1 Jakub
On 06/13/2016 10:43 AM, Jakub Jelinek wrote: > On Mon, Jun 13, 2016 at 10:38:22AM +0200, Andreas Krebbel wrote: >> Committed to GCC 5 and mainline branches. > > What about gcc-6-branch? It also has MAX_ARGS 5, and case for arity 6. Done. -Andreas-
On Mon, Jun 13, 2016 at 10:51:16AM +0200, Andreas Krebbel wrote: > On 06/13/2016 10:43 AM, Jakub Jelinek wrote: > > On Mon, Jun 13, 2016 at 10:38:22AM +0200, Andreas Krebbel wrote: > >> Committed to GCC 5 and mainline branches. > > > > What about gcc-6-branch? It also has MAX_ARGS 5, and case for arity 6. > > Done. Also, it isn't clear to me, are there any s390 builtins right now that actually have 6 arguments (my reading is that you don't count the return value into that)? I.e. beyond the bootstrap issues, is the change actually fixing expansion of any builtins (there is if (arity >= MAX_ARGS) check), or is the arity 6 case there just for potential further builtins? My confusion comes from s390-builtin*.def using e.g. DEF_FN_TYPE_6 which looks to me like actually 5 argument builtin type where the first type is the return type. Wouldn't e.g. gcc/builtin-types.def call it DEF_FUNCTION_TYPE_5 (rather than _6)? Also, where is e.g. __builtin_s390_vstrcbs (as randomly chosen builtin using DEF_FN_TYPE_6) covered in the testsuite? Jakub
On 06/13/2016 11:01 AM, Jakub Jelinek wrote: > Also, it isn't clear to me, are there any s390 builtins right now that > actually have 6 arguments (my reading is that you don't count the return > value into that)? I.e. beyond the bootstrap issues, is the change actually > fixing expansion of any builtins (there is if (arity >= MAX_ARGS) check), > or is the arity 6 case there just for potential further builtins? No, it doesn't fix a problem with builtin expansion. I've only backported the mainline patch because it was inconsistent and there might problems arise with warnings as well. I could also have removed the arity == 6 case. > My confusion comes from s390-builtin*.def using e.g. DEF_FN_TYPE_6 > which looks to me like actually 5 argument builtin type where the first type > is the return type. Wouldn't e.g. gcc/builtin-types.def call it > DEF_FUNCTION_TYPE_5 (rather than _6)? Yes. It is inconsistent to builtin-types.def. Not sure if it is worth fixing it. > Also, where is e.g. __builtin_s390_vstrcbs (as randomly chosen builtin > using DEF_FN_TYPE_6) covered in the testsuite? I test the builtins with a script which generates the testcases from s390-builtins.def. The result are about 10000 testcases I didn't want to check in. -Andreas-
On Mon, Jun 13, 2016 at 12:35:32PM +0200, Andreas Krebbel wrote: > On 06/13/2016 11:01 AM, Jakub Jelinek wrote: > > Also, it isn't clear to me, are there any s390 builtins right now that > > actually have 6 arguments (my reading is that you don't count the return > > value into that)? I.e. beyond the bootstrap issues, is the change actually > > fixing expansion of any builtins (there is if (arity >= MAX_ARGS) check), > > or is the arity 6 case there just for potential further builtins? > > No, it doesn't fix a problem with builtin expansion. I've only backported the mainline patch > because it was inconsistent and there might problems arise with warnings as well. I could also have > removed the arity == 6 case. Ok, glad to hear that that I don't have to rush changing the 4.8-RH backport, of course it won't hurt to fix it up eventually. > > My confusion comes from s390-builtin*.def using e.g. DEF_FN_TYPE_6 > > which looks to me like actually 5 argument builtin type where the first type > > is the return type. Wouldn't e.g. gcc/builtin-types.def call it > > DEF_FUNCTION_TYPE_5 (rather than _6)? > > Yes. It is inconsistent to builtin-types.def. Not sure if it is worth fixing it. I think it wouldn't hurt, it would improve code readability. And it affects just the single source file. > > Also, where is e.g. __builtin_s390_vstrcbs (as randomly chosen builtin > > using DEF_FN_TYPE_6) covered in the testsuite? > > I test the builtins with a script which generates the testcases from s390-builtins.def. The result > are about 10000 testcases I didn't want to check in. AFAIK other targets test all builtins at least some way. 10000 testcases is excessive (though e.g. for i386 there are almost 5000 testcases, and I think about half of them might be testing the builtins), but i386 e.g. has testcases that include all the intrinsics headers and force all builtins inlines to be actually not inlines where possible; or you could e.g. generate a testcase that just tries to use all the builtins with all the possible argument types, without actually trying to run it, just try to assemble it. Or 100 of testcases which each test 100 of cases? BTW, looking at vecintrin.h, the GNU compiler hardware transactional execution intrinsics comment looks like pasto from htmintrin.h. Jakub
On 06/13/2016 12:49 PM, Jakub Jelinek wrote: ... >> Yes. It is inconsistent to builtin-types.def. Not sure if it is worth fixing it. > > I think it wouldn't hurt, it would improve code readability. > And it affects just the single source file. Done. https://gcc.gnu.org/ml/gcc-patches/2016-06/msg00948.html ... > AFAIK other targets test all builtins at least some way. > 10000 testcases is excessive (though e.g. for i386 there are almost 5000 > testcases, and I think about half of them might be testing the builtins), > but i386 e.g. has testcases that include all the intrinsics headers and > force all builtins inlines to be actually not inlines where possible; > or you could e.g. generate a testcase that just tries to use all the > builtins with all the possible argument types, without actually trying > to run it, just try to assemble it. > Or 100 of testcases which each test 100 of cases? I'll have a look whether I can rework the generator a bit. > BTW, looking at vecintrin.h, the > GNU compiler hardware transactional execution intrinsics > comment looks like pasto from htmintrin.h. Fixed. https://gcc.gnu.org/ml/gcc-patches/2016-06/msg00947.html -Andreas-
diff --git a/gcc/config/s390/s390.c b/gcc/config/s390/s390.c index 48b8222..ee0187c 100644 --- a/gcc/config/s390/s390.c +++ b/gcc/config/s390/s390.c @@ -791,7 +791,7 @@ s390_expand_builtin (tree exp, rtx target, rtx subtarget ATTRIBUTE_UNUSED, machine_mode mode ATTRIBUTE_UNUSED, int ignore ATTRIBUTE_UNUSED) { -#define MAX_ARGS 5 +#define MAX_ARGS 6 tree fndecl = TREE_OPERAND (CALL_EXPR_FN (exp), 0); unsigned int fcode = DECL_FUNCTION_CODE (fndecl);