[flang]Add support for -moutline-atomics and -mno-outline-atomics #78755

Leporacanthicus · 2024-01-19T18:39:33Z

This adds the support to add the target-feature.

It will be fully supported once the generic support for adding target-cpu and target-features attributes to functions has landed. See #78289. Regression tests for that will be added once that PR lands.

llvmbot · 2024-01-19T18:40:06Z

@llvm/pr-subscribers-flang-fir-hlfir
@llvm/pr-subscribers-flang-driver
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: Mats Petersson (Leporacanthicus)

Changes

This adds the support to add the target-feature.

It will be fully supported once the generic support for adding target-cpu and target-features attributes to functions has landed. See #78289. Regression tests for that will be added once that PR lands.

Full diff: https://github.com/llvm/llvm-project/pull/78755.diff

6 Files Affected:

(modified) clang/include/clang/Driver/Options.td (+2-2)
(modified) clang/lib/Driver/ToolChains/Flang.cpp (+22-1)
(modified) flang/lib/Frontend/CompilerInvocation.cpp (+15)
(added) flang/test/Driver/aarch64-outline-atomics.f90 (+11)
(modified) flang/test/Driver/driver-help-hidden.f90 (+2)
(modified) flang/test/Driver/driver-help.f90 (+4)

diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index d2e6c3ff721c27..b8be694bf0bf80 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -4960,10 +4960,10 @@ def mno_fmv : Flag<["-"], "mno-fmv">, Group<f_clang_Group>,
   Visibility<[ClangOption, CC1Option]>,
   HelpText<"Disable function multiversioning">;
 def moutline_atomics : Flag<["-"], "moutline-atomics">, Group<f_clang_Group>,
-  Visibility<[ClangOption, CC1Option]>,
+  Visibility<[ClangOption, CC1Option, FlangOption, FC1Option]>,
   HelpText<"Generate local calls to out-of-line atomic operations">;
 def mno_outline_atomics : Flag<["-"], "mno-outline-atomics">, Group<f_clang_Group>,
-  Visibility<[ClangOption, CC1Option]>,
+  Visibility<[ClangOption, CC1Option, FlangOption, FC1Option]>,
   HelpText<"Don't generate local calls to out-of-line atomic operations">;
 def mno_implicit_float : Flag<["-"], "mno-implicit-float">, Group<m_Group>,
   HelpText<"Don't generate implicit floating point or vector instructions">;
diff --git a/clang/lib/Driver/ToolChains/Flang.cpp b/clang/lib/Driver/ToolChains/Flang.cpp
index 03d68c3df7fb37..7394ef6f601c7e 100644
--- a/clang/lib/Driver/ToolChains/Flang.cpp
+++ b/clang/lib/Driver/ToolChains/Flang.cpp
@@ -354,6 +354,27 @@ void Flang::addTargetOptions(const ArgList &Args,
     CmdArgs.push_back(Args.MakeArgString(CPU));
   }
 
+  if (Arg *A = Args.getLastArg(options::OPT_moutline_atomics,
+                               options::OPT_mno_outline_atomics)) {
+    // Option -moutline-atomics supported for AArch64 target only.
+    if (!Triple.isAArch64()) {
+      D.Diag(diag::warn_drv_moutline_atomics_unsupported_opt)
+          << Triple.getArchName() << A->getOption().getName();
+    } else {
+      if (A->getOption().matches(options::OPT_moutline_atomics)) {
+        CmdArgs.push_back("-target-feature");
+        CmdArgs.push_back("+outline-atomics");
+      } else {
+        CmdArgs.push_back("-target-feature");
+        CmdArgs.push_back("-outline-atomics");
+      }
+    }
+  } else if (Triple.isAArch64() &&
+             getToolChain().IsAArch64OutlineAtomicsDefault(Args)) {
+    CmdArgs.push_back("-target-feature");
+    CmdArgs.push_back("+outline-atomics");
+  }
+
   // Add the target features.
   switch (TC.getArch()) {
   default:
@@ -674,7 +695,7 @@ void Flang::ConstructJob(Compilation &C, const JobAction &JA,
   CmdArgs.push_back(Args.MakeArgString(TripleStr));
 
   if (isa<PreprocessJobAction>(JA)) {
-      CmdArgs.push_back("-E");
+    CmdArgs.push_back("-E");
   } else if (isa<CompileJobAction>(JA) || isa<BackendJobAction>(JA)) {
     if (JA.getType() == types::TY_Nothing) {
       CmdArgs.push_back("-fsyntax-only");
diff --git a/flang/lib/Frontend/CompilerInvocation.cpp b/flang/lib/Frontend/CompilerInvocation.cpp
index a3c41fb4611f56..74127327e798e6 100644
--- a/flang/lib/Frontend/CompilerInvocation.cpp
+++ b/flang/lib/Frontend/CompilerInvocation.cpp
@@ -402,6 +402,21 @@ static void parseTargetArgs(TargetOptions &opts, llvm::opt::ArgList &args) {
   for (const llvm::opt::Arg *currentArg :
        args.filtered(clang::driver::options::OPT_target_feature))
     opts.featuresAsWritten.emplace_back(currentArg->getValue());
+
+  llvm::Triple targetTriple{llvm::Triple(opts.triple)};
+  if (const llvm::opt::Arg *A =
+          args.getLastArg(clang::driver::options::OPT_moutline_atomics,
+                          clang::driver::options::OPT_mno_outline_atomics)) {
+    // Option -moutline-atomics supported for AArch64 target only.
+    if (!targetTriple.isAArch64()) {
+      if (A->getOption().matches(
+              clang::driver::options::OPT_moutline_atomics)) {
+        opts.featuresAsWritten.push_back("+outline-atomics");
+      } else {
+        opts.featuresAsWritten.push_back("-outline-atomics");
+      }
+    }
+  }
 }
 
 // Tweak the frontend configuration based on the frontend action
diff --git a/flang/test/Driver/aarch64-outline-atomics.f90 b/flang/test/Driver/aarch64-outline-atomics.f90
new file mode 100644
index 00000000000000..f469d50290ef04
--- /dev/null
+++ b/flang/test/Driver/aarch64-outline-atomics.f90
@@ -0,0 +1,11 @@
+! Test that flang-new forwards the -moutline-atomics and -mno-outline-atomics.
+! RUN: %flang -moutline-atomics --target=aarch64-none-none -### %s -o %t 2>&1  | FileCheck %s
+! CHECK: "-target-feature" "+outline-atomics"
+
+! RUN: %flang -mno-outline-atomics --target=aarch64-none-none -### %s -o %t 2>&1  | FileCheck %s --check-prefix=CHECK-NOOUTLINE
+! CHECK-NOOUTLINE: "-target-feature" "-outline-atomics"
+
+! RUN: %flang -mno-outline-atomics --target=x86-none-none -### %s -o %t 2>&1  | FileCheck %s --check-prefix=CHECK-ERRMSG
+! CHECK-ERRMSG: warning: 'x86' does not support '-mno-outline-atomics'
+
+
diff --git a/flang/test/Driver/driver-help-hidden.f90 b/flang/test/Driver/driver-help-hidden.f90
index 426b0e5a1c367d..13698b65ae461b 100644
--- a/flang/test/Driver/driver-help-hidden.f90
+++ b/flang/test/Driver/driver-help-hidden.f90
@@ -122,7 +122,9 @@
 ! CHECK-NEXT: -mllvm=<arg>            Alias for -mllvm
 ! CHECK-NEXT: -mllvm <value>          Additional arguments to forward to LLVM's option processing
 ! CHECK-NEXT: -mmlir <value>          Additional arguments to forward to MLIR's option processing
+! CHECK-NEXT: -mno-outline-atomics    Don't generate local calls to out-of-line atomic operations
 ! CHECK-NEXT: -module-dir <dir>       Put MODULE files in <dir>
+! CHECK-NEXT: -moutline-atomics       Generate local calls to out-of-line atomic operations
 ! CHECK-NEXT: -mrvv-vector-bits=<value>
 ! CHECK-NEXT:                         Specify the size in bits of an RVV vector register
 ! CHECK-NEXT: -msve-vector-bits=<value>
diff --git a/flang/test/Driver/driver-help.f90 b/flang/test/Driver/driver-help.f90
index 221da6439764b4..73f5d6ee846d3a 100644
--- a/flang/test/Driver/driver-help.f90
+++ b/flang/test/Driver/driver-help.f90
@@ -108,7 +108,9 @@
 ! HELP-NEXT: -mllvm=<arg>            Alias for -mllvm
 ! HELP-NEXT: -mllvm <value>          Additional arguments to forward to LLVM's option processing
 ! HELP-NEXT: -mmlir <value>          Additional arguments to forward to MLIR's option processing
+! HELP-NEXT: -mno-outline-atomics    Don't generate local calls to out-of-line atomic operations
 ! HELP-NEXT: -module-dir <dir>       Put MODULE files in <dir>
+! HELP-NEXT: -moutline-atomics       Generate local calls to out-of-line atomic operations
 ! HELP-NEXT: -mrvv-vector-bits=<value>
 ! HELP-NEXT:                         Specify the size in bits of an RVV vector register
 ! HELP-NEXT: -msve-vector-bits=<value>
@@ -246,8 +248,10 @@
 ! HELP-FC1-NEXT: -mframe-pointer=<value> Specify which frame pointers to retain.
 ! HELP-FC1-NEXT: -mllvm <value>          Additional arguments to forward to LLVM's option processing
 ! HELP-FC1-NEXT: -mmlir <value>          Additional arguments to forward to MLIR's option processing
+! HELP-FC1-NEXT: -mno-outline-atomics    Don't generate local calls to out-of-line atomic operations
 ! HELP-FC1-NEXT: -module-dir <dir>       Put MODULE files in <dir>
 ! HELP-FC1-NEXT: -module-suffix <suffix> Use <suffix> as the suffix for module files (the default value is `.mod`)
+! HELP-FC1-NEXT: -moutline-atomics       Generate local calls to out-of-line atomic operations
 ! HELP-FC1-NEXT: -mreassociate           Allow reassociation transformations for floating-point instructions
 ! HELP-FC1-NEXT: -mrelocation-model <value>
 ! HELP-FC1-NEXT:                         The relocation model to use

kiranchandramohan · 2024-01-19T21:10:32Z

flang/test/Driver/aarch64-outline-atomics.f90

+! RUN: %flang -moutline-atomics --target=aarch64-none-none -### %s -o %t 2>&1  | FileCheck %s
+! CHECK: "-target-feature" "+outline-atomics"
+
+! RUN: %flang -mno-outline-atomics --target=aarch64-none-none -### %s -o %t 2>&1  | FileCheck %s --check-prefix=CHECK-NOOUTLINE
+! CHECK-NOOUTLINE: "-target-feature" "-outline-atomics"
+
+! RUN: %flang -mno-outline-atomics --target=x86-none-none -### %s -o %t 2>&1  | FileCheck %s --check-prefix=CHECK-ERRMSG
+! CHECK-ERRMSG: warning: 'x86' does not support '-mno-outline-atomics'


Should there be a test to check the behaviour when no outline flags are specified and the target is aarch64?

kiranchandramohan · 2024-01-19T21:10:56Z

clang/lib/Driver/ToolChains/Flang.cpp

+  if (Arg *A = Args.getLastArg(options::OPT_moutline_atomics,
+                               options::OPT_mno_outline_atomics)) {
+    // Option -moutline-atomics supported for AArch64 target only.
+    if (!Triple.isAArch64()) {
+      D.Diag(diag::warn_drv_moutline_atomics_unsupported_opt)
+          << Triple.getArchName() << A->getOption().getName();
+    } else {
+      if (A->getOption().matches(options::OPT_moutline_atomics)) {
+        CmdArgs.push_back("-target-feature");
+        CmdArgs.push_back("+outline-atomics");
+      } else {
+        CmdArgs.push_back("-target-feature");
+        CmdArgs.push_back("-outline-atomics");
+      }
+    }
+  } else if (Triple.isAArch64() &&
+             getToolChain().IsAArch64OutlineAtomicsDefault(Args)) {
+    CmdArgs.push_back("-target-feature");
+    CmdArgs.push_back("+outline-atomics");
+  }


Can this code be shared with Clang ?

That's usually done in places like https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/CommonArgs.cpp

tblah · 2024-01-22T11:24:51Z

flang/lib/Frontend/CompilerInvocation.cpp

+          args.getLastArg(clang::driver::options::OPT_moutline_atomics,
+                          clang::driver::options::OPT_mno_outline_atomics)) {
+    // Option -moutline-atomics supported for AArch64 target only.
+    if (!targetTriple.isAArch64()) {


Is this condition the wrong way around?

Clang has support for genarting atomic operations as functions rather than inline atomic operations, using a target feature. This is useful for code-size, some cases of debugging, and it also affects the ability to inline functions from C into Fortran or the other way around when using LTO - the inliner will check the target features to make sure functions are compatible, and a difference in outline-atomics would disable the inlining of an outline-atomics into a non-outline-atomics function.

Also invert condition pointed out in review.

github-actions · 2024-01-31T15:11:57Z

✅ With the latest revision this PR passed the C/C++ code formatter.

kiranchandramohan · 2024-01-31T15:11:54Z

clang/lib/Driver/ToolChains/CommonArgs.cpp

@@ -2790,3 +2790,27 @@ void tools::addHIPRuntimeLibArgs(const ToolChain &TC, Compilation &C,
    }
  }
 }
+
+void tools::addOutlineAtomicsArgs(const Driver &D, const ToolChain &TC, const llvm::opt::ArgList &Args, llvm::opt::ArgStringList & CmdArgs, const llvm::Triple &Triple)


There is probably a clang-format issue here.

kiranchandramohan · 2024-01-31T15:12:43Z

clang/lib/Driver/ToolChains/CommonArgs.h

+void addOutlineAtomicsArgs(const Driver &D, const ToolChain &TC, const llvm::opt::ArgList &Args, llvm::opt::ArgStringList &CmdArgs, const llvm::Triple &Triple);
+


Nit: clang-format

kiranchandramohan · 2024-01-31T15:12:58Z

clang/lib/Driver/ToolChains/CommonArgs.h

 } // end namespace tools
 } // end namespace driver
 } // end namespace clang

 clang::CodeGenOptions::FramePointerKind
 getFramePointerKind(const llvm::opt::ArgList &Args, const llvm::Triple &Triple);

+


Nit: accidental change?

kiranchandramohan · 2024-01-31T15:13:15Z

clang/lib/Driver/ToolChains/Flang.cpp

-      CmdArgs.push_back("-E");
+    CmdArgs.push_back("-E");


Nit: accidental change.

kiranchandramohan · 2024-01-31T15:16:54Z

flang/lib/Frontend/CompilerInvocation.cpp

+  llvm::Triple targetTriple{llvm::Triple(opts.triple)};
+  if (const llvm::opt::Arg *A =
+          args.getLastArg(clang::driver::options::OPT_moutline_atomics,
+                          clang::driver::options::OPT_mno_outline_atomics)) {
+    // Option -moutline-atomics supported for AArch64 target only.
+    if (targetTriple.isAArch64()) {
+      if (A->getOption().matches(
+              clang::driver::options::OPT_moutline_atomics)) {
+        opts.featuresAsWritten.push_back("+outline-atomics");
+      } else {
+        opts.featuresAsWritten.push_back("-outline-atomics");
+      }
+    }
+  }


Is this required? I did not see a corresponding change in Clang frontend driver.

kiranchandramohan

LG. Please address or reply to comments before submitting.

kiranchandramohan · 2024-02-02T12:58:00Z

flang/test/Lower/aarch64-outline-atomics.f90

@@ -0,0 +1,18 @@
+! RUN: %flang -S -emit-llvm --target=aarch64-none-none -moutline-atomics -o - %s | FileCheck %s --check-prefixes=CHECKON,CHECKALL


I think it is better if this is in the integration test directory.

clementval · 2024-02-05T18:13:55Z

This is breaking couple of arm buildbots.

Leporacanthicus · 2024-02-05T18:19:13Z

Thanks for letting me klnow. I have a fix in #80717 I'm not entirely sure why it's broken on Arm but not x86 - the tests run on both, and should give identical output [but clearly doesn't].

…

-- Mats [https://opengraph.githubassets.com/ce56cc443042f8ad8e40eab60d1e7385e82143e2d8f23e4ebfed483158d90a84/llvm/llvm-project/pull/80717]<https://github.com/llvm/llvm-project/pull/80717> Fix broken ARM processor features test by Leporacanthicus ・ Pull Request #80717 ・ llvm/llvm-project<#80717> This test failed when I pulled latest changes. I suspect it's my fault... github.com

________________________________ From: Valentin Clement (バレンタインクレメン) ***@***.***> Sent: 05 February 2024 18:14 To: llvm/llvm-project ***@***.***> Cc: Mats Petersson ***@***.***>; State change ***@***.***> Subject: Re: [llvm/llvm-project] [flang]Add support for -moutline-atomics and -mno-outline-atomics (PR #78755) This is breaking couple of arm buildbots. ― Reply to this email directly, view it on GitHub<#78755 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABUDZBN3CSJ627INV3FTEMTYSEOPDAVCNFSM6AAAAABCCL4I56VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRXGY4DIOBUGU>. You are receiving this because you modified the open/close state.Message ID: ***@***.***> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

clementval · 2024-02-05T18:27:06Z

If you think this is the right fix then you should push it to solve the buildbots or revert the initial commit.

…vm#78755) This adds the support to add the target-feature to outline atomic operations (calling the runtime library instead).

Leporacanthicus requested review from DavidTruby, banach-space, tblah, kiranchandramohan and UsmanNadeem January 19, 2024 18:39

llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' flang:driver flang Flang issues not falling into any other category labels Jan 19, 2024

kiranchandramohan reviewed Jan 19, 2024

View reviewed changes

tblah reviewed Jan 22, 2024

View reviewed changes

Leporacanthicus added 2 commits January 31, 2024 10:54

Make common code for setting args

26e268b

Also invert condition pointed out in review.

Leporacanthicus force-pushed the outline-atomics branch from 7dcafdd to 26e268b Compare January 31, 2024 15:09

kiranchandramohan reviewed Jan 31, 2024

View reviewed changes

Leporacanthicus added 2 commits January 31, 2024 19:47

Fix formatting and update test

cb62b1e

Add lowering test

f80bd4f

llvmbot added the flang:fir-hlfir label Feb 2, 2024

Fix more formatting

6a3148a

kiranchandramohan approved these changes Feb 2, 2024

View reviewed changes

Leporacanthicus added 2 commits February 2, 2024 15:49

Update from review comments

1161aff

Fix broken test

0368874

Leporacanthicus merged commit 992d852 into llvm:main Feb 5, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[flang]Add support for -moutline-atomics and -mno-outline-atomics #78755

[flang]Add support for -moutline-atomics and -mno-outline-atomics #78755

Leporacanthicus commented Jan 19, 2024

llvmbot commented Jan 19, 2024 •

edited

kiranchandramohan Jan 19, 2024

kiranchandramohan Jan 19, 2024

banach-space Jan 22, 2024

tblah Jan 22, 2024

github-actions bot commented Jan 31, 2024 •

edited

kiranchandramohan Jan 31, 2024

kiranchandramohan Jan 31, 2024

kiranchandramohan Jan 31, 2024

kiranchandramohan Jan 31, 2024

kiranchandramohan Jan 31, 2024

kiranchandramohan left a comment

kiranchandramohan Feb 2, 2024

clementval commented Feb 5, 2024

Leporacanthicus commented Feb 5, 2024 via email

clementval commented Feb 5, 2024

		void addOutlineAtomicsArgs(const Driver &D, const ToolChain &TC, const llvm::opt::ArgList &Args, llvm::opt::ArgStringList &CmdArgs, const llvm::Triple &Triple);

		@@ -0,0 +1,18 @@
		! RUN: %flang -S -emit-llvm --target=aarch64-none-none -moutline-atomics -o - %s \| FileCheck %s --check-prefixes=CHECKON,CHECKALL

[flang]Add support for -moutline-atomics and -mno-outline-atomics #78755

[flang]Add support for -moutline-atomics and -mno-outline-atomics #78755

Conversation

Leporacanthicus commented Jan 19, 2024

llvmbot commented Jan 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 31, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kiranchandramohan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clementval commented Feb 5, 2024

Leporacanthicus commented Feb 5, 2024 via email

clementval commented Feb 5, 2024

llvmbot commented Jan 19, 2024 •

edited

github-actions bot commented Jan 31, 2024 •

edited