[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as` on AIX

[Qiongsi Wu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG41447f6fdfe4: [libLTO][AIX] Respect `-f[no]-integrated-as` 
on AIX (authored by qiongsiwu1).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152924

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/test/Driver/lto-aix.c
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/lib/LTO/LTOCodeGenerator.cpp
  llvm/test/tools/llvm-lto/aix.ll
  llvm/tools/llc/llc.cpp

Index: llvm/tools/llc/llc.cpp
===
--- llvm/tools/llc/llc.cpp
+++ llvm/tools/llc/llc.cpp
@@ -106,10 +106,6 @@
  "'none' means that all ELF features can be used, "
  "regardless of binutils support"));
 
-static cl::opt
-NoIntegratedAssembler("no-integrated-as", cl::Hidden,
-  cl::desc("Disable integrated assembler"));
-
 static cl::opt
 PreserveComments("preserve-as-comments", cl::Hidden,
  cl::desc("Preserve Comments in outputted assembly"),
@@ -517,7 +513,6 @@
 
 Options.BinutilsVersion =
 TargetMachine::parseBinutilsVersion(BinutilsVersion);
-Options.DisableIntegratedAS = NoIntegratedAssembler;
 Options.MCOptions.ShowMCEncoding = ShowMCEncoding;
 Options.MCOptions.AsmVerbose = AsmVerbose;
 Options.MCOptions.PreserveAsmComments = PreserveComments;
Index: llvm/test/tools/llvm-lto/aix.ll
===
--- llvm/test/tools/llvm-lto/aix.ll
+++ llvm/test/tools/llvm-lto/aix.ll
@@ -1,7 +1,11 @@
-; REQUIRES: system-aix
+; REQUIRES: powerpc-registered-target
+; RUN: rm -rf %t && mkdir %t && cd %t
 ; RUN: llvm-as < %s > %t1
 ; RUN: llvm-lto %t1 | FileCheck %s
 
+; Test system assembler.
+; RUN: %if system-aix %{ llvm-lto -no-integrated-as=1 %t1 | FileCheck %s %}
+
 target triple = "powerpc-ibm-aix"
 
 define i32 @main() {
@@ -9,4 +13,3 @@
   ret i32 42
 }
 ; CHECK: Wrote native object file
-
Index: llvm/lib/LTO/LTOCodeGenerator.cpp
===
--- llvm/lib/LTO/LTOCodeGenerator.cpp
+++ llvm/lib/LTO/LTOCodeGenerator.cpp
@@ -244,7 +244,7 @@
 
 bool LTOCodeGenerator::useAIXSystemAssembler() {
   const auto  = TargetMach->getTargetTriple();
-  return Triple.isOSAIX();
+  return Triple.isOSAIX() && Config.Options.DisableIntegratedAS;
 }
 
 bool LTOCodeGenerator::runAIXSystemAssembler(SmallString<128> ) {
Index: llvm/lib/CodeGen/CommandFlags.cpp
===
--- llvm/lib/CodeGen/CommandFlags.cpp
+++ llvm/lib/CodeGen/CommandFlags.cpp
@@ -81,6 +81,7 @@
 CGOPT(bool, StackRealign)
 CGOPT(std::string, TrapFuncName)
 CGOPT(bool, UseCtors)
+CGOPT(bool, DisableIntegratedAS)
 CGOPT(bool, RelaxELFRelocations)
 CGOPT_EXP(bool, DataSections)
 CGOPT_EXP(bool, FunctionSections)
@@ -487,6 +488,11 @@
   cl::init(false));
   CGBINDOPT(XCOFFReadOnlyPointers);
 
+  static cl::opt DisableIntegratedAS(
+  "no-integrated-as", cl::desc("Disable integrated assembler"),
+  cl::init(false));
+  CGBINDOPT(DisableIntegratedAS);
+
 #undef CGBINDOPT
 
   mc::RegisterMCTargetOptionsFlags();
@@ -540,6 +546,7 @@
   Options.GuaranteedTailCallOpt = getEnableGuaranteedTailCallOpt();
   Options.StackSymbolOrdering = getStackSymbolOrdering();
   Options.UseInitArray = !getUseCtors();
+  Options.DisableIntegratedAS = getDisableIntegratedAS();
   Options.RelaxELFRelocations = getRelaxELFRelocations();
   Options.DataSections =
   getExplicitDataSections().value_or(TheTriple.hasDefaultDataSections());
Index: llvm/include/llvm/CodeGen/CommandFlags.h
===
--- llvm/include/llvm/CodeGen/CommandFlags.h
+++ llvm/include/llvm/CodeGen/CommandFlags.h
@@ -94,6 +94,8 @@
 
 bool getUseCtors();
 
+bool getDisableIntegratedAS();
+
 bool getRelaxELFRelocations();
 
 bool getDataSections();
Index: clang/test/Driver/lto-aix.c
===
--- clang/test/Driver/lto-aix.c
+++ clang/test/Driver/lto-aix.c
@@ -4,7 +4,7 @@
 //
 // LTOPATH: "-bplugin:{{.*}}libLTO.{{so|dll|dylib}}"
 // MCPUOPTLEVEL: "-bplugin_opt:-mcpu={{.*}}" "-bplugin_opt:-O3"
-//
+
 // More opt level option tests
 // RUN: %clang --target=powerpc-ibm-aix --sysroot %S/Inputs/aix_ppc_tree %s \
 // RUN:   -fuse-ld=ld -flto -O -### 2>&1 | FileCheck --check-prefix=O1 %s
@@ -26,7 +26,7 @@
 // O1: "-bplugin_opt:-O1"
 // O2: "-bplugin_opt:-O2"
 // O3: "-bplugin_opt:-O3"
-//
+
 // vec-extabi option
 // RUN: %clang --target=powerpc-ibm-aix --sysroot %S/Inputs/aix_ppc_tree %s \
 // RUN:   -fuse-ld=ld -flto -mabi=vec-extabi -### 2>&1 \
@@ -36,7 +36,7 @@
 //
 // VECEXTABI: "-bplugin_opt:-vec-extabi"
 // NOVECEXTABI-NOT: 

[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as` on AIX

[Qiongsi Wu via Phabricator via cfe-commits]

qiongsiwu1 updated this revision to Diff 539544.
qiongsiwu1 added a comment.

Address test case review comments. Thanks so much @MaskRay for the feedback!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152924

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/test/Driver/lto-aix.c
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/lib/LTO/LTOCodeGenerator.cpp
  llvm/test/tools/llvm-lto/aix.ll
  llvm/tools/llc/llc.cpp

Index: llvm/tools/llc/llc.cpp
===
--- llvm/tools/llc/llc.cpp
+++ llvm/tools/llc/llc.cpp
@@ -106,10 +106,6 @@
  "'none' means that all ELF features can be used, "
  "regardless of binutils support"));
 
-static cl::opt
-NoIntegratedAssembler("no-integrated-as", cl::Hidden,
-  cl::desc("Disable integrated assembler"));
-
 static cl::opt
 PreserveComments("preserve-as-comments", cl::Hidden,
  cl::desc("Preserve Comments in outputted assembly"),
@@ -517,7 +513,6 @@
 
 Options.BinutilsVersion =
 TargetMachine::parseBinutilsVersion(BinutilsVersion);
-Options.DisableIntegratedAS = NoIntegratedAssembler;
 Options.MCOptions.ShowMCEncoding = ShowMCEncoding;
 Options.MCOptions.AsmVerbose = AsmVerbose;
 Options.MCOptions.PreserveAsmComments = PreserveComments;
Index: llvm/test/tools/llvm-lto/aix.ll
===
--- llvm/test/tools/llvm-lto/aix.ll
+++ llvm/test/tools/llvm-lto/aix.ll
@@ -1,7 +1,11 @@
-; REQUIRES: system-aix
+; REQUIRES: powerpc-registered-target
+; RUN: rm -rf %t && mkdir %t && cd %t
 ; RUN: llvm-as < %s > %t1
 ; RUN: llvm-lto %t1 | FileCheck %s
 
+; Test system assembler.
+; RUN: %if system-aix %{ llvm-lto -no-integrated-as=1 %t1 | FileCheck %s %}
+
 target triple = "powerpc-ibm-aix"
 
 define i32 @main() {
@@ -9,4 +13,3 @@
   ret i32 42
 }
 ; CHECK: Wrote native object file
-
Index: llvm/lib/LTO/LTOCodeGenerator.cpp
===
--- llvm/lib/LTO/LTOCodeGenerator.cpp
+++ llvm/lib/LTO/LTOCodeGenerator.cpp
@@ -244,7 +244,7 @@
 
 bool LTOCodeGenerator::useAIXSystemAssembler() {
   const auto  = TargetMach->getTargetTriple();
-  return Triple.isOSAIX();
+  return Triple.isOSAIX() && Config.Options.DisableIntegratedAS;
 }
 
 bool LTOCodeGenerator::runAIXSystemAssembler(SmallString<128> ) {
Index: llvm/lib/CodeGen/CommandFlags.cpp
===
--- llvm/lib/CodeGen/CommandFlags.cpp
+++ llvm/lib/CodeGen/CommandFlags.cpp
@@ -81,6 +81,7 @@
 CGOPT(bool, StackRealign)
 CGOPT(std::string, TrapFuncName)
 CGOPT(bool, UseCtors)
+CGOPT(bool, DisableIntegratedAS)
 CGOPT(bool, RelaxELFRelocations)
 CGOPT_EXP(bool, DataSections)
 CGOPT_EXP(bool, FunctionSections)
@@ -487,6 +488,11 @@
   cl::init(false));
   CGBINDOPT(XCOFFReadOnlyPointers);
 
+  static cl::opt DisableIntegratedAS(
+  "no-integrated-as", cl::desc("Disable integrated assembler"),
+  cl::init(false));
+  CGBINDOPT(DisableIntegratedAS);
+
 #undef CGBINDOPT
 
   mc::RegisterMCTargetOptionsFlags();
@@ -540,6 +546,7 @@
   Options.GuaranteedTailCallOpt = getEnableGuaranteedTailCallOpt();
   Options.StackSymbolOrdering = getStackSymbolOrdering();
   Options.UseInitArray = !getUseCtors();
+  Options.DisableIntegratedAS = getDisableIntegratedAS();
   Options.RelaxELFRelocations = getRelaxELFRelocations();
   Options.DataSections =
   getExplicitDataSections().value_or(TheTriple.hasDefaultDataSections());
Index: llvm/include/llvm/CodeGen/CommandFlags.h
===
--- llvm/include/llvm/CodeGen/CommandFlags.h
+++ llvm/include/llvm/CodeGen/CommandFlags.h
@@ -94,6 +94,8 @@
 
 bool getUseCtors();
 
+bool getDisableIntegratedAS();
+
 bool getRelaxELFRelocations();
 
 bool getDataSections();
Index: clang/test/Driver/lto-aix.c
===
--- clang/test/Driver/lto-aix.c
+++ clang/test/Driver/lto-aix.c
@@ -4,7 +4,7 @@
 //
 // LTOPATH: "-bplugin:{{.*}}libLTO.{{so|dll|dylib}}"
 // MCPUOPTLEVEL: "-bplugin_opt:-mcpu={{.*}}" "-bplugin_opt:-O3"
-//
+
 // More opt level option tests
 // RUN: %clang --target=powerpc-ibm-aix --sysroot %S/Inputs/aix_ppc_tree %s \
 // RUN:   -fuse-ld=ld -flto -O -### 2>&1 | FileCheck --check-prefix=O1 %s
@@ -26,7 +26,7 @@
 // O1: "-bplugin_opt:-O1"
 // O2: "-bplugin_opt:-O2"
 // O3: "-bplugin_opt:-O3"
-//
+
 // vec-extabi option
 // RUN: %clang --target=powerpc-ibm-aix --sysroot %S/Inputs/aix_ppc_tree %s \
 // RUN:   -fuse-ld=ld -flto -mabi=vec-extabi -### 2>&1 \
@@ -36,7 +36,7 @@
 //
 // VECEXTABI: "-bplugin_opt:-vec-extabi"
 // NOVECEXTABI-NOT: "-bplugin_opt:-vec-extabi"
-//
+
 // Test debugging options
 // RUN: %clang 

[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as` on AIX

[Fangrui Song via Phabricator via cfe-commits]

MaskRay accepted this revision.
MaskRay added a comment.

Some nits about testing, otherwise LG

In D152924#4490950 , @qiongsiwu1 
wrote:

> In D152924#4490581 , @MaskRay wrote:
>
>> We have `TargetOptions::DisableIntegratedAS` (your llc.cpp change). Do you 
>> know why it is not feasible for the places you want to check?
>
> I am checking/using `TargetOptions::DisableIntegratedAS` in 
> `LTOCodeGenerator::useAIXSystemAssembler()` to determine if the system 
> assembler should be used. `NoIntegratedAssembler` is moved from an `llc` only 
> option to a codegen option so LTO can use it as well.

Sorry, my question was not clear. Anyway, I figured it out.
We need to pass `lto::Config` to `lto::backend` with the correct `CGFileType` 
information.  `TargetOptions::DisableIntegratedAS` is already present, but we 
don't initialize it in `llvm/lib/CodeGen/CommandFlags.cpp`.
Thanks. This looks good to me.




Comment at: llvm/test/tools/llvm-lto/aix-sys-as.ll:1
+; REQUIRES: system-aix
+; RUN: llvm-as < %s > %t1

I suggest that you merge this test into aix.ll.

You can use

```
RUN: %if system-aix %{ ... %}
```
to only run a RUN line when the lit feature `system-aix` is available.



Comment at: llvm/test/tools/llvm-lto/aix.ll:3
 ; RUN: llvm-as < %s > %t1
 ; RUN: llvm-lto %t1 | FileCheck %s
 

It seems that with or without `-o`, the behavior may be different?

In some lit configurations (e.g., Google's internal lit runner), the current 
working directory if a test invocation is read-only. We need to ensure that the 
directory is writable.

You can do
```
RUN: rm -rf %t && mkdir %t && cd %t`
RUN: llvm-as < %s -o a.bc
RUN: llvm-lto a.bc
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152924

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

[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as` on AIX

[Qiongsi Wu via Phabricator via cfe-commits]

qiongsiwu1 added a comment.

In D152924#4490581 , @MaskRay wrote:

> We have `TargetOptions::DisableIntegratedAS` (your llc.cpp change). Do you 
> know why it is not feasible for the places you want to check?

I am checking/using `TargetOptions::DisableIntegratedAS` in 
`LTOCodeGenerator::useAIXSystemAssembler()` to determine if the system 
assembler should be used. `NoIntegratedAssembler` is moved from an `llc` only 
option to a codegen option so LTO can use it as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152924

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

[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as` on AIX

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

We have `TargetOptions::DisableIntegratedAS` (your llc.cpp change). Do you know 
why it is not feasible for the places you want to check?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152924

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

[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as` on AIX

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/test/Driver/lto-aix.c:76
 // CSPGO: "-bplugin_opt:-cs-profile-generate" 
"-bplugin_opt:-cs-profile-path=default_%m.profraw"
+//
+// Test integrated assembler options

This `^//$` line is not useful. Some tests use this style, but it's not a good 
style.

If we remove this line, we can actually use Vim `{` `}` navigate among tests.



Comment at: clang/test/Driver/lto-aix.c:79
+// RUN: %clang --target=powerpc-ibm-aix -### %s -flto -fno-integrated-as \
+// RUN:  -fintegrated-as 2>&1 | FileCheck --check-prefix=INTAS %s
+// RUN: %clang --target=powerpc-ibm-aix -### %s -flto -fintegrated-as \

2-space indentation



Comment at: llvm/test/tools/llvm-lto/aix-sys-as.ll:12
+; CHECK: Wrote native object file
+

delete trailing blank line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152924

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

[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as` on AIX

[Qiongsi Wu via Phabricator via cfe-commits]

qiongsiwu1 added a comment.

@MaskRay @w2yehia @shchenz could you let me know if there are further comments? 
I will land this patch if there are none. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152924

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

[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as` on AIX

[Qiongsi Wu via Phabricator via cfe-commits]

qiongsiwu1 updated this revision to Diff 532256.
qiongsiwu1 added a comment.

Clean up code in `addLTOOptions`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152924

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/test/Driver/lto-aix.c
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/lib/LTO/LTOCodeGenerator.cpp
  llvm/test/tools/llvm-lto/aix-sys-as.ll
  llvm/test/tools/llvm-lto/aix.ll
  llvm/tools/llc/llc.cpp

Index: llvm/tools/llc/llc.cpp
===
--- llvm/tools/llc/llc.cpp
+++ llvm/tools/llc/llc.cpp
@@ -106,10 +106,6 @@
  "'none' means that all ELF features can be used, "
  "regardless of binutils support"));
 
-static cl::opt
-NoIntegratedAssembler("no-integrated-as", cl::Hidden,
-  cl::desc("Disable integrated assembler"));
-
 static cl::opt
 PreserveComments("preserve-as-comments", cl::Hidden,
  cl::desc("Preserve Comments in outputted assembly"),
@@ -517,7 +513,6 @@
 
 Options.BinutilsVersion =
 TargetMachine::parseBinutilsVersion(BinutilsVersion);
-Options.DisableIntegratedAS = NoIntegratedAssembler;
 Options.MCOptions.ShowMCEncoding = ShowMCEncoding;
 Options.MCOptions.AsmVerbose = AsmVerbose;
 Options.MCOptions.PreserveAsmComments = PreserveComments;
Index: llvm/test/tools/llvm-lto/aix.ll
===
--- llvm/test/tools/llvm-lto/aix.ll
+++ llvm/test/tools/llvm-lto/aix.ll
@@ -1,4 +1,4 @@
-; REQUIRES: system-aix
+; REQUIRES: powerpc-registered-target
 ; RUN: llvm-as < %s > %t1
 ; RUN: llvm-lto %t1 | FileCheck %s
 
Index: llvm/test/tools/llvm-lto/aix-sys-as.ll
===
--- /dev/null
+++ llvm/test/tools/llvm-lto/aix-sys-as.ll
@@ -0,0 +1,12 @@
+; REQUIRES: system-aix
+; RUN: llvm-as < %s > %t1
+; RUN: llvm-lto -no-integrated-as=1 %t1 | FileCheck %s
+
+target triple = "powerpc-ibm-aix"
+
+define i32 @main() {
+entry:
+  ret i32 42
+}
+; CHECK: Wrote native object file
+
Index: llvm/lib/LTO/LTOCodeGenerator.cpp
===
--- llvm/lib/LTO/LTOCodeGenerator.cpp
+++ llvm/lib/LTO/LTOCodeGenerator.cpp
@@ -244,7 +244,7 @@
 
 bool LTOCodeGenerator::useAIXSystemAssembler() {
   const auto  = TargetMach->getTargetTriple();
-  return Triple.isOSAIX();
+  return Triple.isOSAIX() && Config.Options.DisableIntegratedAS;
 }
 
 bool LTOCodeGenerator::runAIXSystemAssembler(SmallString<128> ) {
Index: llvm/lib/CodeGen/CommandFlags.cpp
===
--- llvm/lib/CodeGen/CommandFlags.cpp
+++ llvm/lib/CodeGen/CommandFlags.cpp
@@ -81,6 +81,7 @@
 CGOPT(bool, StackRealign)
 CGOPT(std::string, TrapFuncName)
 CGOPT(bool, UseCtors)
+CGOPT(bool, DisableIntegratedAS)
 CGOPT(bool, RelaxELFRelocations)
 CGOPT_EXP(bool, DataSections)
 CGOPT_EXP(bool, FunctionSections)
@@ -487,6 +488,11 @@
   cl::init(false));
   CGBINDOPT(XCOFFReadOnlyPointers);
 
+  static cl::opt DisableIntegratedAS(
+  "no-integrated-as", cl::desc("Disable integrated assembler"),
+  cl::init(false));
+  CGBINDOPT(DisableIntegratedAS);
+
 #undef CGBINDOPT
 
   mc::RegisterMCTargetOptionsFlags();
@@ -540,6 +546,7 @@
   Options.GuaranteedTailCallOpt = getEnableGuaranteedTailCallOpt();
   Options.StackSymbolOrdering = getStackSymbolOrdering();
   Options.UseInitArray = !getUseCtors();
+  Options.DisableIntegratedAS = getDisableIntegratedAS();
   Options.RelaxELFRelocations = getRelaxELFRelocations();
   Options.DataSections =
   getExplicitDataSections().value_or(TheTriple.hasDefaultDataSections());
Index: llvm/include/llvm/CodeGen/CommandFlags.h
===
--- llvm/include/llvm/CodeGen/CommandFlags.h
+++ llvm/include/llvm/CodeGen/CommandFlags.h
@@ -94,6 +94,8 @@
 
 bool getUseCtors();
 
+bool getDisableIntegratedAS();
+
 bool getRelaxELFRelocations();
 
 bool getDataSections();
Index: clang/test/Driver/lto-aix.c
===
--- clang/test/Driver/lto-aix.c
+++ clang/test/Driver/lto-aix.c
@@ -73,3 +73,14 @@
 // RUN:   -fcs-profile-generate 2>&1 | FileCheck -check-prefix=CSPGO %s
 //
 // CSPGO: "-bplugin_opt:-cs-profile-generate" "-bplugin_opt:-cs-profile-path=default_%m.profraw"
+//
+// Test integrated assembler options
+// RUN: %clang --target=powerpc-ibm-aix -### %s -flto -fno-integrated-as \
+// RUN:  -fintegrated-as 2>&1 | FileCheck --check-prefix=INTAS %s
+// RUN: %clang --target=powerpc-ibm-aix -### %s -flto -fintegrated-as \
+// RUN:  -fno-integrated-as 2>&1 | FileCheck --check-prefix=NOINTAS %s
+// RUN: %clang --target=powerpc-ibm-aix -### %s -flto 2>&1 \
+// RUN:   | FileCheck 

[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as` on AIX

[Qiongsi Wu via Phabricator via cfe-commits]

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



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:703
+Arg *A = Args.getLastArgNoClaim(options::OPT_fno_integrated_as);
+D.Diag(diag::warn_drv_unsupported_lto_option_for_target)
+<< A->getSpelling() << ToolChain.getTriple().getTriple();

MaskRay wrote:
> qiongsiwu1 wrote:
> > MaskRay wrote:
> > > The patch is titled AIX but this would change the Linux behavior. In my 
> > > experience this diagnostic is not useful.
> > Yes I agree the title is a bit misleading. The warning is added because the 
> > behaviour of `-fno-integrated-as` changes with and without `-flto`. Without 
> > `-flto`, `clang` does create the job to invoke system assembler, and with 
> > `-flto`, currently `clang` silently ignores the `-fno-integrated-as` option 
> > (without this patch). I felt that was unexpected, hence I added the 
> > warning. I am happy to remove the warning if the community thinks that 
> > silently ignoring the option is acceptable. 
> > 
> > The limitation of this diagnostic is that it goes through `clang` and there 
> > is nothing in the backend guarding against using `-no-integrated-as` 
> > directly through `-plugin-opt` on Linux, which will be ignored. @MaskRay is 
> > this something we should worry about? If not, I will not try looking for a 
> > place to add the warning in the backend.
> > 
> > Thanks! 
> I do not worry about LTO ignoring `-fno-integrated-as` on Linux/*BSD targets. 
> The use cases are very limited and the primary user (Linux kernel community) 
> knows this.
> 
> Actually the link action ignores compilation-affected options anyway (all 
> `CompileOnly_option`).
Thanks! The diagnostic is removed. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152924

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

[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as` on AIX

[Qiongsi Wu via Phabricator via cfe-commits]

qiongsiwu1 updated this revision to Diff 532241.
qiongsiwu1 edited the summary of this revision.
qiongsiwu1 added a comment.

Address code review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152924

Files:
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/test/Driver/lto-aix.c
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/lib/LTO/LTOCodeGenerator.cpp
  llvm/test/tools/llvm-lto/aix-sys-as.ll
  llvm/test/tools/llvm-lto/aix.ll
  llvm/tools/llc/llc.cpp

Index: llvm/tools/llc/llc.cpp
===
--- llvm/tools/llc/llc.cpp
+++ llvm/tools/llc/llc.cpp
@@ -106,10 +106,6 @@
  "'none' means that all ELF features can be used, "
  "regardless of binutils support"));
 
-static cl::opt
-NoIntegratedAssembler("no-integrated-as", cl::Hidden,
-  cl::desc("Disable integrated assembler"));
-
 static cl::opt
 PreserveComments("preserve-as-comments", cl::Hidden,
  cl::desc("Preserve Comments in outputted assembly"),
@@ -517,7 +513,6 @@
 
 Options.BinutilsVersion =
 TargetMachine::parseBinutilsVersion(BinutilsVersion);
-Options.DisableIntegratedAS = NoIntegratedAssembler;
 Options.MCOptions.ShowMCEncoding = ShowMCEncoding;
 Options.MCOptions.AsmVerbose = AsmVerbose;
 Options.MCOptions.PreserveAsmComments = PreserveComments;
Index: llvm/test/tools/llvm-lto/aix.ll
===
--- llvm/test/tools/llvm-lto/aix.ll
+++ llvm/test/tools/llvm-lto/aix.ll
@@ -1,4 +1,4 @@
-; REQUIRES: system-aix
+; REQUIRES: powerpc-registered-target
 ; RUN: llvm-as < %s > %t1
 ; RUN: llvm-lto %t1 | FileCheck %s
 
Index: llvm/test/tools/llvm-lto/aix-sys-as.ll
===
--- /dev/null
+++ llvm/test/tools/llvm-lto/aix-sys-as.ll
@@ -0,0 +1,12 @@
+; REQUIRES: system-aix
+; RUN: llvm-as < %s > %t1
+; RUN: llvm-lto -no-integrated-as=1 %t1 | FileCheck %s
+
+target triple = "powerpc-ibm-aix"
+
+define i32 @main() {
+entry:
+  ret i32 42
+}
+; CHECK: Wrote native object file
+
Index: llvm/lib/LTO/LTOCodeGenerator.cpp
===
--- llvm/lib/LTO/LTOCodeGenerator.cpp
+++ llvm/lib/LTO/LTOCodeGenerator.cpp
@@ -244,7 +244,7 @@
 
 bool LTOCodeGenerator::useAIXSystemAssembler() {
   const auto  = TargetMach->getTargetTriple();
-  return Triple.isOSAIX();
+  return Triple.isOSAIX() && Config.Options.DisableIntegratedAS;
 }
 
 bool LTOCodeGenerator::runAIXSystemAssembler(SmallString<128> ) {
Index: llvm/lib/CodeGen/CommandFlags.cpp
===
--- llvm/lib/CodeGen/CommandFlags.cpp
+++ llvm/lib/CodeGen/CommandFlags.cpp
@@ -81,6 +81,7 @@
 CGOPT(bool, StackRealign)
 CGOPT(std::string, TrapFuncName)
 CGOPT(bool, UseCtors)
+CGOPT(bool, DisableIntegratedAS)
 CGOPT(bool, RelaxELFRelocations)
 CGOPT_EXP(bool, DataSections)
 CGOPT_EXP(bool, FunctionSections)
@@ -487,6 +488,11 @@
   cl::init(false));
   CGBINDOPT(XCOFFReadOnlyPointers);
 
+  static cl::opt DisableIntegratedAS(
+  "no-integrated-as", cl::desc("Disable integrated assembler"),
+  cl::init(false));
+  CGBINDOPT(DisableIntegratedAS);
+
 #undef CGBINDOPT
 
   mc::RegisterMCTargetOptionsFlags();
@@ -540,6 +546,7 @@
   Options.GuaranteedTailCallOpt = getEnableGuaranteedTailCallOpt();
   Options.StackSymbolOrdering = getStackSymbolOrdering();
   Options.UseInitArray = !getUseCtors();
+  Options.DisableIntegratedAS = getDisableIntegratedAS();
   Options.RelaxELFRelocations = getRelaxELFRelocations();
   Options.DataSections =
   getExplicitDataSections().value_or(TheTriple.hasDefaultDataSections());
Index: llvm/include/llvm/CodeGen/CommandFlags.h
===
--- llvm/include/llvm/CodeGen/CommandFlags.h
+++ llvm/include/llvm/CodeGen/CommandFlags.h
@@ -94,6 +94,8 @@
 
 bool getUseCtors();
 
+bool getDisableIntegratedAS();
+
 bool getRelaxELFRelocations();
 
 bool getDataSections();
Index: clang/test/Driver/lto-aix.c
===
--- clang/test/Driver/lto-aix.c
+++ clang/test/Driver/lto-aix.c
@@ -73,3 +73,14 @@
 // RUN:   -fcs-profile-generate 2>&1 | FileCheck -check-prefix=CSPGO %s
 //
 // CSPGO: "-bplugin_opt:-cs-profile-generate" "-bplugin_opt:-cs-profile-path=default_%m.profraw"
+//
+// Test integrated assembler options
+// RUN: %clang --target=powerpc-ibm-aix -### %s -flto -fno-integrated-as \
+// RUN:  -fintegrated-as 2>&1 | FileCheck --check-prefix=INTAS %s
+// RUN: %clang --target=powerpc-ibm-aix -### %s -flto -fintegrated-as \
+// RUN:  -fno-integrated-as 2>&1 | FileCheck --check-prefix=NOINTAS %s
+// RUN: %clang --target=powerpc-ibm-aix -### %s -flto 2>&1 \
+// 

[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as` on AIX

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:703
+Arg *A = Args.getLastArgNoClaim(options::OPT_fno_integrated_as);
+D.Diag(diag::warn_drv_unsupported_lto_option_for_target)
+<< A->getSpelling() << ToolChain.getTriple().getTriple();

qiongsiwu1 wrote:
> MaskRay wrote:
> > The patch is titled AIX but this would change the Linux behavior. In my 
> > experience this diagnostic is not useful.
> Yes I agree the title is a bit misleading. The warning is added because the 
> behaviour of `-fno-integrated-as` changes with and without `-flto`. Without 
> `-flto`, `clang` does create the job to invoke system assembler, and with 
> `-flto`, currently `clang` silently ignores the `-fno-integrated-as` option 
> (without this patch). I felt that was unexpected, hence I added the warning. 
> I am happy to remove the warning if the community thinks that silently 
> ignoring the option is acceptable. 
> 
> The limitation of this diagnostic is that it goes through `clang` and there 
> is nothing in the backend guarding against using `-no-integrated-as` directly 
> through `-plugin-opt` on Linux, which will be ignored. @MaskRay is this 
> something we should worry about? If not, I will not try looking for a place 
> to add the warning in the backend.
> 
> Thanks! 
I do not worry about LTO ignoring `-fno-integrated-as` on Linux/*BSD targets. 
The use cases are very limited and the primary user (Linux kernel community) 
knows this.

Actually the link action ignores compilation-affected options anyway (all 
`CompileOnly_option`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152924

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

[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as` on AIX

[Qiongsi Wu via Phabricator via cfe-commits]

qiongsiwu1 added inline comments.



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:703
+Arg *A = Args.getLastArgNoClaim(options::OPT_fno_integrated_as);
+D.Diag(diag::warn_drv_unsupported_lto_option_for_target)
+<< A->getSpelling() << ToolChain.getTriple().getTriple();

MaskRay wrote:
> The patch is titled AIX but this would change the Linux behavior. In my 
> experience this diagnostic is not useful.
Yes I agree the title is a bit misleading. The warning is added because the 
behaviour of `-fno-integrated-as` changes with and without `-flto`. Without 
`-flto`, `clang` does create the job to invoke system assembler, and with 
`-flto`, currently `clang` silently ignores the `-fno-integrated-as` option 
(without this patch). I felt that was unexpected, hence I added the warning. I 
am happy to remove the warning if the community thinks that silently ignoring 
the option is acceptable. 

The limitation of this diagnostic is that it goes through `clang` and there is 
nothing in the backend guarding against using `-no-integrated-as` directly 
through `-plugin-opt` on Linux, which will be ignored. @MaskRay is this 
something we should worry about? If not, I will not try looking for a place to 
add the warning in the backend.

Thanks! 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152924

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

[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as` on AIX

[Qiongsi Wu via Phabricator via cfe-commits]

qiongsiwu1 marked an inline comment as done.
qiongsiwu1 added a comment.

In D152924#4426895 , @shchenz wrote:

> Seems passing `-Wl,-bplugin_opt:-no-integrated-as=1` from clang driver does 
> not work as expected. Is this expected or by design(We can only use 
> -fno-integrated-as but no -Wl,-bplugin_opt:-no-integrated-as=1)?
>
>   clang -flto -Wl,-bplugin_opt:-no-integrated-as=1 *.c -v
> -bplugin_opt:-no-integrated-as=1 -bplugin:../lib/libLTO.so 
> -bplugin_opt:-mcpu=pwr7 -bplugin_opt:-no-integrated-as=0 ;;; Note the second 
> -no-integrated-as=0 pass from clang front-end.

Thanks for catching this! The latest patch fixed the problem. As you rightly 
pointed out, always passing in ` -no-integrated-as=0` by default interferes 
with uses such as above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152924

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

[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as` on AIX

[Qiongsi Wu via Phabricator via cfe-commits]

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



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:700
+CmdArgs.push_back(Args.MakeArgString(
+Twine(PluginOptPrefix) + "-no-integrated-as=" + NoIntegratedAs));
+  else if (!UseIntegratedAs) {

MaskRay wrote:
> shchenz wrote:
> > qiongsiwu1 wrote:
> > > shchenz wrote:
> > > > qiongsiwu1 wrote:
> > > > > shchenz wrote:
> > > > > > Seems other options leverage the default value in the back end, for 
> > > > > > example the default value for `DisableIntegratedAS` in backend is 
> > > > > > false, so when the front end requires integrated-as, maybe we can 
> > > > > > save the option here?
> > > > > Ah thanks for the comment! 
> > > > > 
> > > > > > maybe we can save the option here?
> > > > > 
> > > > > Could you help me understand what we mean by "the option"? Do we mean 
> > > > > saving (or using?) the value of `-f[no]-integrated-as` explicitly 
> > > > > here somehow instead of relying on `ToolChain.useIntegratedAs()`? How 
> > > > > do we intend to use the saved option value? My understanding is that 
> > > > > `DisableIntegratedAS` takes its value from the option 
> > > > > `-no-integrated-as` if `-no-integrated-as` is specified. As pointed 
> > > > > out eariler, `DisableIntegratedAS` is false by default. This code 
> > > > > explicitly sets `-no-integrated-as` to `0` or `1`, so for the LTO use 
> > > > > case, we overwrite the back end default since the option is always 
> > > > > present. 
> > > > For example, if front-end requires to use integrated-assembler which is 
> > > > same with back-end's default value, we don't need to pass 
> > > > `-no-integrated-as=0`? We only pass the option `-no-integrated-as=1` 
> > > > when `if (IsOSAIX && !UseIntegratedAs)`.
> > > Ah I see! Thanks for the clarification! 
> > > 
> > > @w2yehia and I discussed this and we preferred setting the option 
> > > explicitly regardless of the backend's default. The reason was that we 
> > > did not want to leave a hanging case where the option was not passed to 
> > > the backend, a case in which we would rely on a non-local 
> > > option(`DisableIntegratedAS`)'s default value to control the backend. In 
> > > other words, always passing in the option allowed us to reason about this 
> > > code locally within this file. @w2yehia feel free to chime in if I am not 
> > > describing our discussion correctly. 
> > > 
> > > Could you help me understand the benefit of not passing in the option for 
> > > the default case? 
> > > Could you help me understand the benefit of not passing in the option for 
> > > the default case?
> > 
> > Too many options pass from front-end to back-end is a reason. And another 
> > reason is: I met one case that there is a back-end option has no default 
> > value, so each front-end, like clang and FORTRAN will have to explicitly 
> > pass the same option. I was asked to set a default in the back-end, so no 
> > need to explicitly set the options in each front-end.
> > 
> > If what I read is right, some bool type options like `EmulatedTLS`, 
> > `EnableStackSizeSection` are only passed when their values are not the same 
> > with the back-end's default.
> > 
> > I am ok to keep it as now if you guys already have an agreement. This is 
> > just minor I think.
> If you are going to add `-no-integrated-as=0`, I suggest that you rename the 
> option to `-integrated-as` and use `-integrated-as=1` instead.
`-no-integrated-as=0` is no longer added. The backend default will take care of 
the case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152924

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

[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as` on AIX

[Qiongsi Wu via Phabricator via cfe-commits]

qiongsiwu1 updated this revision to Diff 532110.
qiongsiwu1 added a comment.

Revising the option processing logic to not pass in `-no-integrated-as=0`. 
Thanks @shchenz and @MaskRay for the feedback!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152924

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/test/Driver/lto-aix.c
  clang/test/Driver/lto.c
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/lib/LTO/LTOCodeGenerator.cpp
  llvm/test/tools/llvm-lto/aix-sys-as.ll
  llvm/test/tools/llvm-lto/aix.ll
  llvm/tools/llc/llc.cpp

Index: llvm/tools/llc/llc.cpp
===
--- llvm/tools/llc/llc.cpp
+++ llvm/tools/llc/llc.cpp
@@ -106,10 +106,6 @@
  "'none' means that all ELF features can be used, "
  "regardless of binutils support"));
 
-static cl::opt
-NoIntegratedAssembler("no-integrated-as", cl::Hidden,
-  cl::desc("Disable integrated assembler"));
-
 static cl::opt
 PreserveComments("preserve-as-comments", cl::Hidden,
  cl::desc("Preserve Comments in outputted assembly"),
@@ -517,7 +513,6 @@
 
 Options.BinutilsVersion =
 TargetMachine::parseBinutilsVersion(BinutilsVersion);
-Options.DisableIntegratedAS = NoIntegratedAssembler;
 Options.MCOptions.ShowMCEncoding = ShowMCEncoding;
 Options.MCOptions.AsmVerbose = AsmVerbose;
 Options.MCOptions.PreserveAsmComments = PreserveComments;
Index: llvm/test/tools/llvm-lto/aix.ll
===
--- llvm/test/tools/llvm-lto/aix.ll
+++ llvm/test/tools/llvm-lto/aix.ll
@@ -1,4 +1,4 @@
-; REQUIRES: system-aix
+; REQUIRES: powerpc-registered-target
 ; RUN: llvm-as < %s > %t1
 ; RUN: llvm-lto %t1 | FileCheck %s
 
Index: llvm/test/tools/llvm-lto/aix-sys-as.ll
===
--- /dev/null
+++ llvm/test/tools/llvm-lto/aix-sys-as.ll
@@ -0,0 +1,12 @@
+; REQUIRES: system-aix
+; RUN: llvm-as < %s > %t1
+; RUN: llvm-lto -no-integrated-as=1 %t1 | FileCheck %s
+
+target triple = "powerpc-ibm-aix"
+
+define i32 @main() {
+entry:
+  ret i32 42
+}
+; CHECK: Wrote native object file
+
Index: llvm/lib/LTO/LTOCodeGenerator.cpp
===
--- llvm/lib/LTO/LTOCodeGenerator.cpp
+++ llvm/lib/LTO/LTOCodeGenerator.cpp
@@ -244,7 +244,7 @@
 
 bool LTOCodeGenerator::useAIXSystemAssembler() {
   const auto  = TargetMach->getTargetTriple();
-  return Triple.isOSAIX();
+  return Triple.isOSAIX() && Config.Options.DisableIntegratedAS;
 }
 
 bool LTOCodeGenerator::runAIXSystemAssembler(SmallString<128> ) {
Index: llvm/lib/CodeGen/CommandFlags.cpp
===
--- llvm/lib/CodeGen/CommandFlags.cpp
+++ llvm/lib/CodeGen/CommandFlags.cpp
@@ -81,6 +81,7 @@
 CGOPT(bool, StackRealign)
 CGOPT(std::string, TrapFuncName)
 CGOPT(bool, UseCtors)
+CGOPT(bool, DisableIntegratedAS)
 CGOPT(bool, RelaxELFRelocations)
 CGOPT_EXP(bool, DataSections)
 CGOPT_EXP(bool, FunctionSections)
@@ -487,6 +488,11 @@
   cl::init(false));
   CGBINDOPT(XCOFFReadOnlyPointers);
 
+  static cl::opt DisableIntegratedAS(
+  "no-integrated-as", cl::desc("Disable integrated assembler"),
+  cl::init(false));
+  CGBINDOPT(DisableIntegratedAS);
+
 #undef CGBINDOPT
 
   mc::RegisterMCTargetOptionsFlags();
@@ -540,6 +546,7 @@
   Options.GuaranteedTailCallOpt = getEnableGuaranteedTailCallOpt();
   Options.StackSymbolOrdering = getStackSymbolOrdering();
   Options.UseInitArray = !getUseCtors();
+  Options.DisableIntegratedAS = getDisableIntegratedAS();
   Options.RelaxELFRelocations = getRelaxELFRelocations();
   Options.DataSections =
   getExplicitDataSections().value_or(TheTriple.hasDefaultDataSections());
Index: llvm/include/llvm/CodeGen/CommandFlags.h
===
--- llvm/include/llvm/CodeGen/CommandFlags.h
+++ llvm/include/llvm/CodeGen/CommandFlags.h
@@ -94,6 +94,8 @@
 
 bool getUseCtors();
 
+bool getDisableIntegratedAS();
+
 bool getRelaxELFRelocations();
 
 bool getDataSections();
Index: clang/test/Driver/lto.c
===
--- clang/test/Driver/lto.c
+++ clang/test/Driver/lto.c
@@ -105,3 +105,10 @@
 // FLTO-THIN: -flto=thin
 // FLTO-THIN-NOT: "-flto"
 // FLTO-THIN-NOT: -flto=full
+//
+
+// Check that using -fno-integrated-as produces a warning on Linux
+// RUN: %clang --target=x86_64-unknown-linux -### %s -fno-integrated-as \
+// RUN: -flto 2>&1 | FileCheck --check-prefix=NO-INTEGRATED-AS %s
+//
+// NO-INTEGRATED-AS: clang: warning: ignoring '-fno-integrated-as' option as it is not currently supported for target 'x86_64-unknown-linux' 

[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as` on AIX

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:703
+Arg *A = Args.getLastArgNoClaim(options::OPT_fno_integrated_as);
+D.Diag(diag::warn_drv_unsupported_lto_option_for_target)
+<< A->getSpelling() << ToolChain.getTriple().getTriple();

The patch is titled AIX but this would change the Linux behavior. In my 
experience this diagnostic is not useful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152924

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

[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as` on AIX

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:700
+CmdArgs.push_back(Args.MakeArgString(
+Twine(PluginOptPrefix) + "-no-integrated-as=" + NoIntegratedAs));
+  else if (!UseIntegratedAs) {

shchenz wrote:
> qiongsiwu1 wrote:
> > shchenz wrote:
> > > qiongsiwu1 wrote:
> > > > shchenz wrote:
> > > > > Seems other options leverage the default value in the back end, for 
> > > > > example the default value for `DisableIntegratedAS` in backend is 
> > > > > false, so when the front end requires integrated-as, maybe we can 
> > > > > save the option here?
> > > > Ah thanks for the comment! 
> > > > 
> > > > > maybe we can save the option here?
> > > > 
> > > > Could you help me understand what we mean by "the option"? Do we mean 
> > > > saving (or using?) the value of `-f[no]-integrated-as` explicitly here 
> > > > somehow instead of relying on `ToolChain.useIntegratedAs()`? How do we 
> > > > intend to use the saved option value? My understanding is that 
> > > > `DisableIntegratedAS` takes its value from the option 
> > > > `-no-integrated-as` if `-no-integrated-as` is specified. As pointed out 
> > > > eariler, `DisableIntegratedAS` is false by default. This code 
> > > > explicitly sets `-no-integrated-as` to `0` or `1`, so for the LTO use 
> > > > case, we overwrite the back end default since the option is always 
> > > > present. 
> > > For example, if front-end requires to use integrated-assembler which is 
> > > same with back-end's default value, we don't need to pass 
> > > `-no-integrated-as=0`? We only pass the option `-no-integrated-as=1` when 
> > > `if (IsOSAIX && !UseIntegratedAs)`.
> > Ah I see! Thanks for the clarification! 
> > 
> > @w2yehia and I discussed this and we preferred setting the option 
> > explicitly regardless of the backend's default. The reason was that we did 
> > not want to leave a hanging case where the option was not passed to the 
> > backend, a case in which we would rely on a non-local 
> > option(`DisableIntegratedAS`)'s default value to control the backend. In 
> > other words, always passing in the option allowed us to reason about this 
> > code locally within this file. @w2yehia feel free to chime in if I am not 
> > describing our discussion correctly. 
> > 
> > Could you help me understand the benefit of not passing in the option for 
> > the default case? 
> > Could you help me understand the benefit of not passing in the option for 
> > the default case?
> 
> Too many options pass from front-end to back-end is a reason. And another 
> reason is: I met one case that there is a back-end option has no default 
> value, so each front-end, like clang and FORTRAN will have to explicitly pass 
> the same option. I was asked to set a default in the back-end, so no need to 
> explicitly set the options in each front-end.
> 
> If what I read is right, some bool type options like `EmulatedTLS`, 
> `EnableStackSizeSection` are only passed when their values are not the same 
> with the back-end's default.
> 
> I am ok to keep it as now if you guys already have an agreement. This is just 
> minor I think.
If you are going to add `-no-integrated-as=0`, I suggest that you rename the 
option to `-integrated-as` and use `-integrated-as=1` instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152924

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

[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as` on AIX

[ChenZheng via Phabricator via cfe-commits]

shchenz added a comment.

Seems passing `-Wl,-bplugin_opt:-no-integrated-as=1` from clang driver does not 
work as expected. Is this expected or by design(We can only use 
-fno-integrated-as but no -Wl,-bplugin_opt:-no-integrated-as=1)?

  clang -flto -Wl,-bplugin_opt:-no-integrated-as=1 *.c -v
-bplugin_opt:-no-integrated-as=1 -bplugin:../lib/libLTO.so 
-bplugin_opt:-mcpu=pwr7 -bplugin_opt:-no-integrated-as=0 ;;; Note the second 
-no-integrated-as=0 pass from clang front-end.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152924

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

[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as` on AIX

[ChenZheng via Phabricator via cfe-commits]

shchenz added inline comments.



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:700
+CmdArgs.push_back(Args.MakeArgString(
+Twine(PluginOptPrefix) + "-no-integrated-as=" + NoIntegratedAs));
+  else if (!UseIntegratedAs) {

qiongsiwu1 wrote:
> shchenz wrote:
> > qiongsiwu1 wrote:
> > > shchenz wrote:
> > > > Seems other options leverage the default value in the back end, for 
> > > > example the default value for `DisableIntegratedAS` in backend is 
> > > > false, so when the front end requires integrated-as, maybe we can save 
> > > > the option here?
> > > Ah thanks for the comment! 
> > > 
> > > > maybe we can save the option here?
> > > 
> > > Could you help me understand what we mean by "the option"? Do we mean 
> > > saving (or using?) the value of `-f[no]-integrated-as` explicitly here 
> > > somehow instead of relying on `ToolChain.useIntegratedAs()`? How do we 
> > > intend to use the saved option value? My understanding is that 
> > > `DisableIntegratedAS` takes its value from the option `-no-integrated-as` 
> > > if `-no-integrated-as` is specified. As pointed out eariler, 
> > > `DisableIntegratedAS` is false by default. This code explicitly sets 
> > > `-no-integrated-as` to `0` or `1`, so for the LTO use case, we overwrite 
> > > the back end default since the option is always present. 
> > For example, if front-end requires to use integrated-assembler which is 
> > same with back-end's default value, we don't need to pass 
> > `-no-integrated-as=0`? We only pass the option `-no-integrated-as=1` when 
> > `if (IsOSAIX && !UseIntegratedAs)`.
> Ah I see! Thanks for the clarification! 
> 
> @w2yehia and I discussed this and we preferred setting the option explicitly 
> regardless of the backend's default. The reason was that we did not want to 
> leave a hanging case where the option was not passed to the backend, a case 
> in which we would rely on a non-local option(`DisableIntegratedAS`)'s default 
> value to control the backend. In other words, always passing in the option 
> allowed us to reason about this code locally within this file. @w2yehia feel 
> free to chime in if I am not describing our discussion correctly. 
> 
> Could you help me understand the benefit of not passing in the option for the 
> default case? 
> Could you help me understand the benefit of not passing in the option for the 
> default case?

Too many options pass from front-end to back-end is a reason. And another 
reason is: I met one case that there is a back-end option has no default value, 
so each front-end, like clang and FORTRAN will have to explicitly pass the same 
option. I was asked to set a default in the back-end, so no need to explicitly 
set the options in each front-end.

If what I read is right, some bool type options like `EmulatedTLS`, 
`EnableStackSizeSection` are only passed when their values are not the same 
with the back-end's default.

I am ok to keep it as now if you guys already have an agreement. This is just 
minor I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152924

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

[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as` on AIX

[Qiongsi Wu via Phabricator via cfe-commits]

qiongsiwu1 added inline comments.



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:700
+CmdArgs.push_back(Args.MakeArgString(
+Twine(PluginOptPrefix) + "-no-integrated-as=" + NoIntegratedAs));
+  else if (!UseIntegratedAs) {

shchenz wrote:
> qiongsiwu1 wrote:
> > shchenz wrote:
> > > Seems other options leverage the default value in the back end, for 
> > > example the default value for `DisableIntegratedAS` in backend is false, 
> > > so when the front end requires integrated-as, maybe we can save the 
> > > option here?
> > Ah thanks for the comment! 
> > 
> > > maybe we can save the option here?
> > 
> > Could you help me understand what we mean by "the option"? Do we mean 
> > saving (or using?) the value of `-f[no]-integrated-as` explicitly here 
> > somehow instead of relying on `ToolChain.useIntegratedAs()`? How do we 
> > intend to use the saved option value? My understanding is that 
> > `DisableIntegratedAS` takes its value from the option `-no-integrated-as` 
> > if `-no-integrated-as` is specified. As pointed out eariler, 
> > `DisableIntegratedAS` is false by default. This code explicitly sets 
> > `-no-integrated-as` to `0` or `1`, so for the LTO use case, we overwrite 
> > the back end default since the option is always present. 
> For example, if front-end requires to use integrated-assembler which is same 
> with back-end's default value, we don't need to pass `-no-integrated-as=0`? 
> We only pass the option `-no-integrated-as=1` when `if (IsOSAIX && 
> !UseIntegratedAs)`.
Ah I see! Thanks for the clarification! 

@w2yehia and I discussed this and we preferred setting the option explicitly 
regardless of the backend's default. The reason was that we did not want to 
leave a hanging case where the option was not passed to the backend, a case in 
which we would rely on a non-local option(`DisableIntegratedAS`)'s default 
value to control the backend. In other words, always passing in the option 
allowed us to reason about this code locally within this file. @w2yehia feel 
free to chime in if I am not describing our discussion correctly. 

Could you help me understand the benefit of not passing in the option for the 
default case? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152924

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

[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as` on AIX

[ChenZheng via Phabricator via cfe-commits]

shchenz added inline comments.



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:700
+CmdArgs.push_back(Args.MakeArgString(
+Twine(PluginOptPrefix) + "-no-integrated-as=" + NoIntegratedAs));
+  else if (!UseIntegratedAs) {

qiongsiwu1 wrote:
> shchenz wrote:
> > Seems other options leverage the default value in the back end, for example 
> > the default value for `DisableIntegratedAS` in backend is false, so when 
> > the front end requires integrated-as, maybe we can save the option here?
> Ah thanks for the comment! 
> 
> > maybe we can save the option here?
> 
> Could you help me understand what we mean by "the option"? Do we mean saving 
> (or using?) the value of `-f[no]-integrated-as` explicitly here somehow 
> instead of relying on `ToolChain.useIntegratedAs()`? How do we intend to use 
> the saved option value? My understanding is that `DisableIntegratedAS` takes 
> its value from the option `-no-integrated-as` if `-no-integrated-as` is 
> specified. As pointed out eariler, `DisableIntegratedAS` is false by default. 
> This code explicitly sets `-no-integrated-as` to `0` or `1`, so for the LTO 
> use case, we overwrite the back end default since the option is always 
> present. 
For example, if front-end requires to use integrated-assembler which is same 
with back-end's default value, we don't need to pass `-no-integrated-as=0`? We 
only pass the option `-no-integrated-as=1` when `if (IsOSAIX && 
!UseIntegratedAs)`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152924

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

[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as` on AIX

[Qiongsi Wu via Phabricator via cfe-commits]

qiongsiwu1 added inline comments.



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:700
+CmdArgs.push_back(Args.MakeArgString(
+Twine(PluginOptPrefix) + "-no-integrated-as=" + NoIntegratedAs));
+  else if (!UseIntegratedAs) {

shchenz wrote:
> Seems other options leverage the default value in the back end, for example 
> the default value for `DisableIntegratedAS` in backend is false, so when the 
> front end requires integrated-as, maybe we can save the option here?
Ah thanks for the comment! 

> maybe we can save the option here?

Could you help me understand what we mean by "the option"? Do we mean saving 
(or using?) the value of `-f[no]-integrated-as` explicitly here somehow instead 
of relying on `ToolChain.useIntegratedAs()`? How do we intend to use the saved 
option value? My understanding is that `DisableIntegratedAS` takes its value 
from the option `-no-integrated-as` if `-no-integrated-as` is specified. As 
pointed out eariler, `DisableIntegratedAS` is false by default. This code 
explicitly sets `-no-integrated-as` to `0` or `1`, so for the LTO use case, we 
overwrite the back end default since the option is always present. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152924

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

[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as` on AIX

[ChenZheng via Phabricator via cfe-commits]

shchenz added a comment.

@qiongsiwu1 Hi, thanks for the patch. Almost good to me, except one small nit.




Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:700
+CmdArgs.push_back(Args.MakeArgString(
+Twine(PluginOptPrefix) + "-no-integrated-as=" + NoIntegratedAs));
+  else if (!UseIntegratedAs) {

Seems other options leverage the default value in the back end, for example the 
default value for `DisableIntegratedAS` in backend is false, so when the front 
end requires integrated-as, maybe we can save the option here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152924

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

[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as` on AIX

[Steven Wu via Phabricator via cfe-commits]

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

I see. Sounds fine to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152924

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

[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as` on AIX

[Qiongsi Wu via Phabricator via cfe-commits]

qiongsiwu1 added a comment.

In D152924#4421624 , @steven_wu wrote:

> `lld` does not use LTOCodeGenerator.
>
> I am also not sure how disable integrated assembler suppose to work? Do you 
> have an end to end design for how to invoke assembler during linking? For 
> current libLTO API, this is probably going to produce assembler file buffer 
> to linker, which is not usable by linker.

Thanks for chiming in @steven_wu! This patch is enabling 
`-f[no-]-integrated-as` on //AIX only//. There is already code to invoke the 
assembler during linking 
https://github.com/llvm/llvm-project/blob/807adcf4b9cca5586fd1fb669811768528d9dd1d/llvm/lib/LTO/LTOCodeGenerator.cpp#L343.
 `clang` will emit a warning on other platforms. The patch title is revised to 
reflect that this feature is AIX only at the moment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152924

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

[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as`

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

`lld` does not use LTOCodeGenerator.

I am also not sure how disable integrated assembler suppose to work? Do you 
have an end to end design for how to invoke assembler during linking? For 
current libLTO API, this is probably going to produce assembler file buffer to 
linker, which is not usable by linker.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152924

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

[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as`

[Qiongsi Wu via Phabricator via cfe-commits]

qiongsiwu1 created this revision.
qiongsiwu1 added reviewers: w2yehia, MaskRay, shchenz, Esme.
qiongsiwu1 added a project: clang.
Herald added subscribers: ormris, steven_wu, hiraditya, inglorion.
Herald added a project: All.
qiongsiwu1 requested review of this revision.
Herald added subscribers: llvm-commits, cfe-commits.
Herald added a project: LLVM.

`libLTO` currently ignores the `-f[no-]integrated-as` flags. This patch teaches 
`libLTO` to respect them.

The implementation consists of two parts:

1. Migrate `llc`'s `-no-integrated-as` option to a codegen option so that the 
option is available to `libLTO`/`lld`/`gold`.
2. Teach `clang` to pass `-no-integrated-as` accordingly to `libLTO` depending 
on the `-f[no-]integrated-as` flags.

`libLTO` cannot use system assemblers on platforms other than AIX. As part of 
the option processing, a warning is implemented if `-fno-integrated-as` is used 
for LTO on platforms other than AIX.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D152924

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/test/Driver/lto-aix.c
  clang/test/Driver/lto.c
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/lib/LTO/LTOCodeGenerator.cpp
  llvm/test/tools/llvm-lto/aix-sys-as.ll
  llvm/test/tools/llvm-lto/aix.ll
  llvm/tools/llc/llc.cpp

Index: llvm/tools/llc/llc.cpp
===
--- llvm/tools/llc/llc.cpp
+++ llvm/tools/llc/llc.cpp
@@ -106,10 +106,6 @@
  "'none' means that all ELF features can be used, "
  "regardless of binutils support"));
 
-static cl::opt
-NoIntegratedAssembler("no-integrated-as", cl::Hidden,
-  cl::desc("Disable integrated assembler"));
-
 static cl::opt
 PreserveComments("preserve-as-comments", cl::Hidden,
  cl::desc("Preserve Comments in outputted assembly"),
@@ -517,7 +513,6 @@
 
 Options.BinutilsVersion =
 TargetMachine::parseBinutilsVersion(BinutilsVersion);
-Options.DisableIntegratedAS = NoIntegratedAssembler;
 Options.MCOptions.ShowMCEncoding = ShowMCEncoding;
 Options.MCOptions.AsmVerbose = AsmVerbose;
 Options.MCOptions.PreserveAsmComments = PreserveComments;
Index: llvm/test/tools/llvm-lto/aix.ll
===
--- llvm/test/tools/llvm-lto/aix.ll
+++ llvm/test/tools/llvm-lto/aix.ll
@@ -1,4 +1,4 @@
-; REQUIRES: system-aix
+; REQUIRES: powerpc-registered-target
 ; RUN: llvm-as < %s > %t1
 ; RUN: llvm-lto %t1 | FileCheck %s
 
Index: llvm/test/tools/llvm-lto/aix-sys-as.ll
===
--- /dev/null
+++ llvm/test/tools/llvm-lto/aix-sys-as.ll
@@ -0,0 +1,12 @@
+; REQUIRES: system-aix
+; RUN: llvm-as < %s > %t1
+; RUN: llvm-lto -no-integrated-as=1 %t1 | FileCheck %s
+
+target triple = "powerpc-ibm-aix"
+
+define i32 @main() {
+entry:
+  ret i32 42
+}
+; CHECK: Wrote native object file
+
Index: llvm/lib/LTO/LTOCodeGenerator.cpp
===
--- llvm/lib/LTO/LTOCodeGenerator.cpp
+++ llvm/lib/LTO/LTOCodeGenerator.cpp
@@ -244,7 +244,7 @@
 
 bool LTOCodeGenerator::useAIXSystemAssembler() {
   const auto  = TargetMach->getTargetTriple();
-  return Triple.isOSAIX();
+  return Triple.isOSAIX() && Config.Options.DisableIntegratedAS;
 }
 
 bool LTOCodeGenerator::runAIXSystemAssembler(SmallString<128> ) {
Index: llvm/lib/CodeGen/CommandFlags.cpp
===
--- llvm/lib/CodeGen/CommandFlags.cpp
+++ llvm/lib/CodeGen/CommandFlags.cpp
@@ -81,6 +81,7 @@
 CGOPT(bool, StackRealign)
 CGOPT(std::string, TrapFuncName)
 CGOPT(bool, UseCtors)
+CGOPT(bool, DisableIntegratedAS)
 CGOPT(bool, RelaxELFRelocations)
 CGOPT_EXP(bool, DataSections)
 CGOPT_EXP(bool, FunctionSections)
@@ -487,6 +488,11 @@
   cl::init(false));
   CGBINDOPT(XCOFFReadOnlyPointers);
 
+  static cl::opt DisableIntegratedAS(
+  "no-integrated-as", cl::desc("Disable integrated assembler"),
+  cl::init(false));
+  CGBINDOPT(DisableIntegratedAS);
+
 #undef CGBINDOPT
 
   mc::RegisterMCTargetOptionsFlags();
@@ -540,6 +546,7 @@
   Options.GuaranteedTailCallOpt = getEnableGuaranteedTailCallOpt();
   Options.StackSymbolOrdering = getStackSymbolOrdering();
   Options.UseInitArray = !getUseCtors();
+  Options.DisableIntegratedAS = getDisableIntegratedAS();
   Options.RelaxELFRelocations = getRelaxELFRelocations();
   Options.DataSections =
   getExplicitDataSections().value_or(TheTriple.hasDefaultDataSections());
Index: llvm/include/llvm/CodeGen/CommandFlags.h
===
--- llvm/include/llvm/CodeGen/CommandFlags.h
+++ llvm/include/llvm/CodeGen/CommandFlags.h
@@ -94,6 +94,8 @@
 
 bool getUseCtors();
 
+bool getDisableIntegratedAS();
+
 bool