Skip to content

Conversation

@clementval
Copy link
Contributor

In case where a fir.global might be duplicated in an inner module (gpu.module), the conversion pattern will be applied on the module and the gpu module version of the global and try to generate multiple comdat with the same symbol name. This is what we have in the implementation of CUDA Fortran.

Just check for the presence of the ComdatSelectorOp before creating a new one.

@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir flang:codegen labels Oct 31, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 31, 2024

@llvm/pr-subscribers-flang-codegen

Author: Valentin Clement (バレンタイン クレメン) (clementval)

Changes

In case where a fir.global might be duplicated in an inner module (gpu.module), the conversion pattern will be applied on the module and the gpu module version of the global and try to generate multiple comdat with the same symbol name. This is what we have in the implementation of CUDA Fortran.

Just check for the presence of the ComdatSelectorOp before creating a new one.


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

2 Files Affected:

  • (modified) flang/lib/Optimizer/CodeGen/CodeGen.cpp (+3)
  • (added) flang/test/Fir/comdat-present.fir (+14)
diff --git a/flang/lib/Optimizer/CodeGen/CodeGen.cpp b/flang/lib/Optimizer/CodeGen/CodeGen.cpp
index 4c8c56e0f21cef..d038efcb2eb42c 100644
--- a/flang/lib/Optimizer/CodeGen/CodeGen.cpp
+++ b/flang/lib/Optimizer/CodeGen/CodeGen.cpp
@@ -2931,6 +2931,9 @@ struct GlobalOpConversion : public fir::FIROpConversion<fir::GlobalOp> {
       comdatOp =
           rewriter.create<mlir::LLVM::ComdatOp>(module.getLoc(), comdatName);
     }
+    if (auto select = comdatOp.lookupSymbol<mlir::LLVM::ComdatSelectorOp>(
+            global.getSymName()))
+      return;
     mlir::OpBuilder::InsertionGuard guard(rewriter);
     rewriter.setInsertionPointToEnd(&comdatOp.getBody().back());
     auto selectorOp = rewriter.create<mlir::LLVM::ComdatSelectorOp>(
diff --git a/flang/test/Fir/comdat-present.fir b/flang/test/Fir/comdat-present.fir
new file mode 100644
index 00000000000000..96d14e5973f4f7
--- /dev/null
+++ b/flang/test/Fir/comdat-present.fir
@@ -0,0 +1,14 @@
+// RUN: fir-opt %s --fir-to-llvm-ir="target=x86_64-unknown-linux-gnu" | FileCheck %s
+// RUN: fir-opt %s --fir-to-llvm-ir="target=x86_64-pc-windows-msvc" | FileCheck %s
+
+fir.global linkonce_odr @global_linkonce_odr constant : i32 {
+  %0 = arith.constant 0 : i32
+  fir.has_value %0 : i32
+}
+
+llvm.comdat @__llvm_comdat {
+  llvm.comdat_selector @global_linkonce_odr any
+}
+
+// CHECK-LABEL: llvm.comdat @__llvm_comdat
+// CHECK: llvm.comdat_selector @global_linkonce_odr any

@llvmbot
Copy link
Member

llvmbot commented Oct 31, 2024

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

Author: Valentin Clement (バレンタイン クレメン) (clementval)

Changes

In case where a fir.global might be duplicated in an inner module (gpu.module), the conversion pattern will be applied on the module and the gpu module version of the global and try to generate multiple comdat with the same symbol name. This is what we have in the implementation of CUDA Fortran.

Just check for the presence of the ComdatSelectorOp before creating a new one.


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

2 Files Affected:

  • (modified) flang/lib/Optimizer/CodeGen/CodeGen.cpp (+3)
  • (added) flang/test/Fir/comdat-present.fir (+14)
diff --git a/flang/lib/Optimizer/CodeGen/CodeGen.cpp b/flang/lib/Optimizer/CodeGen/CodeGen.cpp
index 4c8c56e0f21cef..d038efcb2eb42c 100644
--- a/flang/lib/Optimizer/CodeGen/CodeGen.cpp
+++ b/flang/lib/Optimizer/CodeGen/CodeGen.cpp
@@ -2931,6 +2931,9 @@ struct GlobalOpConversion : public fir::FIROpConversion<fir::GlobalOp> {
       comdatOp =
           rewriter.create<mlir::LLVM::ComdatOp>(module.getLoc(), comdatName);
     }
+    if (auto select = comdatOp.lookupSymbol<mlir::LLVM::ComdatSelectorOp>(
+            global.getSymName()))
+      return;
     mlir::OpBuilder::InsertionGuard guard(rewriter);
     rewriter.setInsertionPointToEnd(&comdatOp.getBody().back());
     auto selectorOp = rewriter.create<mlir::LLVM::ComdatSelectorOp>(
diff --git a/flang/test/Fir/comdat-present.fir b/flang/test/Fir/comdat-present.fir
new file mode 100644
index 00000000000000..96d14e5973f4f7
--- /dev/null
+++ b/flang/test/Fir/comdat-present.fir
@@ -0,0 +1,14 @@
+// RUN: fir-opt %s --fir-to-llvm-ir="target=x86_64-unknown-linux-gnu" | FileCheck %s
+// RUN: fir-opt %s --fir-to-llvm-ir="target=x86_64-pc-windows-msvc" | FileCheck %s
+
+fir.global linkonce_odr @global_linkonce_odr constant : i32 {
+  %0 = arith.constant 0 : i32
+  fir.has_value %0 : i32
+}
+
+llvm.comdat @__llvm_comdat {
+  llvm.comdat_selector @global_linkonce_odr any
+}
+
+// CHECK-LABEL: llvm.comdat @__llvm_comdat
+// CHECK: llvm.comdat_selector @global_linkonce_odr any

Copy link
Contributor

@Renaud-K Renaud-K left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

@clementval clementval merged commit 466b58b into llvm:main Nov 1, 2024
12 checks passed
@clementval clementval deleted the comdat_duplicate_sym branch November 1, 2024 01:59
smallp-o-p pushed a commit to smallp-o-p/llvm-project that referenced this pull request Nov 3, 2024
In case where a fir.global might be duplicated in an inner module
(gpu.module), the conversion pattern will be applied on the module and
the gpu module version of the global and try to generate multiple comdat
with the same symbol name. This is what we have in the implementation of
CUDA Fortran.

Just check for the presence of the `ComdatSelectorOp` before creating a
new one.
NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
In case where a fir.global might be duplicated in an inner module
(gpu.module), the conversion pattern will be applied on the module and
the gpu module version of the global and try to generate multiple comdat
with the same symbol name. This is what we have in the implementation of
CUDA Fortran.

Just check for the presence of the `ComdatSelectorOp` before creating a
new one.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

flang:codegen flang:fir-hlfir flang Flang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants