Hi Thomas,

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

commit de4d7cbcf979edc095a48dff5b38d12846bdab6f
Author: Thomas Schwinge <tho...@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 <tho...@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

Reply via email to