[flang] Move genCommonBlockMember from OpenMP to ConvertVariable, NFC

[kparzysz]

The function genCommonBlockMember is not specific to OpenMP, and it could very well be a common utility. Move it to ConvertVariable.cpp where it logically belongs.

[llvmbot]

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

@llvm/pr-subscribers-flang-openmp

Author: Krzysztof Parzyszek (kparzysz)

Changes

The function genCommonBlockMember is not specific to OpenMP, and it could very well be a common utility. Move it to ConvertVariable.cpp where it logically belongs.

---

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

3 Files Affected:

• (modified) flang/include/flang/Lower/ConvertVariable.h (+16-2)
• (modified) flang/lib/Lower/ConvertVariable.cpp (+21)
• (modified) flang/lib/Lower/OpenMP.cpp (+2-27)

diff --git a/flang/include/flang/Lower/ConvertVariable.h b/flang/include/flang/Lower/ConvertVariable.h
index 7da04fea35167..970353b5fed93 100644
--- a/flang/include/flang/Lower/ConvertVariable.h
+++ b/flang/include/flang/Lower/ConvertVariable.h
@@ -19,6 +19,7 @@
 
 #include "flang/Lower/Support/Utils.h"
 #include "flang/Optimizer/Dialect/FIRAttr.h"
+#include "flang/Semantics/symbol.h"
 #include "mlir/IR/Value.h"
 #include "llvm/ADT/DenseMap.h"
 
@@ -29,7 +30,12 @@ class GlobalOp;
 class FortranVariableFlagsAttr;
 } // namespace fir
 
-namespace Fortran ::lower {
+namespace Fortran {
+namespace semantics {
+class Scope;
+} // namespace semantics
+
+namespace lower {
 class AbstractConverter;
 class CallerInterface;
 class StatementContext;
@@ -66,6 +72,13 @@ void defineCommonBlocks(
     const std::vector<std::pair<semantics::SymbolRef, std::size_t>>
         &commonBlocks);
 
+/// The COMMON block is a global structure. \p commonValue is the base address
+/// of the COMMON block. As the offset from the symbol \p sym, generate the
+/// COMMON block member value (commonValue + offset) for the symbol.
+mlir::Value genCommonBlockMember(AbstractConverter &converter,
+                                 const Fortran::semantics::Symbol &sym,
+                                 mlir::Value commonValue);
+
 /// Lower a symbol attributes given an optional storage \p and add it to the
 /// provided symbol map. If \preAlloc is not provided, a temporary storage will
 /// be allocated. This is a low level function that should only be used if
@@ -138,5 +151,6 @@ void genDeclareSymbol(Fortran::lower::AbstractConverter &converter,
 /// Cray pointer symbol. Assert if the pointer symbol cannot be found.
 Fortran::semantics::SymbolRef getCrayPointer(Fortran::semantics::SymbolRef sym);
 
-} // namespace Fortran::lower
+} // namespace lower
+} // namespace Fortran
 #endif // FORTRAN_LOWER_CONVERT_VARIABLE_H
diff --git a/flang/lib/Lower/ConvertVariable.cpp b/flang/lib/Lower/ConvertVariable.cpp
index 7bdb501e757cc..b36a8a0caa1f0 100644
--- a/flang/lib/Lower/ConvertVariable.cpp
+++ b/flang/lib/Lower/ConvertVariable.cpp
@@ -1331,6 +1331,27 @@ void Fortran::lower::defineCommonBlocks(
     finalizeCommonBlockDefinition(loc, converter, global, cmnBlkMems);
 }
 
+/// FIXME: Share the code with `instantiateCommon` in ConvertVariable.cpp.
+mlir::Value Fortran::lower::genCommonBlockMember(
+    Fortran::lower::AbstractConverter &converter,
+    const Fortran::semantics::Symbol &sym, mlir::Value commonValue) {
+  fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
+  mlir::Location currentLocation = converter.getCurrentLocation();
+  mlir::IntegerType i8Ty = firOpBuilder.getIntegerType(8);
+  mlir::Type i8Ptr = firOpBuilder.getRefType(i8Ty);
+  mlir::Type seqTy = firOpBuilder.getRefType(firOpBuilder.getVarLenSeqTy(i8Ty));
+  mlir::Value base =
+      firOpBuilder.createConvert(currentLocation, seqTy, commonValue);
+  std::size_t byteOffset = sym.GetUltimate().offset();
+  mlir::Value offs = firOpBuilder.createIntegerConstant(
+      currentLocation, firOpBuilder.getIndexType(), byteOffset);
+  mlir::Value varAddr = firOpBuilder.create<fir::CoordinateOp>(
+      currentLocation, i8Ptr, base, mlir::ValueRange{offs});
+  mlir::Type symType = converter.genType(sym);
+  return firOpBuilder.createConvert(currentLocation,
+                                    firOpBuilder.getRefType(symType), varAddr);
+}
+
 /// The COMMON block is a global structure. `var` will be at some offset
 /// within the COMMON block. Adds the address of `var` (COMMON + offset) to
 /// the symbol map.
diff --git a/flang/lib/Lower/OpenMP.cpp b/flang/lib/Lower/OpenMP.cpp
index adbc277d6b019..9906bcff548e8 100644
--- a/flang/lib/Lower/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP.cpp
@@ -1959,31 +1959,6 @@ static mlir::Operation *getCompareFromReductionOp(mlir::Operation *reductionOp,
   return nullptr;
 }
 
-/// The COMMON block is a global structure. \p commonValue is the base address
-/// of the COMMON block. As the offset from the symbol \p sym, generate the
-/// COMMON block member value (commonValue + offset) for the symbol.
-/// FIXME: Share the code with `instantiateCommon` in ConvertVariable.cpp.
-static mlir::Value
-genCommonBlockMember(Fortran::lower::AbstractConverter &converter,
-                     const Fortran::semantics::Symbol &sym,
-                     mlir::Value commonValue) {
-  fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
-  mlir::Location currentLocation = converter.getCurrentLocation();
-  mlir::IntegerType i8Ty = firOpBuilder.getIntegerType(8);
-  mlir::Type i8Ptr = firOpBuilder.getRefType(i8Ty);
-  mlir::Type seqTy = firOpBuilder.getRefType(firOpBuilder.getVarLenSeqTy(i8Ty));
-  mlir::Value base =
-      firOpBuilder.createConvert(currentLocation, seqTy, commonValue);
-  std::size_t byteOffset = sym.GetUltimate().offset();
-  mlir::Value offs = firOpBuilder.createIntegerConstant(
-      currentLocation, firOpBuilder.getIndexType(), byteOffset);
-  mlir::Value varAddr = firOpBuilder.create<fir::CoordinateOp>(
-      currentLocation, i8Ptr, base, mlir::ValueRange{offs});
-  mlir::Type symType = converter.genType(sym);
-  return firOpBuilder.createConvert(currentLocation,
-                                    firOpBuilder.getRefType(symType), varAddr);
-}
-
 // Get the extended value for \p val by extracting additional variable
 // information from \p base.
 static fir::ExtendedValue getExtendedValue(fir::ExtendedValue base,
@@ -2049,8 +2024,8 @@ static void threadPrivatizeVars(Fortran::lower::AbstractConverter &converter,
         converter.bindSymbol(*common, commonThreadprivateValue);
         commonSyms.insert(common);
       }
-      symThreadprivateValue =
-          genCommonBlockMember(converter, *sym, commonThreadprivateValue);
+      symThreadprivateValue = Fortran::lower::genCommonBlockMember(
+          converter, *sym, commonThreadprivateValue);
     } else {
       symThreadprivateValue = genThreadprivateOp(*sym);
     }

[clementval]

If there is no use of it elsewhere it can stay in OpenMP.cpp

[kiranchandramohan]

If you can address the FIXME that would be a good reason to move the code.

[kparzysz]

The build failures are caused by lit on Windows not being able to open four of the test files, e.g.

OSError: [Errno 22] Invalid argument: 'C:\\ws\\src\\flang\\test\\Lower\\OpenMP\\FIR\\declare-target-implicit-func-and-subr-cap-enter.f90'

[kiranchandramohan]

Thanks @kparzysz for this cleanup. LGTM. Please wait for either @clementval or @jeanPerier.

[kiranchandramohan]

The build failures are caused by lit on Windows not being able to open four of the test files, e.g.

OSError: [Errno 22] Invalid argument: 'C:\\ws\\src\\flang\\test\\Lower\\OpenMP\\FIR\\declare-target-implicit-func-and-subr-cap-enter.f90'

Would a rebase solve the issue?

FYI @shraiysh.

[jeanPerier]

LGTM

[kparzysz]

Rebased.