[flang-commits] [flang] [flang] Move `genCommonBlockMember` from OpenMP to ConvertVariable, NFC (PR #74488)

Krzysztof Parzyszek via flang-commits flang-commits at lists.llvm.org
Wed Dec 6 06:23:51 PST 2023


https://github.com/kparzysz updated https://github.com/llvm/llvm-project/pull/74488

>From 335cb4ee9b3179e05ec277b96b39aa61859a1bcc Mon Sep 17 00:00:00 2001
From: Krzysztof Parzyszek <Krzysztof.Parzyszek at amd.com>
Date: Tue, 5 Dec 2023 09:37:57 -0600
Subject: [PATCH 1/2] [flang] Move `genCommonBlockMember` from OpenMP to
 ConvertVariable, NFC

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.
---
 flang/include/flang/Lower/ConvertVariable.h | 18 +++++++++++--
 flang/lib/Lower/ConvertVariable.cpp         | 21 +++++++++++++++
 flang/lib/Lower/OpenMP.cpp                  | 29 ++-------------------
 3 files changed, 39 insertions(+), 29 deletions(-)

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);
     }

>From b990900194504336fb1fd46dc0a4f4a83d56d584 Mon Sep 17 00:00:00 2001
From: Krzysztof Parzyszek <Krzysztof.Parzyszek at amd.com>
Date: Tue, 5 Dec 2023 11:52:50 -0600
Subject: [PATCH 2/2] Call `getCommonBlockMember` from `instantiateCommon`

This avoids code duplication, and addresses an outstanding FIXME.
---
 flang/include/flang/Lower/ConvertVariable.h |  1 +
 flang/lib/Lower/ConvertVariable.cpp         | 48 ++++++++-------------
 flang/lib/Lower/OpenMP.cpp                  |  6 +--
 3 files changed, 22 insertions(+), 33 deletions(-)

diff --git a/flang/include/flang/Lower/ConvertVariable.h b/flang/include/flang/Lower/ConvertVariable.h
index 970353b5fed93..0ff3ca9bdeac7 100644
--- a/flang/include/flang/Lower/ConvertVariable.h
+++ b/flang/include/flang/Lower/ConvertVariable.h
@@ -76,6 +76,7 @@ void defineCommonBlocks(
 /// 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,
+                                 mlir::Location loc,
                                  const Fortran::semantics::Symbol &sym,
                                  mlir::Value commonValue);
 
diff --git a/flang/lib/Lower/ConvertVariable.cpp b/flang/lib/Lower/ConvertVariable.cpp
index b36a8a0caa1f0..676fecdb52a8b 100644
--- a/flang/lib/Lower/ConvertVariable.cpp
+++ b/flang/lib/Lower/ConvertVariable.cpp
@@ -1331,25 +1331,26 @@ 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,
+    Fortran::lower::AbstractConverter &converter, mlir::Location loc,
     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);
+  fir::FirOpBuilder &builder = converter.getFirOpBuilder();
+
   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::IntegerType i8Ty = builder.getIntegerType(8);
+  mlir::Type i8Ptr = builder.getRefType(i8Ty);
+  mlir::Type seqTy = builder.getRefType(builder.getVarLenSeqTy(i8Ty));
+  mlir::Value base = builder.createConvert(loc, seqTy, commonValue);
+
+  mlir::Value offs =
+      builder.createIntegerConstant(loc, builder.getIndexType(), byteOffset);
+  mlir::Value varAddr = builder.create<fir::CoordinateOp>(
+      loc, i8Ptr, base, mlir::ValueRange{offs});
   mlir::Type symType = converter.genType(sym);
-  return firOpBuilder.createConvert(currentLocation,
-                                    firOpBuilder.getRefType(symType), varAddr);
+
+  return Fortran::semantics::FindEquivalenceSet(sym) != nullptr
+             ? castAliasToPointer(builder, loc, symType, varAddr)
+             : builder.createConvert(loc, builder.getRefType(symType), varAddr);
 }
 
 /// The COMMON block is a global structure. `var` will be at some offset
@@ -1374,21 +1375,8 @@ static void instantiateCommon(Fortran::lower::AbstractConverter &converter,
 
     symMap.addSymbol(common, commonAddr);
   }
-  std::size_t byteOffset = varSym.GetUltimate().offset();
-  mlir::IntegerType i8Ty = builder.getIntegerType(8);
-  mlir::Type i8Ptr = builder.getRefType(i8Ty);
-  mlir::Type seqTy = builder.getRefType(builder.getVarLenSeqTy(i8Ty));
-  mlir::Value base = builder.createConvert(loc, seqTy, commonAddr);
-  mlir::Value offs =
-      builder.createIntegerConstant(loc, builder.getIndexType(), byteOffset);
-  auto varAddr = builder.create<fir::CoordinateOp>(loc, i8Ptr, base,
-                                                   mlir::ValueRange{offs});
-  mlir::Type symType = converter.genType(var.getSymbol());
-  mlir::Value local;
-  if (Fortran::semantics::FindEquivalenceSet(var.getSymbol()) != nullptr)
-    local = castAliasToPointer(builder, loc, symType, varAddr);
-  else
-    local = builder.createConvert(loc, builder.getRefType(symType), varAddr);
+
+  mlir::Value local = genCommonBlockMember(converter, loc, varSym, commonAddr);
   Fortran::lower::StatementContext stmtCtx;
   mapSymbolAttributes(converter, var, symMap, stmtCtx, local);
 }
diff --git a/flang/lib/Lower/OpenMP.cpp b/flang/lib/Lower/OpenMP.cpp
index 9906bcff548e8..0fa1ac76d57ed 100644
--- a/flang/lib/Lower/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP.cpp
@@ -2025,7 +2025,7 @@ static void threadPrivatizeVars(Fortran::lower::AbstractConverter &converter,
         commonSyms.insert(common);
       }
       symThreadprivateValue = Fortran::lower::genCommonBlockMember(
-          converter, *sym, commonThreadprivateValue);
+          converter, currentLocation, *sym, commonThreadprivateValue);
     } else {
       symThreadprivateValue = genThreadprivateOp(*sym);
     }
@@ -3529,8 +3529,8 @@ void Fortran::lower::genThreadprivateOp(
             currentLocation, commonValue.getType(), commonValue);
     converter.bindSymbol(*common, commonThreadprivateValue);
     // Generate the threadprivate value for the common block member.
-    symThreadprivateValue =
-        genCommonBlockMember(converter, sym, commonThreadprivateValue);
+    symThreadprivateValue = genCommonBlockMember(converter, currentLocation,
+                                                 sym, commonThreadprivateValue);
   } else if (!var.isGlobal()) {
     // Non-global variable which can be in threadprivate directive must be one
     // variable in main program, and it has implicit SAVE attribute. Take it as



More information about the flang-commits mailing list