[flang] Check x86 CPU and issue diagnostics

[eugeneepshteyn]

Check for invalid -march option for x86_64 and issue diagnostics similar to clang.

Fix for #97466

[llvmbot]

@llvm/pr-subscribers-flang-driver

Author: Eugene Epshteyn (eugeneepshteyn)

Changes

Check for invalid -march option for x86_64 and issue diagnostics similar to clang.

Fix for #97466

---

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

1 Files Affected:

• (modified) flang/tools/flang-driver/fc1_main.cpp (+36-1)

diff --git a/flang/tools/flang-driver/fc1_main.cpp b/flang/tools/flang-driver/fc1_main.cpp
index 561a0dd5524e3..ce430ee30ca87 100644
--- a/flang/tools/flang-driver/fc1_main.cpp
+++ b/flang/tools/flang-driver/fc1_main.cpp
@@ -27,6 +27,8 @@
 #include "llvm/Option/OptTable.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/Support/raw_ostream.h"
+#include "llvm/TargetParser/X86TargetParser.h"
+#include "llvm/ADT/StringExtras.h"
 
 #include <cstdio>
 
@@ -39,7 +41,6 @@ static int printSupportedCPUs(llvm::StringRef triple) {
       llvm::TargetRegistry::lookupTarget(triple, error);
   if (!target) {
     llvm::errs() << error;
-    return 1;
   }
 
   // the target machine will handle the mcpu printing
@@ -50,6 +51,35 @@ static int printSupportedCPUs(llvm::StringRef triple) {
   return 0;
 }
 
+/// Check that given CPU is valid for given target.
+static bool checkSupportedCPU(clang::DiagnosticsEngine &diags,
+                              llvm::StringRef str_cpu,
+                              llvm::StringRef str_triple) {
+
+  llvm::Triple triple{str_triple};
+
+  // Need to check for empty CPU string, because it can be empty for some
+  // cases, e.g., "-x cuda" compilation.
+  if (triple.getArch() == llvm::Triple::x86_64 && !str_cpu.empty()) {
+    constexpr bool only64bit{true};
+    llvm::X86::CPUKind x86cpu = llvm::X86::parseArchX86(str_cpu, only64bit);
+    if (x86cpu == llvm::X86::CK_None) {
+      diags.Report(clang::diag::err_target_unknown_cpu) << str_cpu;
+      llvm::SmallVector<llvm::StringRef, 32> validList;
+      llvm::X86::fillValidCPUArchList(validList, only64bit);
+      if (!validList.empty())
+        diags.Report(clang::diag::note_valid_options)
+            << llvm::join(validList, ", ");
+
+      return false;
+    }
+  }
+
+  // TODO: only support check for x86_64 for now. Anything else is considered
+  // as "supported".
+  return true;
+}
+
 int fc1_main(llvm::ArrayRef<const char *> argv, const char *argv0) {
   // Create CompilerInstance
   std::unique_ptr<CompilerInstance> flang(new CompilerInstance());
@@ -82,6 +112,11 @@ int fc1_main(llvm::ArrayRef<const char *> argv, const char *argv0) {
   if (flang->getFrontendOpts().printSupportedCPUs)
     return printSupportedCPUs(flang->getInvocation().getTargetOpts().triple);
 
+  // Check that requested CPU can be properly supported
+  success = success &&
+            checkSupportedCPU(diags, flang->getInvocation().getTargetOpts().cpu,
+                              flang->getInvocation().getTargetOpts().triple);
+
   diagsBuffer->flushDiagnostics(flang->getDiagnostics());
 
   if (!success)

[github-actions]

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Discourse for more information.

[github-actions]

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

[kiranchandramohan]

Thanks @eugeneepshteyn. Can you add a test?

[tarunprabhu]

How does clang check for this? Can we share some code between clang and flang?

[eugeneepshteyn]

How does clang check for this? Can we share some code between clang and flang?

See this comment: #97466 (comment)

To share the code would be more work than I currently have time to do. My goal was to try to fix immediate flang crash issue for x86. (ARM doesn't crash for invalid CPU, but it does something different.) Perhaps we can fix x86 now and file a new "feature" issue for trying to get some code re-use in flang and clang?

[eugeneepshteyn]

Thanks @eugeneepshteyn. Can you add a test?

Done

[tarunprabhu]

How does clang check for this? Can we share some code between clang and flang?

See this comment: #97466 (comment)

To share the code would be more work than I currently have time to do. My goal was to try to fix immediate flang crash issue for x86. (ARM doesn't crash for invalid CPU, but it does something different.) Perhaps we can fix x86 now and file a new "feature" issue for trying to get some code re-use in flang and clang?

Unless @banach-space has any objections, I am ok with it. Could you add a comment to the code explaining that this could be done better and that it is a short-term fix. You could refer to the issues and comments. Filing a feature issue will also help track it.

[banach-space]

Thanks for the fix!

How does clang check for this? Can we share some code between clang and flang?

See this comment: #97466 (comment)
To share the code would be more work than I currently have time to do. My goal was to try to fix immediate flang crash issue for x86. (ARM doesn't crash for invalid CPU, but it does something different.) Perhaps we can fix x86 now and file a new "feature" issue for trying to get some code re-use in flang and clang?

Unless @banach-space has any objections, I am ok with it. Could you add a comment to the code explaining that this could be done better and that it is a short-term fix. You could refer to the issues and comments. Filing a feature issue will also help track it.

If you are OK with this then so am I :)

I suggest being explicit about the limitations/assumptions made. Specifically, checkSupportedCPU suggests that this is a generic hook that's meant to be used by all targets. However, this is clearly an X86-specific fix. Lets rename the hook accordingly and add TODOs.

Also, back when this was reported I put some cycles into providing possible "generic" solutions:

• [Flang] flang-new ICE when specifying unsupported -march name on x86 #97466 (comment)

I don't see any justification not to follow that other than:

To share the code would be more work than I currently have time to do.

Still, could you include this somewhere? It should be "easy" to identify why wasn't the "generic" route followed.

Perhaps we can fix x86 now and file a new "feature" issue for trying to get some code re-use in flang and clang?

Please create a ticket.

-Andrzej