[flang] Add MSC_VER and target arch defines when targeting the MSVC ABI #73250

DavidTruby · 2023-11-23T15:36:52Z

Fixes #67675

llvmbot · 2023-11-23T15:37:22Z

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: David Truby (DavidTruby)

Changes

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

2 Files Affected:

(modified) clang/lib/Driver/ToolChains/Flang.cpp (+24)
(added) flang/test/Driver/msvc-defines.f90 (+10)

diff --git a/clang/lib/Driver/ToolChains/Flang.cpp b/clang/lib/Driver/ToolChains/Flang.cpp
index 01f07b9228256cf..87aa546e5549423 100644
--- a/clang/lib/Driver/ToolChains/Flang.cpp
+++ b/clang/lib/Driver/ToolChains/Flang.cpp
@@ -204,6 +204,29 @@ void Flang::AddAArch64TargetArgs(const ArgList &Args,
   }
 }
 
+static void addVSDefines(const ToolChain &TC, const ArgList &Args,
+                         ArgStringList &CmdArgs) {
+
+  unsigned ver = 0;
+  const VersionTuple vt = TC.computeMSVCVersion(nullptr, Args);
+  ver = vt.getMajor() * 10000000 + vt.getMinor().value_or(0) * 100000 +
+        vt.getSubminor().value_or(0);
+  CmdArgs.push_back(Args.MakeArgString("-D_MSC_VER=" + Twine(ver / 100000)));
+  CmdArgs.push_back(Args.MakeArgString("-D_MSC_FULL_VER=" + Twine(ver)));
+
+  llvm::Triple triple = TC.getTriple();
+  if (triple.isAArch64()) {
+    CmdArgs.push_back("-D_M_ARM64=1");
+  } else if (triple.isX86() && triple.isArch32Bit()) {
+    CmdArgs.push_back("-D_M_IX86=600");
+  } else if (triple.isX86() && triple.isArch64Bit()) {
+    CmdArgs.push_back("-D_M_X64=100");
+  } else {
+    llvm_unreachable(
+        "Flang on Windows only supports X86_32, X86_64 and AArch64");
+  }
+}
+
 static void processVSRuntimeLibrary(const ToolChain &TC, const ArgList &Args,
                                     ArgStringList &CmdArgs) {
   assert(TC.getTriple().isKnownWindowsMSVCEnvironment() &&
@@ -322,6 +345,7 @@ void Flang::addTargetOptions(const ArgList &Args,
 
   if (Triple.isKnownWindowsMSVCEnvironment()) {
     processVSRuntimeLibrary(TC, Args, CmdArgs);
+    addVSDefines(TC, Args, CmdArgs);
   }
 
   // TODO: Add target specific flags, ABI, mtune option etc.
diff --git a/flang/test/Driver/msvc-defines.f90 b/flang/test/Driver/msvc-defines.f90
new file mode 100644
index 000000000000000..97a6ce888d7f579
--- /dev/null
+++ b/flang/test/Driver/msvc-defines.f90
@@ -0,0 +1,10 @@
+! RUN: %flang -### --target=aarch64-windows-msvc %S/Inputs/hello.f90 -v 2>&1 | FileCheck %s --check-prefixes=MSVC,MSVC-AARCH64
+! RUN: %flang -### --target=i386-windows-msvc %S/Inputs/hello.f90 -v 2>&1 | FileCheck %s --check-prefixes=MSVC,MSVC-X86_32
+! RUN: %flang -### --target=x86_64-windows-msvc %S/Inputs/hello.f90 -v 2>&1 | FileCheck %s --check-prefixes=MSVC,MSVC-X86_64
+
+! MSVC: -fc1
+! MSVC-SAME: -D_MSC_VER={{[0-9]*}}
+! MSVC-SAME: -D_MSC_FULL_VER={{[0-9]*}}
+! MSVC-AARCH64-SAME: -D_M_ARM64=1
+! MSVC-X86_32-SAME: -D_M_IX86=600
+! MSVC-X86_64-SAME: -D_M_X64=100

llvmbot · 2023-11-23T15:37:22Z

@llvm/pr-subscribers-flang-driver

Author: David Truby (DavidTruby)

Changes

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

2 Files Affected:

(modified) clang/lib/Driver/ToolChains/Flang.cpp (+24)
(added) flang/test/Driver/msvc-defines.f90 (+10)

diff --git a/clang/lib/Driver/ToolChains/Flang.cpp b/clang/lib/Driver/ToolChains/Flang.cpp
index 01f07b9228256cf..87aa546e5549423 100644
--- a/clang/lib/Driver/ToolChains/Flang.cpp
+++ b/clang/lib/Driver/ToolChains/Flang.cpp
@@ -204,6 +204,29 @@ void Flang::AddAArch64TargetArgs(const ArgList &Args,
   }
 }
 
+static void addVSDefines(const ToolChain &TC, const ArgList &Args,
+                         ArgStringList &CmdArgs) {
+
+  unsigned ver = 0;
+  const VersionTuple vt = TC.computeMSVCVersion(nullptr, Args);
+  ver = vt.getMajor() * 10000000 + vt.getMinor().value_or(0) * 100000 +
+        vt.getSubminor().value_or(0);
+  CmdArgs.push_back(Args.MakeArgString("-D_MSC_VER=" + Twine(ver / 100000)));
+  CmdArgs.push_back(Args.MakeArgString("-D_MSC_FULL_VER=" + Twine(ver)));
+
+  llvm::Triple triple = TC.getTriple();
+  if (triple.isAArch64()) {
+    CmdArgs.push_back("-D_M_ARM64=1");
+  } else if (triple.isX86() && triple.isArch32Bit()) {
+    CmdArgs.push_back("-D_M_IX86=600");
+  } else if (triple.isX86() && triple.isArch64Bit()) {
+    CmdArgs.push_back("-D_M_X64=100");
+  } else {
+    llvm_unreachable(
+        "Flang on Windows only supports X86_32, X86_64 and AArch64");
+  }
+}
+
 static void processVSRuntimeLibrary(const ToolChain &TC, const ArgList &Args,
                                     ArgStringList &CmdArgs) {
   assert(TC.getTriple().isKnownWindowsMSVCEnvironment() &&
@@ -322,6 +345,7 @@ void Flang::addTargetOptions(const ArgList &Args,
 
   if (Triple.isKnownWindowsMSVCEnvironment()) {
     processVSRuntimeLibrary(TC, Args, CmdArgs);
+    addVSDefines(TC, Args, CmdArgs);
   }
 
   // TODO: Add target specific flags, ABI, mtune option etc.
diff --git a/flang/test/Driver/msvc-defines.f90 b/flang/test/Driver/msvc-defines.f90
new file mode 100644
index 000000000000000..97a6ce888d7f579
--- /dev/null
+++ b/flang/test/Driver/msvc-defines.f90
@@ -0,0 +1,10 @@
+! RUN: %flang -### --target=aarch64-windows-msvc %S/Inputs/hello.f90 -v 2>&1 | FileCheck %s --check-prefixes=MSVC,MSVC-AARCH64
+! RUN: %flang -### --target=i386-windows-msvc %S/Inputs/hello.f90 -v 2>&1 | FileCheck %s --check-prefixes=MSVC,MSVC-X86_32
+! RUN: %flang -### --target=x86_64-windows-msvc %S/Inputs/hello.f90 -v 2>&1 | FileCheck %s --check-prefixes=MSVC,MSVC-X86_64
+
+! MSVC: -fc1
+! MSVC-SAME: -D_MSC_VER={{[0-9]*}}
+! MSVC-SAME: -D_MSC_FULL_VER={{[0-9]*}}
+! MSVC-AARCH64-SAME: -D_M_ARM64=1
+! MSVC-X86_32-SAME: -D_M_IX86=600
+! MSVC-X86_64-SAME: -D_M_X64=100

DavidTruby · 2023-11-23T15:38:52Z

@pbo-linaro @bradking I think this fixes the linked issue, could someone confirm that for me? Thanks!

pbo-linaro · 2023-11-23T15:54:45Z

I'll let @bradking check this, as he implemented this.

MehdiChinoune · 2023-11-27T07:15:22Z

clang/lib/Driver/ToolChains/Flang.cpp

+  } else if (triple.isX86() && triple.isArch32Bit()) {
+    CmdArgs.push_back("-D_M_IX86=600");
+  } else if (triple.isX86() && triple.isArch64Bit()) {
+    CmdArgs.push_back("-D_M_X64=100");
+  } else {
+    llvm_unreachable(
+        "Flang on Windows only supports X86_32, X86_64 and AArch64");
+  }


Flang doesn't support 32bit

llvm-project/flang/CMakeLists.txt

Line 60 in 659e401

message(FATAL_ERROR "flang isn't supported on 32 bit CPUs")

We don't support 32 bit hosts but I think we do support 32 bit targets

bradking · 2023-11-27T18:36:19Z

flang/test/Driver/msvc-defines.f90

This file has CRLF newlines. Is that expected?

It wasn't, I think I have replaced this

banach-space · 2023-11-27T19:20:40Z

clang/lib/Driver/ToolChains/Flang.cpp

@@ -204,6 +204,29 @@ void Flang::AddAArch64TargetArgs(const ArgList &Args,
  }
 }

+static void addVSDefines(const ToolChain &TC, const ArgList &Args,


Suggested change

static void addVSDefines(const ToolChain &TC, const ArgList &Args,

static void addMSVCDefines(const ToolChain &TC, const ArgList &Args,

?

I named it this way to match processVSRuntimeLibs which I stole from clang :)

bradking · 2023-11-27T20:08:47Z

clang/lib/Driver/ToolChains/Flang.cpp

+
+  llvm::Triple triple = TC.getTriple();
+  if (triple.isAArch64()) {
+    CmdArgs.push_back("-D_M_ARM64=1");


Does LLVM/Flang support the _M_ARM64EC ABI?

I don't believe so. It's not tested at all if it does as far as I can tell!

bradking · 2023-11-27T20:23:51Z

clang/lib/Driver/ToolChains/Flang.cpp

@@ -322,6 +345,7 @@ void Flang::addTargetOptions(const ArgList &Args,

  if (Triple.isKnownWindowsMSVCEnvironment()) {
    processVSRuntimeLibrary(TC, Args, CmdArgs);
+    addVSDefines(TC, Args, CmdArgs);
  }

  // TODO: Add target specific flags, ABI, mtune option etc.


Please also add -D_WIN32 to indicate that this is targeting a Windows platform.

OTOH this could be considered out of scope for this PR.

bradking · 2023-11-30T16:55:36Z

@DavidTruby thanks. This LGTM now. I've locally updated CMake to use the preprocessor definitions in place of the workaround we had before, and it seems to work with this PR.

[flang] Add MSC_VER and target arch defines when targeting the MSVC ABI

656463c

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 Nov 23, 2023

DavidTruby requested review from mstorsjo and banach-space November 23, 2023 15:37

MehdiChinoune reviewed Nov 27, 2023

View reviewed changes

bradking reviewed Nov 27, 2023

View reviewed changes

banach-space reviewed Nov 27, 2023

View reviewed changes

bradking reviewed Nov 27, 2023

View reviewed changes

bradking mentioned this pull request Nov 27, 2023

[flang] Implement MSVC predefined macros for Windows #67675

Closed

Changes for review

50833e2

DavidTruby merged commit dff5bb9 into llvm:main Dec 4, 2023
3 checks passed

DavidTruby deleted the msvc-defines branch December 4, 2023 02:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[flang] Add MSC_VER and target arch defines when targeting the MSVC ABI #73250

[flang] Add MSC_VER and target arch defines when targeting the MSVC ABI #73250

DavidTruby commented Nov 23, 2023 •

edited

llvmbot commented Nov 23, 2023 •

edited

llvmbot commented Nov 23, 2023

DavidTruby commented Nov 23, 2023

pbo-linaro commented Nov 23, 2023

MehdiChinoune Nov 27, 2023

DavidTruby Nov 30, 2023

bradking Nov 27, 2023

DavidTruby Nov 30, 2023

banach-space Nov 27, 2023

DavidTruby Nov 30, 2023

bradking Nov 27, 2023

DavidTruby Nov 30, 2023

bradking Nov 27, 2023

bradking Nov 27, 2023

bradking commented Nov 30, 2023

	static void addVSDefines(const ToolChain &TC, const ArgList &Args,
	static void addMSVCDefines(const ToolChain &TC, const ArgList &Args,

[flang] Add MSC_VER and target arch defines when targeting the MSVC ABI #73250

[flang] Add MSC_VER and target arch defines when targeting the MSVC ABI #73250

Conversation

DavidTruby commented Nov 23, 2023 • edited

llvmbot commented Nov 23, 2023 • edited

llvmbot commented Nov 23, 2023

DavidTruby commented Nov 23, 2023

pbo-linaro commented Nov 23, 2023

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

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

bradking commented Nov 30, 2023

DavidTruby commented Nov 23, 2023 •

edited

llvmbot commented Nov 23, 2023 •

edited