[PATCH] D69620: Add AIX assembler support

[David Tenty via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG3ebfab709583: Add AIX assembler support (authored by 
stevewan, committed by daltenty).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69620/new/

https://reviews.llvm.org/D69620

Files:
  clang/lib/Driver/ToolChains/AIX.cpp
  clang/lib/Driver/ToolChains/AIX.h
  clang/test/Driver/Inputs/aix_ppc_tree/dummy0.s
  clang/test/Driver/Inputs/aix_ppc_tree/dummy1.s
  clang/test/Driver/Inputs/aix_ppc_tree/dummy2.s
  clang/test/Driver/aix-as.c

Index: clang/test/Driver/aix-as.c
===
--- /dev/null
+++ clang/test/Driver/aix-as.c
@@ -0,0 +1,73 @@
+// General tests that as(1) invocations on AIX targets are sane. Note that we
+// only test assembler functionalities in this suite.
+
+// Check powerpc-ibm-aix7.1.0.0, 32-bit.
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN: -target powerpc-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS32 %s
+// CHECK-AS32-NOT: warning:
+// CHECK-AS32: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc-ibm-aix7.1.0.0"
+// CHECK-AS32: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS32: "-a32" 
+// CHECK-AS32: "-u" 
+// CHECK-AS32: "-many" 
+
+// Check powerpc64-ibm-aix7.1.0.0, 64-bit.
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN: -target powerpc64-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS64 %s
+// CHECK-AS64-NOT: warning:
+// CHECK-AS64: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc64-ibm-aix7.1.0.0"
+// CHECK-AS64: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS64: "-a64" 
+// CHECK-AS64: "-u" 
+// CHECK-AS64: "-many"
+
+
+// Check powerpc-ibm-aix7.1.0.0, 32-bit. -Xassembler  option. 
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN: -Xassembler -w \
+// RUN: -target powerpc-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS32-Xassembler %s
+// CHECK-AS32-Xassembler-NOT: warning:
+// CHECK-AS32-Xassembler: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc-ibm-aix7.1.0.0"
+// CHECK-AS32-Xassembler: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS32-Xassembler: "-a32" 
+// CHECK-AS32-Xassembler: "-u" 
+// CHECK-AS32-Xassembler: "-many"
+// CHECK-AS32-Xassembler: "-w"
+
+// Check powerpc64-ibm-aix7.1.0.0, 64-bit. -Wa,, option.
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN: -Wa,-v,-w \
+// RUN: -target powerpc64-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS64-Wa %s
+// CHECK-AS64-Wa-NOT: warning:
+// CHECK-AS64-Wa: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc64-ibm-aix7.1.0.0"
+// CHECK-AS64-Wa: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS64-Wa: "-a64" 
+// CHECK-AS64-Wa: "-u" 
+// CHECK-AS64-Wa: "-many"
+// CHECK-AS64-Wa: "-v"
+// CHECK-AS64-Wa: "-w"
+
+// Check powerpc-ibm-aix7.1.0.0, 32-bit. Multiple input files.
+// RUN: %clang -no-canonical-prefixes -### -c \
+// RUN: %S/Inputs/aix_ppc_tree/dummy0.s \
+// RUN: %S/Inputs/aix_ppc_tree/dummy1.s \
+// RUN: %S/Inputs/aix_ppc_tree/dummy2.s 2>&1 \
+// RUN: -target powerpc-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS32-MultiInput %s
+// CHECK-AS32-MultiInput-NOT: warning:
+// CHECK-AS32-MultiInput: "{{.*}}as{{(.exe)?}}"
+// CHECK-AS32-MultiInput: "-a32"
+// CHECK-AS32-MultiInput: "-u"
+// CHECK-AS32-MultiInput: "-many"
+// CHECK-AS32-MultiInput: "{{.*}}as{{(.exe)?}}"
+// CHECK-AS32-MultiInput: "-a32"
+// CHECK-AS32-MultiInput: "-u"
+// CHECK-AS32-MultiInput: "-many"
+// CHECK-AS32-MultiInput: "{{.*}}as{{(.exe)?}}"
+// CHECK-AS32-MultiInput: "-a32"
+// CHECK-AS32-MultiInput: "-u"
+// CHECK-AS32-MultiInput: "-many"
Index: clang/lib/Driver/ToolChains/AIX.h
===
--- clang/lib/Driver/ToolChains/AIX.h
+++ clang/lib/Driver/ToolChains/AIX.h
@@ -16,10 +16,21 @@
 namespace driver {
 namespace tools {
 
-/// aix -- Directly call system default linker.
-// TODO: Enable direct call to system default assembler.
+/// aix -- Directly call system default assembler and linker.
 namespace aix {
 
+class LLVM_LIBRARY_VISIBILITY Assembler : public Tool {
+public:
+  Assembler(const ToolChain ) : Tool("aix::Assembler", "assembler", TC) {}
+
+  bool hasIntegratedCPP() const override { return false; }
+
+  void ConstructJob(Compilation , const JobAction ,
+const InputInfo , const InputInfoList ,
+const llvm::opt::ArgList ,
+const char *LinkingOutput) const override;
+};
+
 class LLVM_LIBRARY_VISIBILITY Linker : public Tool {
 public:
   Linker(const ToolChain ) : Tool("aix::Linker", "linker", TC) {}
@@ -53,6 +64,7 @@
   bool isPICDefaultForced() const override { return true; }
 
 protected:
+  Tool *buildAssembler() const override;
   Tool *buildLinker() const override;
 };
 
Index: clang/lib/Driver/ToolChains/AIX.cpp

[PATCH] D69620: Add AIX assembler support

[Steven Wan via Phabricator via cfe-commits]

stevewan updated this revision to Diff 231712.
stevewan added a comment.

State in the comment the expectation to driver in handling assembler source 
input when invoking as(1).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69620/new/

https://reviews.llvm.org/D69620

Files:
  clang/lib/Driver/ToolChains/AIX.cpp
  clang/lib/Driver/ToolChains/AIX.h
  clang/test/Driver/Inputs/aix_ppc_tree/dummy0.s
  clang/test/Driver/Inputs/aix_ppc_tree/dummy1.s
  clang/test/Driver/Inputs/aix_ppc_tree/dummy2.s
  clang/test/Driver/aix-as.c

Index: clang/test/Driver/aix-as.c
===
--- /dev/null
+++ clang/test/Driver/aix-as.c
@@ -0,0 +1,73 @@
+// General tests that as(1) invocations on AIX targets are sane. Note that we
+// only test assembler functionalities in this suite.
+
+// Check powerpc-ibm-aix7.1.0.0, 32-bit.
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN: -target powerpc-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS32 %s
+// CHECK-AS32-NOT: warning:
+// CHECK-AS32: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc-ibm-aix7.1.0.0"
+// CHECK-AS32: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS32: "-a32" 
+// CHECK-AS32: "-u" 
+// CHECK-AS32: "-many" 
+
+// Check powerpc64-ibm-aix7.1.0.0, 64-bit.
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN: -target powerpc64-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS64 %s
+// CHECK-AS64-NOT: warning:
+// CHECK-AS64: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc64-ibm-aix7.1.0.0"
+// CHECK-AS64: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS64: "-a64" 
+// CHECK-AS64: "-u" 
+// CHECK-AS64: "-many"
+
+
+// Check powerpc-ibm-aix7.1.0.0, 32-bit. -Xassembler  option. 
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN: -Xassembler -w \
+// RUN: -target powerpc-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS32-Xassembler %s
+// CHECK-AS32-Xassembler-NOT: warning:
+// CHECK-AS32-Xassembler: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc-ibm-aix7.1.0.0"
+// CHECK-AS32-Xassembler: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS32-Xassembler: "-a32" 
+// CHECK-AS32-Xassembler: "-u" 
+// CHECK-AS32-Xassembler: "-many"
+// CHECK-AS32-Xassembler: "-w"
+
+// Check powerpc64-ibm-aix7.1.0.0, 64-bit. -Wa,, option.
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN: -Wa,-v,-w \
+// RUN: -target powerpc64-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS64-Wa %s
+// CHECK-AS64-Wa-NOT: warning:
+// CHECK-AS64-Wa: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc64-ibm-aix7.1.0.0"
+// CHECK-AS64-Wa: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS64-Wa: "-a64" 
+// CHECK-AS64-Wa: "-u" 
+// CHECK-AS64-Wa: "-many"
+// CHECK-AS64-Wa: "-v"
+// CHECK-AS64-Wa: "-w"
+
+// Check powerpc-ibm-aix7.1.0.0, 32-bit. Multiple input files.
+// RUN: %clang -no-canonical-prefixes -### -c \
+// RUN: %S/Inputs/aix_ppc_tree/dummy0.s \
+// RUN: %S/Inputs/aix_ppc_tree/dummy1.s \
+// RUN: %S/Inputs/aix_ppc_tree/dummy2.s 2>&1 \
+// RUN: -target powerpc-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS32-MultiInput %s
+// CHECK-AS32-MultiInput-NOT: warning:
+// CHECK-AS32-MultiInput: "{{.*}}as{{(.exe)?}}"
+// CHECK-AS32-MultiInput: "-a32"
+// CHECK-AS32-MultiInput: "-u"
+// CHECK-AS32-MultiInput: "-many"
+// CHECK-AS32-MultiInput: "{{.*}}as{{(.exe)?}}"
+// CHECK-AS32-MultiInput: "-a32"
+// CHECK-AS32-MultiInput: "-u"
+// CHECK-AS32-MultiInput: "-many"
+// CHECK-AS32-MultiInput: "{{.*}}as{{(.exe)?}}"
+// CHECK-AS32-MultiInput: "-a32"
+// CHECK-AS32-MultiInput: "-u"
+// CHECK-AS32-MultiInput: "-many"
Index: clang/lib/Driver/ToolChains/AIX.h
===
--- clang/lib/Driver/ToolChains/AIX.h
+++ clang/lib/Driver/ToolChains/AIX.h
@@ -16,10 +16,21 @@
 namespace driver {
 namespace tools {
 
-/// aix -- Directly call system default linker.
-// TODO: Enable direct call to system default assembler.
+/// aix -- Directly call system default assembler and linker.
 namespace aix {
 
+class LLVM_LIBRARY_VISIBILITY Assembler : public Tool {
+public:
+  Assembler(const ToolChain ) : Tool("aix::Assembler", "assembler", TC) {}
+
+  bool hasIntegratedCPP() const override { return false; }
+
+  void ConstructJob(Compilation , const JobAction ,
+const InputInfo , const InputInfoList ,
+const llvm::opt::ArgList ,
+const char *LinkingOutput) const override;
+};
+
 class LLVM_LIBRARY_VISIBILITY Linker : public Tool {
 public:
   Linker(const ToolChain ) : Tool("aix::Linker", "linker", TC) {}
@@ -53,6 +64,7 @@
   bool isPICDefaultForced() const override { return true; }
 
 protected:
+  Tool *buildAssembler() const override;
   Tool *buildLinker() const override;
 };
 
Index: clang/lib/Driver/ToolChains/AIX.cpp

[PATCH] D69620: Add AIX assembler support

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast accepted this revision.
hubert.reinterpretcast added a comment.

LGTM with minor change request to a comment. Thanks.




Comment at: clang/lib/Driver/ToolChains/AIX.cpp:68
+  if (Inputs.size() != 1)
+llvm_unreachable("Invalid number of input files.");
+  const InputInfo  = Inputs[0];

stevewan wrote:
> hubert.reinterpretcast wrote:
> > `llvm_unreachable` is not the right solution if this can be reached by 
> > "user error". We should produce a proper error message.
> One risky condition I can think of is the user passes in multiple assembly 
> sources to the driver, which may lead to multiple assembler inputs. To verify 
> how the driver handles such a case, I added a new test into `aix-as.c` below, 
> whose results suggested that this is okay because the driver would invoke 
> `as` for each and every input files respectively. Looking into the code, the 
> driver would construct an action list for each input files individually, 
> which again matches the behaviour we observed in the testing results. That 
> said, I believe the `llvm_unreachable` here is indeed not reachable by "user 
> errors" like this, and if it triggers, it's likly an internal error. 
Thanks. Can we indicate that we are expecting the driver to invoke `as` 
separately for each assembler source file in the comment?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69620/new/

https://reviews.llvm.org/D69620



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69620: Add AIX assembler support

[Steven Wan via Phabricator via cfe-commits]

stevewan marked 3 inline comments as done.
stevewan added inline comments.



Comment at: clang/lib/Driver/ToolChains/AIX.cpp:68
+  if (Inputs.size() != 1)
+llvm_unreachable("Invalid number of input files.");
+  const InputInfo  = Inputs[0];

hubert.reinterpretcast wrote:
> `llvm_unreachable` is not the right solution if this can be reached by "user 
> error". We should produce a proper error message.
One risky condition I can think of is the user passes in multiple assembly 
sources to the driver, which may lead to multiple assembler inputs. To verify 
how the driver handles such a case, I added a new test into `aix-as.c` below, 
whose results suggested that this is okay because the driver would invoke `as` 
for each and every input files respectively. Looking into the code, the driver 
would construct an action list for each input files individually, which again 
matches the behaviour we observed in the testing results. That said, I believe 
the `llvm_unreachable` here is indeed not reachable by "user errors" like this, 
and if it triggers, it's likly an internal error. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69620/new/

https://reviews.llvm.org/D69620



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69620: Add AIX assembler support

[Steven Wan via Phabricator via cfe-commits]

stevewan updated this revision to Diff 231579.
stevewan added a comment.
Herald added subscribers: kbarton, nemanjai.

Add more input file handling

1. Check if the input file is of type Filename or Nothing, throw assertion 
otherwise.
2. Add a test case to verify the correct behaviour of assembler invocation when 
multiple input files are specified by the user.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69620/new/

https://reviews.llvm.org/D69620

Files:
  clang/lib/Driver/ToolChains/AIX.cpp
  clang/lib/Driver/ToolChains/AIX.h
  clang/test/Driver/Inputs/aix_ppc_tree/dummy0.s
  clang/test/Driver/Inputs/aix_ppc_tree/dummy1.s
  clang/test/Driver/Inputs/aix_ppc_tree/dummy2.s
  clang/test/Driver/aix-as.c

Index: clang/test/Driver/aix-as.c
===
--- /dev/null
+++ clang/test/Driver/aix-as.c
@@ -0,0 +1,73 @@
+// General tests that as(1) invocations on AIX targets are sane. Note that we
+// only test assembler functionalities in this suite.
+
+// Check powerpc-ibm-aix7.1.0.0, 32-bit.
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN: -target powerpc-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS32 %s
+// CHECK-AS32-NOT: warning:
+// CHECK-AS32: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc-ibm-aix7.1.0.0"
+// CHECK-AS32: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS32: "-a32" 
+// CHECK-AS32: "-u" 
+// CHECK-AS32: "-many" 
+
+// Check powerpc64-ibm-aix7.1.0.0, 64-bit.
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN: -target powerpc64-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS64 %s
+// CHECK-AS64-NOT: warning:
+// CHECK-AS64: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc64-ibm-aix7.1.0.0"
+// CHECK-AS64: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS64: "-a64" 
+// CHECK-AS64: "-u" 
+// CHECK-AS64: "-many"
+
+
+// Check powerpc-ibm-aix7.1.0.0, 32-bit. -Xassembler  option. 
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN: -Xassembler -w \
+// RUN: -target powerpc-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS32-Xassembler %s
+// CHECK-AS32-Xassembler-NOT: warning:
+// CHECK-AS32-Xassembler: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc-ibm-aix7.1.0.0"
+// CHECK-AS32-Xassembler: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS32-Xassembler: "-a32" 
+// CHECK-AS32-Xassembler: "-u" 
+// CHECK-AS32-Xassembler: "-many"
+// CHECK-AS32-Xassembler: "-w"
+
+// Check powerpc64-ibm-aix7.1.0.0, 64-bit. -Wa,, option.
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN: -Wa,-v,-w \
+// RUN: -target powerpc64-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS64-Wa %s
+// CHECK-AS64-Wa-NOT: warning:
+// CHECK-AS64-Wa: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc64-ibm-aix7.1.0.0"
+// CHECK-AS64-Wa: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS64-Wa: "-a64" 
+// CHECK-AS64-Wa: "-u" 
+// CHECK-AS64-Wa: "-many"
+// CHECK-AS64-Wa: "-v"
+// CHECK-AS64-Wa: "-w"
+
+// Check powerpc-ibm-aix7.1.0.0, 32-bit. Multiple input files.
+// RUN: %clang -no-canonical-prefixes -### -c \
+// RUN: %S/Inputs/aix_ppc_tree/dummy0.s \
+// RUN: %S/Inputs/aix_ppc_tree/dummy1.s \
+// RUN: %S/Inputs/aix_ppc_tree/dummy2.s 2>&1 \
+// RUN: -target powerpc-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS32-MultiInput %s
+// CHECK-AS32-MultiInput-NOT: warning:
+// CHECK-AS32-MultiInput: "{{.*}}as{{(.exe)?}}"
+// CHECK-AS32-MultiInput: "-a32"
+// CHECK-AS32-MultiInput: "-u"
+// CHECK-AS32-MultiInput: "-many"
+// CHECK-AS32-MultiInput: "{{.*}}as{{(.exe)?}}"
+// CHECK-AS32-MultiInput: "-a32"
+// CHECK-AS32-MultiInput: "-u"
+// CHECK-AS32-MultiInput: "-many"
+// CHECK-AS32-MultiInput: "{{.*}}as{{(.exe)?}}"
+// CHECK-AS32-MultiInput: "-a32"
+// CHECK-AS32-MultiInput: "-u"
+// CHECK-AS32-MultiInput: "-many"
Index: clang/lib/Driver/ToolChains/AIX.h
===
--- clang/lib/Driver/ToolChains/AIX.h
+++ clang/lib/Driver/ToolChains/AIX.h
@@ -16,10 +16,21 @@
 namespace driver {
 namespace tools {
 
-/// aix -- Directly call system default linker.
-// TODO: Enable direct call to system default assembler.
+/// aix -- Directly call system default assembler and linker.
 namespace aix {
 
+class LLVM_LIBRARY_VISIBILITY Assembler : public Tool {
+public:
+  Assembler(const ToolChain ) : Tool("aix::Assembler", "assembler", TC) {}
+
+  bool hasIntegratedCPP() const override { return false; }
+
+  void ConstructJob(Compilation , const JobAction ,
+const InputInfo , const InputInfoList ,
+const llvm::opt::ArgList ,
+const char *LinkingOutput) const override;
+};
+
 class LLVM_LIBRARY_VISIBILITY Linker : public Tool {
 public:
   Linker(const ToolChain ) : Tool("aix::Linker", "linker", TC) {}
@@ -53,6 +64,7 @@
   bool 

[PATCH] D69620: Add AIX assembler support

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/Driver/ToolChains/AIX.cpp:70
+  const InputInfo  = Inputs[0];
+  if (II.isFilename())
+CmdArgs.push_back(II.getFilename());

Should is also be checked as being either a filename or "nothing"?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69620/new/

https://reviews.llvm.org/D69620



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69620: Add AIX assembler support

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/Driver/ToolChains/AIX.cpp:68
+  if (Inputs.size() != 1)
+llvm_unreachable("Invalid number of input files.");
+  const InputInfo  = Inputs[0];

`llvm_unreachable` is not the right solution if this can be reached by "user 
error". We should produce a proper error message.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69620/new/

https://reviews.llvm.org/D69620



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69620: Add AIX assembler support

[Steven Wan via Phabricator via cfe-commits]

stevewan updated this revision to Diff 231557.
stevewan added a comment.

Correct a typo in a comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69620/new/

https://reviews.llvm.org/D69620

Files:
  clang/lib/Driver/ToolChains/AIX.cpp
  clang/lib/Driver/ToolChains/AIX.h
  clang/test/Driver/aix-as.c

Index: clang/test/Driver/aix-as.c
===
--- /dev/null
+++ clang/test/Driver/aix-as.c
@@ -0,0 +1,52 @@
+// General tests that as(1) invocations on AIX targets are sane. Note that we
+// only test assembler functionalities in this suite.
+
+// Check powerpc-ibm-aix7.1.0.0, 32-bit.
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN: -target powerpc-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS32 %s
+// CHECK-AS32-NOT: warning:
+// CHECK-AS32: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc-ibm-aix7.1.0.0"
+// CHECK-AS32: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS32: "-a32" 
+// CHECK-AS32: "-u" 
+// CHECK-AS32: "-many" 
+
+// Check powerpc64-ibm-aix7.1.0.0, 64-bit.
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN: -target powerpc64-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS64 %s
+// CHECK-AS64-NOT: warning:
+// CHECK-AS64: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc64-ibm-aix7.1.0.0"
+// CHECK-AS64: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS64: "-a64" 
+// CHECK-AS64: "-u" 
+// CHECK-AS64: "-many"
+
+
+// Check powerpc-ibm-aix7.1.0.0, 32-bit. -Xassembler  option. 
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN: -Xassembler -w \
+// RUN: -target powerpc-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS32-Xassembler %s
+// CHECK-AS32-Xassembler-NOT: warning:
+// CHECK-AS32-Xassembler: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc-ibm-aix7.1.0.0"
+// CHECK-AS32-Xassembler: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS32-Xassembler: "-a32" 
+// CHECK-AS32-Xassembler: "-u" 
+// CHECK-AS32-Xassembler: "-many"
+// CHECK-AS32-Xassembler: "-w"
+
+// Check powerpc64-ibm-aix7.1.0.0, 64-bit. -Wa,, option.
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN: -Wa,-v,-w \
+// RUN: -target powerpc64-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS64-Wa %s
+// CHECK-AS64-Wa-NOT: warning:
+// CHECK-AS64-Wa: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc64-ibm-aix7.1.0.0"
+// CHECK-AS64-Wa: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS64-Wa: "-a64" 
+// CHECK-AS64-Wa: "-u" 
+// CHECK-AS64-Wa: "-many"
+// CHECK-AS64-Wa: "-v"
+// CHECK-AS64-Wa: "-w"
Index: clang/lib/Driver/ToolChains/AIX.h
===
--- clang/lib/Driver/ToolChains/AIX.h
+++ clang/lib/Driver/ToolChains/AIX.h
@@ -16,10 +16,21 @@
 namespace driver {
 namespace tools {
 
-/// aix -- Directly call system default linker.
-// TODO: Enable direct call to system default assembler.
+/// aix -- Directly call system default assembler and linker.
 namespace aix {
 
+class LLVM_LIBRARY_VISIBILITY Assembler : public Tool {
+public:
+  Assembler(const ToolChain ) : Tool("aix::Assembler", "assembler", TC) {}
+
+  bool hasIntegratedCPP() const override { return false; }
+
+  void ConstructJob(Compilation , const JobAction ,
+const InputInfo , const InputInfoList ,
+const llvm::opt::ArgList ,
+const char *LinkingOutput) const override;
+};
+
 class LLVM_LIBRARY_VISIBILITY Linker : public Tool {
 public:
   Linker(const ToolChain ) : Tool("aix::Linker", "linker", TC) {}
@@ -53,6 +64,7 @@
   bool isPICDefaultForced() const override { return true; }
 
 protected:
+  Tool *buildAssembler() const override;
   Tool *buildLinker() const override;
 };
 
Index: clang/lib/Driver/ToolChains/AIX.cpp
===
--- clang/lib/Driver/ToolChains/AIX.cpp
+++ clang/lib/Driver/ToolChains/AIX.cpp
@@ -20,6 +20,60 @@
 
 using namespace llvm::opt;
 
+void aix::Assembler::ConstructJob(Compilation , const JobAction ,
+  const InputInfo ,
+  const InputInfoList ,
+  const ArgList ,
+  const char *LinkingOutput) const {
+  ArgStringList CmdArgs;
+
+  const bool IsArch32Bit = getToolChain().getTriple().isArch32Bit();
+  const bool IsArch64Bit = getToolChain().getTriple().isArch64Bit();
+  // Only support 32 and 64 bit.
+  if (!IsArch32Bit && !IsArch64Bit)
+llvm_unreachable("Unsupported bit width value.");
+
+  // Specify the mode in which the as(1) command operates.
+  if (IsArch32Bit) {
+CmdArgs.push_back("-a32");
+  } else {
+// Must be 64-bit, otherwise asserted already.
+CmdArgs.push_back("-a64");
+  }
+
+  // Accept an undefined symbol as an extern so that an error message is not
+  // displayed. 

[PATCH] D69620: Add AIX assembler support

[David Tenty via Phabricator via cfe-commits]

daltenty accepted this revision.
daltenty added a comment.
This revision is now accepted and ready to land.

Other than minor nit, LGTM




Comment at: clang/lib/Driver/ToolChains/AIX.cpp:68
+  if (Inputs.size() != 1)
+llvm_unreachable("Invalid number of input file.");
+  const InputInfo  = Inputs[0];

nit: plural, "files"


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69620/new/

https://reviews.llvm.org/D69620



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69620: Add AIX assembler support

[Steven Wan via Phabricator via cfe-commits]

stevewan marked an inline comment as done.
stevewan added inline comments.



Comment at: clang/lib/Driver/ToolChains/AIX.cpp:28
+  const char *LinkingOutput) const {
+  claimNoWarnArgs(Args);
+  ArgStringList CmdArgs;

stevewan wrote:
> stevewan wrote:
> > Xiangling_L wrote:
> > > The definition of `claimNoWarnArgs` is to suppress warnings for some 
> > > options if they are unused, can you explain a little bit about how did 
> > > you figure out that we don't want warnings for those?
> > > 
> > > Some context of `claimNoWarnArgs`:
> > > 
> > > ```
> > > // Claim options we don't want to warn if they are unused. We do this for
> > > // options that build systems might add but are unused when assembling or 
> > > only
> > > // running the preprocessor for example.
> > > void tools::claimNoWarnArgs(const ArgList ) {
> > >   // Don't warn about unused -f(no-)?lto.  This can happen when we're
> > >   // preprocessing, precompiling or assembling.
> > >   Args.ClaimAllArgs(options::OPT_flto_EQ);
> > >   Args.ClaimAllArgs(options::OPT_flto);
> > >   Args.ClaimAllArgs(options::OPT_fno_lto);
> > > }
> > > ```
> > > 
> > The original reason was that, since we are doing assembling here, and as 
> > stated in `claimNoWarnArgs`, there might be an unused `-f(no-)?lto` when 
> > we're assembling, and we'd like to suppress the warning(s) for that. 
> > 
> > Looking into it, the three options [[ 
> > https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-flto
> >  | `flto`, `fno_lto` ]] , and [[ 
> > https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang1-flto
> >  | `flto_EQ` ]] are used to enable/disable link time optimization, but LTO 
> > is not supported by the AIX system linker. Driver would just throw error if 
> > the user passes it `-flto` or `-flto=` on AIX, so claiming them or not 
> > currently makes no actual difference as far as I'm concerned. 
> > 
> > That being said, I don't have a strong opinion either way. Let's see how 
> > other people think.
> It's probably also worth mentioning that, adding `claimNoWarnArgs` does not 
> affect those LTO options being parsed and consumed by the driver in 
> `Driver::setLTOMode`.
Turned out `Driver::BuildCompilation` would always call `Driver::setLTOMode`, 
which inherently sets the three LTO options to "claimed". This happens before 
the driver gets to assembler invocation, hence no need to call 
`claimNoWarnArgs` here.

For future reference, I'm also documenting here that `claimNoWarnArgs` was 
added in [[ https://reviews.llvm.org/rL225100 | rL225100 ]], which was 
originally posted to fix build bots that failed due to warnings from unused LTO 
options. `setLTOMode` was introduced months later in [[ 
https://reviews.llvm.org/rL250455 | rL250455 ]], which claims the three LTO 
options during `BuildCompilation`.



Comment at: clang/lib/Driver/ToolChains/AIX.cpp:61
+
+  // Specify assembler input file(s).
+  for (const auto  : Inputs)

hubert.reinterpretcast wrote:
> The AIX assembler does not accept more than one input file. If the intent is 
> to let the assembler produce the error message, then I think we should at 
> least have a comment to acknowledge the situation.
Good point. I didn't intend to leave this to assembler. Looking into it, when 
presented multiple assembly sources, the driver would invoke `as` to assemble 
each of the input files individually. This behaviour also matches that of GCC 
and XL. In case something goes wrong and the driver attempts to pass multiple 
input files to `as`, adding a check to guard the number of inputs probably 
makes sense?



Comment at: clang/test/Driver/aix-as.c:31
+// RUN: -target powerpc-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS32-PTHREAD %s
+// CHECK-AS32-PTHREAD-NOT: warning:

hubert.reinterpretcast wrote:
> I am not getting how this block is related to "pthread".
Looks like I copy-pasted the wrong thing while rebasing. Good catch, thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69620/new/

https://reviews.llvm.org/D69620



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69620: Add AIX assembler support

[Steven Wan via Phabricator via cfe-commits]

stevewan updated this revision to Diff 231000.
stevewan marked 12 inline comments as done.
stevewan added a comment.

Remove claimNoWarnArgs, limit the number of input files, and add more comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69620/new/

https://reviews.llvm.org/D69620

Files:
  clang/lib/Driver/ToolChains/AIX.cpp
  clang/lib/Driver/ToolChains/AIX.h
  clang/test/Driver/aix-as.c

Index: clang/test/Driver/aix-as.c
===
--- /dev/null
+++ clang/test/Driver/aix-as.c
@@ -0,0 +1,52 @@
+// General tests that as(1) invocations on AIX targets are sane. Note that we
+// only test assembler functionalities in this suite.
+
+// Check powerpc-ibm-aix7.1.0.0, 32-bit.
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN: -target powerpc-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS32 %s
+// CHECK-AS32-NOT: warning:
+// CHECK-AS32: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc-ibm-aix7.1.0.0"
+// CHECK-AS32: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS32: "-a32" 
+// CHECK-AS32: "-u" 
+// CHECK-AS32: "-many" 
+
+// Check powerpc64-ibm-aix7.1.0.0, 64-bit.
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN: -target powerpc64-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS64 %s
+// CHECK-AS64-NOT: warning:
+// CHECK-AS64: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc64-ibm-aix7.1.0.0"
+// CHECK-AS64: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS64: "-a64" 
+// CHECK-AS64: "-u" 
+// CHECK-AS64: "-many"
+
+
+// Check powerpc-ibm-aix7.1.0.0, 32-bit. -Xassembler  option. 
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN: -Xassembler -w \
+// RUN: -target powerpc-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS32-Xassembler %s
+// CHECK-AS32-Xassembler-NOT: warning:
+// CHECK-AS32-Xassembler: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc-ibm-aix7.1.0.0"
+// CHECK-AS32-Xassembler: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS32-Xassembler: "-a32" 
+// CHECK-AS32-Xassembler: "-u" 
+// CHECK-AS32-Xassembler: "-many"
+// CHECK-AS32-Xassembler: "-w"
+
+// Check powerpc64-ibm-aix7.1.0.0, 64-bit. -Wa,, option.
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN: -Wa,-v,-w \
+// RUN: -target powerpc64-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS64-Wa %s
+// CHECK-AS64-Wa-NOT: warning:
+// CHECK-AS64-Wa: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc64-ibm-aix7.1.0.0"
+// CHECK-AS64-Wa: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS64-Wa: "-a64" 
+// CHECK-AS64-Wa: "-u" 
+// CHECK-AS64-Wa: "-many"
+// CHECK-AS64-Wa: "-v"
+// CHECK-AS64-Wa: "-w"
Index: clang/lib/Driver/ToolChains/AIX.h
===
--- clang/lib/Driver/ToolChains/AIX.h
+++ clang/lib/Driver/ToolChains/AIX.h
@@ -16,10 +16,21 @@
 namespace driver {
 namespace tools {
 
-/// aix -- Directly call system default linker.
-// TODO: Enable direct call to system default assembler.
+/// aix -- Directly call system default assembler and linker.
 namespace aix {
 
+class LLVM_LIBRARY_VISIBILITY Assembler : public Tool {
+public:
+  Assembler(const ToolChain ) : Tool("aix::Assembler", "assembler", TC) {}
+
+  bool hasIntegratedCPP() const override { return false; }
+
+  void ConstructJob(Compilation , const JobAction ,
+const InputInfo , const InputInfoList ,
+const llvm::opt::ArgList ,
+const char *LinkingOutput) const override;
+};
+
 class LLVM_LIBRARY_VISIBILITY Linker : public Tool {
 public:
   Linker(const ToolChain ) : Tool("aix::Linker", "linker", TC) {}
@@ -53,6 +64,7 @@
   bool isPICDefaultForced() const override { return true; }
 
 protected:
+  Tool *buildAssembler() const override;
   Tool *buildLinker() const override;
 };
 
Index: clang/lib/Driver/ToolChains/AIX.cpp
===
--- clang/lib/Driver/ToolChains/AIX.cpp
+++ clang/lib/Driver/ToolChains/AIX.cpp
@@ -20,6 +20,60 @@
 
 using namespace llvm::opt;
 
+void aix::Assembler::ConstructJob(Compilation , const JobAction ,
+  const InputInfo ,
+  const InputInfoList ,
+  const ArgList ,
+  const char *LinkingOutput) const {
+  ArgStringList CmdArgs;
+
+  const bool IsArch32Bit = getToolChain().getTriple().isArch32Bit();
+  const bool IsArch64Bit = getToolChain().getTriple().isArch64Bit();
+  // Only support 32 and 64 bit.
+  if (!IsArch32Bit && !IsArch64Bit)
+llvm_unreachable("Unsupported bit width value.");
+
+  // Specify the mode in which the as(1) command operates.
+  if (IsArch32Bit) {
+CmdArgs.push_back("-a32");
+  } else {
+// Must be 64-bit, otherwise asserted already.
+CmdArgs.push_back("-a64");
+  }
+

[PATCH] D69620: Add AIX assembler support

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/lib/Driver/ToolChains/AIX.cpp:37
+
+  // Specify the mode in which the as command operates.
+  if (IsArch32Bit) {

Refer to the other comment regarding confusion of "as" with the English word.



Comment at: clang/lib/Driver/ToolChains/AIX.cpp:47
+  // displayed. Otherwise, undefined symbols are flagged with error messages.
+  CmdArgs.push_back("-u");
+

There is no FIXME here indicating the plan to remove this in the future when 
the assembly generation from the compiler no longer needs this.



Comment at: clang/lib/Driver/ToolChains/AIX.cpp:49
+
+  // Acccept any mixture of instructions.
+  CmdArgs.push_back("-many");

Minor nit: Typo: s/Acccept/Accept/;



Comment at: clang/lib/Driver/ToolChains/AIX.cpp:50
+  // Acccept any mixture of instructions.
+  CmdArgs.push_back("-many");
+

A comment should indicate that this behaviour matches that of GCC on Power for 
AIX and Linux for both the user-provided assembler source case and the 
compiler-produced assembler source case. The behaviour for XL with 
user-provided assemble source is different.



Comment at: clang/lib/Driver/ToolChains/AIX.cpp:61
+
+  // Specify assembler input file(s).
+  for (const auto  : Inputs)

The AIX assembler does not accept more than one input file. If the intent is to 
let the assembler produce the error message, then I think we should at least 
have a comment to acknowledge the situation.



Comment at: clang/test/Driver/aix-as.c:1
+// General tests that as invocations on AIX targets are sane. Note that we
+// only test assembler functionalities in this suite.

Minor nit: To avoid confusion with the English word, use
```
`as`
```
or
```
as(1)
```



Comment at: clang/test/Driver/aix-as.c:31
+// RUN: -target powerpc-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS32-PTHREAD %s
+// CHECK-AS32-PTHREAD-NOT: warning:

I am not getting how this block is related to "pthread".



Comment at: clang/test/Driver/aix-as.c:40
+
+// Check powerpc-ibm-aix7.1.0.0, 64-bit. -Wa,, option.
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \

s/powerpc/powerpc64/;



Comment at: clang/test/Driver/aix-as.c:44
+// RUN: -target powerpc64-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS64-PTHREAD %s
+// CHECK-AS64-PTHREAD-NOT: warning:

Same comment re: "pthread".


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69620/new/

https://reviews.llvm.org/D69620



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69620: Add AIX assembler support

[Steven Wan via Phabricator via cfe-commits]

stevewan marked an inline comment as done.
stevewan added inline comments.



Comment at: clang/lib/Driver/ToolChains/AIX.cpp:28
+  const char *LinkingOutput) const {
+  claimNoWarnArgs(Args);
+  ArgStringList CmdArgs;

stevewan wrote:
> Xiangling_L wrote:
> > The definition of `claimNoWarnArgs` is to suppress warnings for some 
> > options if they are unused, can you explain a little bit about how did you 
> > figure out that we don't want warnings for those?
> > 
> > Some context of `claimNoWarnArgs`:
> > 
> > ```
> > // Claim options we don't want to warn if they are unused. We do this for
> > // options that build systems might add but are unused when assembling or 
> > only
> > // running the preprocessor for example.
> > void tools::claimNoWarnArgs(const ArgList ) {
> >   // Don't warn about unused -f(no-)?lto.  This can happen when we're
> >   // preprocessing, precompiling or assembling.
> >   Args.ClaimAllArgs(options::OPT_flto_EQ);
> >   Args.ClaimAllArgs(options::OPT_flto);
> >   Args.ClaimAllArgs(options::OPT_fno_lto);
> > }
> > ```
> > 
> The original reason was that, since we are doing assembling here, and as 
> stated in `claimNoWarnArgs`, there might be an unused `-f(no-)?lto` when 
> we're assembling, and we'd like to suppress the warning(s) for that. 
> 
> Looking into it, the three options [[ 
> https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-flto
>  | `flto`, `fno_lto` ]] , and [[ 
> https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang1-flto
>  | `flto_EQ` ]] are used to enable/disable link time optimization, but LTO is 
> not supported by the AIX system linker. Driver would just throw error if the 
> user passes it `-flto` or `-flto=` on AIX, so claiming them or not 
> currently makes no actual difference as far as I'm concerned. 
> 
> That being said, I don't have a strong opinion either way. Let's see how 
> other people think.
It's probably also worth mentioning that, adding `claimNoWarnArgs` does not 
affect those LTO options being parsed and consumed by the driver in 
`Driver::setLTOMode`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69620/new/

https://reviews.llvm.org/D69620



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69620: Add AIX assembler support

[Steven Wan via Phabricator via cfe-commits]

stevewan updated this revision to Diff 230360.
stevewan marked an inline comment as done.
stevewan added a comment.

Add "-u" to accept undefined symbol as extern.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69620/new/

https://reviews.llvm.org/D69620

Files:
  clang/lib/Driver/ToolChains/AIX.cpp
  clang/lib/Driver/ToolChains/AIX.h
  clang/test/Driver/aix-as.c

Index: clang/test/Driver/aix-as.c
===
--- /dev/null
+++ clang/test/Driver/aix-as.c
@@ -0,0 +1,52 @@
+// General tests that as invocations on AIX targets are sane. Note that we
+// only test assembler functionalities in this suite.
+
+// Check powerpc-ibm-aix7.1.0.0, 32-bit.
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN: -target powerpc-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS32 %s
+// CHECK-AS32-NOT: warning:
+// CHECK-AS32: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc-ibm-aix7.1.0.0"
+// CHECK-AS32: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS32: "-a32" 
+// CHECK-AS32: "-u" 
+// CHECK-AS32: "-many" 
+
+// Check powerpc64-ibm-aix7.1.0.0, 64-bit.
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN: -target powerpc64-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS64 %s
+// CHECK-AS64-NOT: warning:
+// CHECK-AS64: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc64-ibm-aix7.1.0.0"
+// CHECK-AS64: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS64: "-a64" 
+// CHECK-AS64: "-u" 
+// CHECK-AS64: "-many"
+
+
+// Check powerpc-ibm-aix7.1.0.0, 32-bit. -Xassembler  option. 
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN: -Xassembler -w \
+// RUN: -target powerpc-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS32-PTHREAD %s
+// CHECK-AS32-PTHREAD-NOT: warning:
+// CHECK-AS32-PTHREAD: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc-ibm-aix7.1.0.0"
+// CHECK-AS32-PTHREAD: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS32-PTHREAD: "-a32" 
+// CHECK-AS32-PTHREAD: "-u" 
+// CHECK-AS32-PTHREAD: "-many"
+// CHECK-AS32-PTHREAD: "-w"
+
+// Check powerpc-ibm-aix7.1.0.0, 64-bit. -Wa,, option.
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN: -Wa,-v,-w \
+// RUN: -target powerpc64-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS64-PTHREAD %s
+// CHECK-AS64-PTHREAD-NOT: warning:
+// CHECK-AS64-PTHREAD: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc64-ibm-aix7.1.0.0"
+// CHECK-AS64-PTHREAD: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS64-PTHREAD: "-a64" 
+// CHECK-AS64-PTHREAD: "-u" 
+// CHECK-AS64-PTHREAD: "-many"
+// CHECK-AS64-PTHREAD: "-v"
+// CHECK-AS64-PTHREAD: "-w"
Index: clang/lib/Driver/ToolChains/AIX.h
===
--- clang/lib/Driver/ToolChains/AIX.h
+++ clang/lib/Driver/ToolChains/AIX.h
@@ -16,10 +16,21 @@
 namespace driver {
 namespace tools {
 
-/// aix -- Directly call system default linker.
-// TODO: Enable direct call to system default assembler.
+/// aix -- Directly call system default assembler and linker.
 namespace aix {
 
+class LLVM_LIBRARY_VISIBILITY Assembler : public Tool {
+public:
+  Assembler(const ToolChain ) : Tool("aix::Assembler", "assembler", TC) {}
+
+  bool hasIntegratedCPP() const override { return false; }
+
+  void ConstructJob(Compilation , const JobAction ,
+const InputInfo , const InputInfoList ,
+const llvm::opt::ArgList ,
+const char *LinkingOutput) const override;
+};
+
 class LLVM_LIBRARY_VISIBILITY Linker : public Tool {
 public:
   Linker(const ToolChain ) : Tool("aix::Linker", "linker", TC) {}
@@ -53,6 +64,7 @@
   bool isPICDefaultForced() const override { return true; }
 
 protected:
+  Tool *buildAssembler() const override;
   Tool *buildLinker() const override;
 };
 
Index: clang/lib/Driver/ToolChains/AIX.cpp
===
--- clang/lib/Driver/ToolChains/AIX.cpp
+++ clang/lib/Driver/ToolChains/AIX.cpp
@@ -20,6 +20,52 @@
 
 using namespace llvm::opt;
 
+void aix::Assembler::ConstructJob(Compilation , const JobAction ,
+  const InputInfo ,
+  const InputInfoList ,
+  const ArgList ,
+  const char *LinkingOutput) const {
+  claimNoWarnArgs(Args);
+  ArgStringList CmdArgs;
+
+  const bool IsArch32Bit = getToolChain().getTriple().isArch32Bit();
+  const bool IsArch64Bit = getToolChain().getTriple().isArch64Bit();
+  // Only support 32 and 64 bit.
+  if (!IsArch32Bit && !IsArch64Bit)
+llvm_unreachable("Unsupported bit width value.");
+
+  // Specify the mode in which the as command operates.
+  if (IsArch32Bit) {
+CmdArgs.push_back("-a32");
+  } else {
+// Must be 64-bit, otherwise asserted already.
+CmdArgs.push_back("-a64");
+ 

[PATCH] D69620: Add AIX assembler support

[Steven Wan via Phabricator via cfe-commits]

stevewan marked 4 inline comments as done.
stevewan added inline comments.



Comment at: clang/lib/Driver/ToolChains/AIX.cpp:28
+  const char *LinkingOutput) const {
+  claimNoWarnArgs(Args);
+  ArgStringList CmdArgs;

Xiangling_L wrote:
> The definition of `claimNoWarnArgs` is to suppress warnings for some options 
> if they are unused, can you explain a little bit about how did you figure out 
> that we don't want warnings for those?
> 
> Some context of `claimNoWarnArgs`:
> 
> ```
> // Claim options we don't want to warn if they are unused. We do this for
> // options that build systems might add but are unused when assembling or only
> // running the preprocessor for example.
> void tools::claimNoWarnArgs(const ArgList ) {
>   // Don't warn about unused -f(no-)?lto.  This can happen when we're
>   // preprocessing, precompiling or assembling.
>   Args.ClaimAllArgs(options::OPT_flto_EQ);
>   Args.ClaimAllArgs(options::OPT_flto);
>   Args.ClaimAllArgs(options::OPT_fno_lto);
> }
> ```
> 
The original reason was that, since we are doing assembling here, and as stated 
in `claimNoWarnArgs`, there might be an unused `-f(no-)?lto` when we're 
assembling, and we'd like to suppress the warning(s) for that. 

Looking into it, the three options [[ 
https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-flto 
| `flto`, `fno_lto` ]] , and [[ 
https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang1-flto
 | `flto_EQ` ]] are used to enable/disable link time optimization, but LTO is 
not supported by the AIX system linker. Driver would just throw error if the 
user passes it `-flto` or `-flto=` on AIX, so claiming them or not 
currently makes no actual difference as far as I'm concerned. 

That being said, I don't have a strong opinion either way. Let's see how other 
people think.



Comment at: clang/lib/Driver/ToolChains/AIX.h:26
+
+  bool hasIntegratedCPP() const override { return false; }
+

daltenty wrote:
> stevewan wrote:
> > Xiangling_L wrote:
> > > I saw a lot of other target also set `hasIntegratedCPP()` as false, but I 
> > > didn't find any explanation in documentation, so I am curious that what 
> > > this is about?
> > I also failed to find useful resources that explains the purpose of this 
> > function. The main rationales of adding it were essentially that,
> > 1. It's a pure virtual function in the base `Tool` class,
> > 2. Most if not all of other targets had overridden this function such that 
> > it returns false.
> > 
> > I'll leave this comment open, and see if someone could enlighten us. 
> Only "Compiler" tools set hasIntegratedCPP() to true. Looking into it, it 
> seems combineWithPreprocessor() uses this to decide whether the tool supports 
> preprocessor actions so it may fold them in to one step.  I think we are safe 
> leaving this as is.
I checked this function on other targets. As David pointed out, only the 
compiler tools would set hasIntegratedCPP() to true, the assembler and linker 
tools set it to false because they do not support combining with preprocessor 
action.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69620/new/

https://reviews.llvm.org/D69620



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69620: Add AIX assembler support

[Steven Wan via Phabricator via cfe-commits]

stevewan marked an inline comment as done.
stevewan added inline comments.



Comment at: clang/lib/Driver/ToolChains/AIX.cpp:45
+  // Acccept any mixture of instructions.
+  CmdArgs.push_back("-many");
+

Xiangling_L wrote:
> GCC invokes system assembler also with options `-mpwr4` and `-u`, I think you 
> need to verify that do we need those? And as far as I can recall, `-mpwr4` is 
> to pick up new version AIX instruction set, and `-u` is to suppress warning 
> for undefined symbols. 90% sure that we need `-mpwr4`(I could be wrong), but 
> not sure about `-u`.
For the `-u` option, I'll be adding it to this patch with a FIXME that we'd 
like to remove it in the future. The rationale is that, since currently we do 
not have the assembly writing ready that writes the extern(s), we'll pass in 
this `-u` to suppress any related warnings. Once the assembly writing is ready, 
we should remove this flag as it potentially covers up for undefined symbols 
that are actually problematic.

For the `-m` option, adding `-many` seemed to be more suitable at the moment. 
During code generation, the compiler should've already guaranteed that it used 
a correct and appropriate instruction set (i.e., conforms to the user specified 
driver option `-mcpu=`). The `-m` assembler option here double checks the same 
thing that's already been checked at codegen. That said, we can just pass in 
`-many` to accept all and rely on codegen to do the checking. On the other 
hand, potential problems might get away with it in one of the two scenarios I 
can think of right now,


  # User passes in a `.s` assembly file that uses instructions from the 
super-set of what's specified in `-mcpu=`. This is mostly on the user side 
because essentially they pass in contradictory inputs that are not going to fly.

  # User passes in a `.c` source file, but codegen hit some issue and falsely 
decides to use a super-set of what's specified in `-mcpu=`. Given that we don't 
have codegen ready yet, we don't know how reliable it might be. I would suggest 
that we keep `-many` as it is for now, and we may change it when needed once we 
are more clear on code generation. 


With regard to GCC's behaviour, GCC would append first `-mpwr4` then `-many` to 
the system assembler, which effectively means adding `-many` solely because the 
last `-m` flag would overwrite all its preceding one(s).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69620/new/

https://reviews.llvm.org/D69620



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69620: Add AIX assembler support

[David Tenty via Phabricator via cfe-commits]

daltenty added inline comments.



Comment at: clang/lib/Driver/ToolChains/AIX.h:26
+
+  bool hasIntegratedCPP() const override { return false; }
+

stevewan wrote:
> Xiangling_L wrote:
> > I saw a lot of other target also set `hasIntegratedCPP()` as false, but I 
> > didn't find any explanation in documentation, so I am curious that what 
> > this is about?
> I also failed to find useful resources that explains the purpose of this 
> function. The main rationales of adding it were essentially that,
> 1. It's a pure virtual function in the base `Tool` class,
> 2. Most if not all of other targets had overridden this function such that it 
> returns false.
> 
> I'll leave this comment open, and see if someone could enlighten us. 
Only "Compiler" tools set hasIntegratedCPP() to true. Looking into it, it seems 
combineWithPreprocessor() uses this to decide whether the tool supports 
preprocessor actions so it may fold them in to one step.  I think we are safe 
leaving this as is.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69620/new/

https://reviews.llvm.org/D69620



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69620: Add AIX assembler support

[Steven Wan via Phabricator via cfe-commits]

stevewan marked an inline comment as done.
stevewan added inline comments.



Comment at: clang/lib/Driver/ToolChains/AIX.h:26
+
+  bool hasIntegratedCPP() const override { return false; }
+

Xiangling_L wrote:
> I saw a lot of other target also set `hasIntegratedCPP()` as false, but I 
> didn't find any explanation in documentation, so I am curious that what this 
> is about?
I also failed to find useful resources that explains the purpose of this 
function. The main rationales of adding it were essentially that,
1. It's a pure virtual function in the base `Tool` class,
2. Most if not all of other targets had overridden this function such that it 
returns false.

I'll leave this comment open, and see if someone could enlighten us. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69620/new/

https://reviews.llvm.org/D69620



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69620: Add AIX assembler support

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L added inline comments.



Comment at: clang/lib/Driver/ToolChains/AIX.cpp:28
+  const char *LinkingOutput) const {
+  claimNoWarnArgs(Args);
+  ArgStringList CmdArgs;

The definition of `claimNoWarnArgs` is to suppress warnings for some options if 
they are unused, can you explain a little bit about how did you figure out that 
we don't want warnings for those?

Some context of `claimNoWarnArgs`:

```
// Claim options we don't want to warn if they are unused. We do this for
// options that build systems might add but are unused when assembling or only
// running the preprocessor for example.
void tools::claimNoWarnArgs(const ArgList ) {
  // Don't warn about unused -f(no-)?lto.  This can happen when we're
  // preprocessing, precompiling or assembling.
  Args.ClaimAllArgs(options::OPT_flto_EQ);
  Args.ClaimAllArgs(options::OPT_flto);
  Args.ClaimAllArgs(options::OPT_fno_lto);
}
```




Comment at: clang/lib/Driver/ToolChains/AIX.cpp:45
+  // Acccept any mixture of instructions.
+  CmdArgs.push_back("-many");
+

GCC invokes system assembler also with options `-mpwr4` and `-u`, I think you 
need to verify that do we need those? And as far as I can recall, `-mpwr4` is 
to pick up new version AIX instruction set, and `-u` is to suppress warning for 
undefined symbols. 90% sure that we need `-mpwr4`(I could be wrong), but not 
sure about `-u`.



Comment at: clang/lib/Driver/ToolChains/AIX.h:26
+
+  bool hasIntegratedCPP() const override { return false; }
+

I saw a lot of other target also set `hasIntegratedCPP()` as false, but I 
didn't find any explanation in documentation, so I am curious that what this is 
about?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69620/new/

https://reviews.llvm.org/D69620



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69620: Add AIX assembler support

[Steven Wan via Phabricator via cfe-commits]

stevewan created this revision.
Herald added subscribers: cfe-commits, jfb.
Herald added a project: clang.

This is a follow on patch to D68340 . Given 
the AIX toolchain skeleton and system linker support introduced in D68340 
, this patch adds suuport to system assembler 
invocation on AIX.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69620

Files:
  clang/lib/Driver/ToolChains/AIX.cpp
  clang/lib/Driver/ToolChains/AIX.h
  clang/test/Driver/aix-as.c

Index: clang/test/Driver/aix-as.c
===
--- /dev/null
+++ clang/test/Driver/aix-as.c
@@ -0,0 +1,48 @@
+// General tests that as invocations on AIX targets are sane. Note that we
+// only test assembler functionalities in this suite.
+
+// Check powerpc-ibm-aix7.1.0.0, 32-bit.
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN: -target powerpc-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS32 %s
+// CHECK-AS32-NOT: warning:
+// CHECK-AS32: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc-ibm-aix7.1.0.0"
+// CHECK-AS32: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS32: "-a32" 
+// CHECK-AS32: "-many" 
+
+// Check powerpc64-ibm-aix7.1.0.0, 64-bit.
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN: -target powerpc64-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS64 %s
+// CHECK-AS64-NOT: warning:
+// CHECK-AS64: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc64-ibm-aix7.1.0.0"
+// CHECK-AS64: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS64: "-a64" 
+// CHECK-AS64: "-many"
+
+
+// Check powerpc-ibm-aix7.1.0.0, 32-bit. -Xassembler  option. 
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN: -Xassembler -w \
+// RUN: -target powerpc-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS32-PTHREAD %s
+// CHECK-AS32-PTHREAD-NOT: warning:
+// CHECK-AS32-PTHREAD: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc-ibm-aix7.1.0.0"
+// CHECK-AS32-PTHREAD: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS32-PTHREAD: "-a32" 
+// CHECK-AS32-PTHREAD: "-many"
+// CHECK-AS32-PTHREAD: "-w"
+
+// Check powerpc-ibm-aix7.1.0.0, 64-bit. -Wa,, option.
+// RUN: %clang -no-canonical-prefixes %s -### -c -o %t.o 2>&1 \
+// RUN: -Wa,-v,-w \
+// RUN: -target powerpc64-ibm-aix7.1.0.0 \
+// RUN:   | FileCheck --check-prefix=CHECK-AS64-PTHREAD %s
+// CHECK-AS64-PTHREAD-NOT: warning:
+// CHECK-AS64-PTHREAD: {{.*}}clang{{(.exe)?}}" "-cc1" "-triple" "powerpc64-ibm-aix7.1.0.0"
+// CHECK-AS64-PTHREAD: "{{.*}}as{{(.exe)?}}" 
+// CHECK-AS64-PTHREAD: "-a64" 
+// CHECK-AS64-PTHREAD: "-many"
+// CHECK-AS64-PTHREAD: "-v"
+// CHECK-AS64-PTHREAD: "-w"
Index: clang/lib/Driver/ToolChains/AIX.h
===
--- clang/lib/Driver/ToolChains/AIX.h
+++ clang/lib/Driver/ToolChains/AIX.h
@@ -16,10 +16,21 @@
 namespace driver {
 namespace tools {
 
-/// aix -- Directly call system default linker.
-// TODO: Enable direct call to system default assembler.
+/// aix -- Directly call system default assembler and linker.
 namespace aix {
 
+class LLVM_LIBRARY_VISIBILITY Assembler : public Tool {
+public:
+  Assembler(const ToolChain ) : Tool("aix::Assembler", "assembler", TC) {}
+
+  bool hasIntegratedCPP() const override { return false; }
+
+  void ConstructJob(Compilation , const JobAction ,
+const InputInfo , const InputInfoList ,
+const llvm::opt::ArgList ,
+const char *LinkingOutput) const override;
+};
+
 class LLVM_LIBRARY_VISIBILITY Linker : public Tool {
 public:
   Linker(const ToolChain ) : Tool("aix::Linker", "linker", TC) {}
@@ -53,6 +64,7 @@
   bool isPICDefaultForced() const override { return true; }
 
 protected:
+  Tool *buildAssembler() const override;
   Tool *buildLinker() const override;
 };
 
Index: clang/lib/Driver/ToolChains/AIX.cpp
===
--- clang/lib/Driver/ToolChains/AIX.cpp
+++ clang/lib/Driver/ToolChains/AIX.cpp
@@ -20,6 +20,47 @@
 
 using namespace llvm::opt;
 
+void aix::Assembler::ConstructJob(Compilation , const JobAction ,
+  const InputInfo ,
+  const InputInfoList ,
+  const ArgList ,
+  const char *LinkingOutput) const {
+  claimNoWarnArgs(Args);
+  ArgStringList CmdArgs;
+
+  const bool IsArch32Bit = getToolChain().getTriple().isArch32Bit();
+  const bool IsArch64Bit = getToolChain().getTriple().isArch64Bit();
+  // Only support 32 and 64 bit.
+  if (!IsArch32Bit && !IsArch64Bit)
+llvm_unreachable("Unsupported bit width value.");
+
+  // Specify the mode in which the as command operates.
+  if (IsArch32Bit) {
+CmdArgs.push_back("-a32");
+  } else {
+// Must be 64-bit, otherwise asserted already.
+