[flang] Add Win32 to the list of supported triples

[awarzynski]

Summary

This patch adds Win32 to the list of supported triples in
fir::CodeGenSpecifics. This change means that we can use the "native"
triple, even when running tests on Windows. Currently this affects only
1 test, but it will change once we start adding more tests for lowering
and code-generation.

[awarzynski]

This change is required for the unit test from https://reviews.llvm.org/D119012 to pass on Windows (it uses the "native" triple).

[schweitz]

Out of curiosity, how much has this been tested? I wouldn't be surprised if there are some gotcha differences between the Windows and Linux ABIs...

flang/lib/Optimizer/CodeGen/Target.cpp

241

[clementval]

Have you tried this patch on fir-dev?

[rovka]

flang/lib/Optimizer/CodeGen/Target.cpp

275–276

While you're at it, can you please also add it to AArch64? We can compile a hello world on Windows on AArch64 with something like this on top of fir-dev (I have it here; I haven't looked into ABI stuff yet but at least the basics are in place).

[awarzynski]

Out of curiosity, how much has this been tested?

Diana is experimenting on Windows. That's probably it at the moment.

I wouldn't be surprised if there are some gotcha differences between the Windows and Linux ABIs...

Possibly. But we won't be able to test at all unless this code-path is enabled, right?

Have you tried this patch on fir-dev?

Yes and everything worked fine for me. Though I only tested on Linux (not sure that matters though, most/all tests seem to use --target=x86_64-unknown-linux-gnu).

I didn't see anything that would be OS (or ABI) specific in KindMapping, which is what fir::CodeGenSpecifics::get effectively returns. To me this looks like a fairly safe change to make.

[awarzynski]

Add AArch64 support

flang/lib/Optimizer/CodeGen/Target.cpp

275–276

Done!

[clementval]

Have you tried this patch on fir-dev?

Yes and everything worked fine for me. Though I only tested on Linux (not sure that matters though, most/all tests seem to use --target=x86_64-unknown-linux-gnu).

I didn't see anything that would be OS (or ABI) specific in KindMapping, which is what fir::CodeGenSpecifics::get effectively returns. To me this looks like a fairly safe change to make.

On our side we do not run any test suites on windows. I'm just hoping this does not cause problem while we upstream the rest of fir-dev.

[awarzynski]

In D119332#3310553, @clementval wrote:

On our side we do not run any test suites on windows.

It sounds that you are very unlikely to hit this case then (i.e. this is always going to be true in your test/dev env: triple != -{}-{}-windows-win32). But we do have Windows pre-merge and post-merge testing, so it's not like this is not tested at all.

I'm just hoping this does not cause problem while we upstream the rest of fir-dev.

Upstreaming remains our top-priority. If this change becomes a blocker, we can revert it.

[rovka]

Thanks, this LGTM if everyone else is ok with it.

[clementval]

In D119332#3310757, @awarzynski wrote:

In D119332#3310553, @clementval wrote:

On our side we do not run any test suites on windows.

It sounds that you are very unlikely to hit this case then (i.e. this is always going to be true in your test/dev env: triple != -{}-{}-windows-win32). But we do have Windows pre-merge and post-merge testing, so it's not like this is not tested at all.

I'm just hoping this does not cause problem while we upstream the rest of fir-dev.

Upstreaming remains our top-priority. If this change becomes a blocker, we can revert it.

Ok for me.

[schweitz]

In D119332#3310504, @awarzynski wrote:

Have you tried this patch on fir-dev?

Yes and everything worked fine for me. Though I only tested on Linux (not sure that matters though, most/all tests seem to use --target=x86_64-unknown-linux-gnu).

I didn't see anything that would be OS (or ABI) specific in KindMapping, which is what fir::CodeGenSpecifics::get effectively returns. To me this looks like a fairly safe change to make.

I don't follow that.

The KindMap is not related to the target machine. It is effectively a mini-language to allow support for different dialects of Fortran that may use or reassign KIND values (in the source syntax) as a form of future-proofing the optimizer. (It's a source code thing.)

Code gen to a specific target can and does depend on what the target (triple) is from the user, the ABI(s) of that target, and how the LLVM machine code expects to see aspects of the LLVM IR presented to produce the correct calls, memory layouts, etc. flang has to follow in clang's footsteps in that regard as different LLVM backends expect different types, etc. (The target is a codegen thing.)

[awarzynski]

In D119332#3311785, @schweitz wrote:

I don't follow that.

The KindMap is not related to the target machine. It is effectively a mini-language to allow support for different dialects of Fortran that may use or reassign KIND values (in the source syntax) as a form of future-proofing the optimizer. (It's a source code thing.)

@schweitz, I referred to KindMap as that's one of the things that's returned by fir::CodeGenSpecifics::get. I think that we agree that it's rather OS-agnostic and in this respect, this change is safe.

Code gen to a specific target can and does depend on what the target (triple) is from the user, the ABI(s) of that target, and how the LLVM machine code expects to see aspects of the LLVM IR presented to produce the correct calls, memory layouts, etc. flang has to follow in clang's footsteps in that regard as different LLVM backends expect different types, etc. (The target is a codegen thing.)

Noted. This is currently expressed via e.g TargetI386 or TargetAArch64, right? These structs contain architecture-specific information, but I don't see anything OS-specific. For this reason, I feel that it is safe to enable Win32 here. This way:

• we can keep our tests more generic (i.e. avoid using --target when the generated code is target-agnostic),
• we avoid reducing the test coverage on Windows.

I think that we all agree that more work will be required to make sure that all ABIs work as expected. But we can re-visit this in due time and fine-tune things as required. For now, basic tests on Windows (and MacOSX, but that's on fir-dev) hit llvm::report_fatal_error unless the triple is hard-coded. But we run our CI on these platforms, fir::CodeGenSpecifics::get is OS-agnostic (am I missing something here?), so why not include Win32 (and MacOSX) here?

[schweitz]

LGTM. Thanks for verifying on Windows.

flang/lib/Optimizer/CodeGen/Target.cpp

complexReturnType(mlir::Type eleTy) const override {
     mlir::TypeRange range = {eleTy, eleTy};
     marshal.emplace_back(mlir::TupleType::get(eleTy.getContext(), range), AT{});
     return marshal;
   }
 };
 } // namespace
 
 // Instantiate the overloaded target instance based on the triple value.
-// Currently, the implementation only instantiates `i386-unknown-linux-gnu`,
-// `x86_64-unknown-linux-gnu`, aarch64 and ppc64le like triples. Other targets
-// should be added to this file as needed.
+// TODO: Add other targets to this file as needed.
 std::unique_ptr<fir::CodeGenSpecifics>
 fir::CodeGenSpecifics::get(mlir::MLIRContext *ctx, llvm::Triple &&trp,
                            KindMapping &&kindMap) {
   switch (trp.getArch()) {
   default:
     break;
   case llvm::Triple::ArchType::x86:
     switch (trp.getOS()) {
     default:
       break;
     case llvm::Triple::OSType::Linux:
     case llvm::Triple::OSType::Darwin:
+    case llvm::Triple::OSType::Win32:
       return std::make_unique<TargetI386>(ctx, std::move(trp),
                                           std::move(kindMap));
     }
     break;
   case llvm::Triple::ArchType::x86_64:
     switch (trp.getOS()) {
     default:
       break;
     case llvm::Triple::OSType::Linux:
     case llvm::Triple::OSType::Darwin:
+    case llvm::Triple::OSType::Win32:
       return std::make_unique<TargetX86_64>(ctx, std::move(trp),
                                             std::move(kindMap));
     }
     break;
   case llvm::Triple::ArchType::aarch64:
     switch (trp.getOS()) {
     default:
       break;
     case llvm::Triple::OSType::Linux:
     case llvm::Triple::OSType::Darwin:
+    case llvm::Triple::OSType::Win32:
       return std::make_unique<TargetAArch64>(ctx, std::move(trp),
                                              std::move(kindMap));
     }
     break;
   case llvm::Triple::ArchType::ppc64le:
     switch (trp.getOS()) {
     default:
       break;
     case llvm::Triple::OSType::Linux:
       return std::make_unique<TargetPPC64le>(ctx, std::move(trp),
                                              std::move(kindMap));
     }
     break;
   }
   llvm::report_fatal_error("target not implemented");
 }

flang/test/Fir/basic-program.fir

-// RUN: tco --target=x86_64-unknown-linux-gnu %s | FileCheck %s
+// RUN: tco %s | FileCheck %s
 
 // Check that tco is working with a basic test.
 
 func @_QQmain() {
   return
 }
 
 // CHECK: ; ModuleID = 'FIRModule'
 // CHECK-LABEL: define void @_QQmain()
 // CHECK:       ret void