Use gcc/coretypes.h:enum offload_abi in mkoffloads

Bernd Schmidt bschmidt@redhat.com
Mon Sep 28 11:28:00 GMT 2015


Hi Thomas,

Your patch submissions are sometimes very verbose which makes them hard 
to follow.

>> commit de4d7cbcf979edc095a48dff5b38d12846bdab6f
>> Author: Thomas Schwinge <thomas@codesourcery.com>
>> Date:   Tue Aug 4 13:12:36 2015 +0200

Cut unnecessary information such as this. git headers are uninteresting.

>>      Use gcc/coretypes.h:enum offload_abi in mkoffloads
>
> That one included unfortunate yet popular ;-) strcmp "typos":
>
>> +#define STR "-foffload-abi="
>> +      if (strncmp (argv[i], STR, strlen (STR)) == 0)
>> +	{
>> +	  if (strcmp (argv[i] + strlen (STR), "lp64"))
>> +	    offload_abi = OFFLOAD_ABI_LP64;
>> +	  else if (strcmp (argv[i] + strlen (STR), "ilp32"))
>> +	    offload_abi = OFFLOAD_ABI_ILP32;
>
> ..., so with these fixed up:
>
> --- gcc/config/i386/intelmic-mkoffload.c
> +++ gcc/config/i386/intelmic-mkoffload.c
> @@ -544,9 +544,9 @@ main (int argc, char **argv)
>   #define STR "-foffload-abi="
>         if (strncmp (argv[i], STR, strlen (STR)) == 0)
>   	{
> -	  if (strcmp (argv[i] + strlen (STR), "lp64"))
> +	  if (strcmp (argv[i] + strlen (STR), "lp64") == 0)
>   	    offload_abi = OFFLOAD_ABI_LP64;
> -	  else if (strcmp (argv[i] + strlen (STR), "ilp32"))
> +	  else if (strcmp (argv[i] + strlen (STR), "ilp32") == 0)
>   	    offload_abi = OFFLOAD_ABI_ILP32;
>   	  else
>   	    fatal_error (input_location,
> --- gcc/config/nvptx/mkoffload.c
> +++ gcc/config/nvptx/mkoffload.c
> @@ -1013,9 +1013,9 @@ main (int argc, char **argv)
>   #define STR "-foffload-abi="
>         if (strncmp (argv[i], STR, strlen (STR)) == 0)
>   	{
> -	  if (strcmp (argv[i] + strlen (STR), "lp64"))
> +	  if (strcmp (argv[i] + strlen (STR), "lp64") == 0)
>   	    offload_abi = OFFLOAD_ABI_LP64;
> -	  else if (strcmp (argv[i] + strlen (STR), "ilp32"))
> +	  else if (strcmp (argv[i] + strlen (STR), "ilp32") == 0)
>   	    offload_abi = OFFLOAD_ABI_ILP32;
>   	  else
>   	    fatal_error (input_location,

This confused me for a while because I thought you were proposing the 
above patch. A single line "I fixed that in the following version" would 
have been a clearer way to communicate that doesn't take up a page of space.

> ..., I'll again propose the following patch for trunk:
>
> commit 9288882278239a9d346ebde99e616185a91fbfaf
> Author: Thomas Schwinge <thomas@codesourcery.com>
> Date:   Tue Aug 4 13:12:36 2015 +0200
>
>      Use gcc/coretypes.h:enum offload_abi in mkoffloads
>
>      	gcc/
>      	* config/i386/intelmic-mkoffload.c (target_ilp32): Remove
>      	variable, replacing it with...
>      	(offload_abi): ... this new variable.  Adjust all users.
>      	* config/nvptx/mkoffload.c (target_ilp32, offload_abi): Likewise.
> ---
>   gcc/config/i386/intelmic-mkoffload.c |   90 +++++++++++++++++++++++-----------
>   gcc/config/nvptx/mkoffload.c         |   56 +++++++++++++++------
>   2 files changed, 101 insertions(+), 45 deletions(-)

Just the ChangeLog please, not the other noise.

> +      abort ();

Can we have gcc_unreachable() in these tools?

Other than that, it looks ok but it also doesn't seem to do anything. 
Are you intending to add more ABIs?


Bernd



More information about the Gcc-patches mailing list