[Flang][OpenMP] Fix HLFIR lowering for commonblock threadprivate

[kiranchandramohan]

Summary

Commonblock names are not variables, but they can be marked as
threadprivate in OpenMP. This requires the commonblock name to
be bound to the address of the Commonblock. hlfir.declares are
not required for these, but we should be able to retrieve the
mlir Value corresponding to the Commonblock. This patch enables
this by special casing the Commonblocks like procedures.

[kiranchandramohan]

The atlernative implementations are :
-> Have separate API functions in the converter for binding and retrieving mlir Values for non-variable symbols.
-> Making the hlfir variable op interface aware of commonblocks (This did not make sense since commonblocks are not variables).

[tblah]

LGTM, thanks!

[kiranchandramohan]

@vzakhari Is this OK with you?

[vzakhari]

Hi @kiranchandramohan! Yes, it looks okay to me. Thank you for the changes!

flang/lib/Lower/Bridge.cpp

if (lowerToHighLevelFIR()) {
             return symMap->lookupSymbol(sym);
 
       // Procedure dummies are not mapped with an hlfir.declare because
       // they are not "variable" (cannot be assigned to), and it would
       // make hlfir.declare more complex than it needs to to allow this.
       // Do a regular lookup.
       if (Fortran::semantics::IsProcedure(sym))
         return symMap->lookupSymbol(sym);
+
+      // Commonblock names are not variables, but in some lowerings (like
+      // OpenMP) it is useful to maintain the address of the commonblock in an
+      // MLIR value and query it. hlfir.declare need not be created for these.
+      if (sym.detailsIf<Fortran::semantics::CommonBlockDetails>())
+        return symMap->lookupSymbol(sym);
+
       return {};
     }
     if (Fortran::lower::SymbolBox v = symMap->lookupSymbol(sym))
       return v;
     return {};
   }
 
   /// Find the symbol in the inner-most level of the local map or return null.

flang/lib/Lower/ConvertVariable.cpp

static void genDeclareSymbol(Fortran::lower::AbstractConverter &converter,
                              llvm::ArrayRef<mlir::Value> shape = std::nullopt,
                              llvm::ArrayRef<mlir::Value> lbounds = std::nullopt,
                              bool force = false) {
   // In HLFIR, procedure dummy symbols are not added with an hlfir.declare
   // because they are "values", and hlfir.declare is intended for variables. It
   // would add too much complexity to hlfir.declare to support this case, and
   // this would bring very little (the only point being debug info, that are not
   // yet emitted) since alias analysis is meaningless for those.
+  // Commonblock names are not variables, but in some lowerings (like OpenMP) it
+  // is useful to maintain the address of the commonblock in an MLIR value and
+  // query it. hlfir.declare need not be created for these.
   if (converter.getLoweringOptions().getLowerToHighLevelFIR() &&
-      !Fortran::semantics::IsProcedure(sym)) {
+      !Fortran::semantics::IsProcedure(sym) &&
+      !sym.detailsIf<Fortran::semantics::CommonBlockDetails>()) {
     fir::FirOpBuilder &builder = converter.getFirOpBuilder();
     const mlir::Location loc = genLocation(converter, sym);
     mlir::Value shapeOrShift;
     if (!shape.empty() && !lbounds.empty())
       shapeOrShift = builder.genShape(loc, lbounds, shape);
     else if (!shape.empty())
       shapeOrShift = builder.genShape(loc, shape);
     else if (!lbounds.empty())

 
 /// Map a symbol to its FIR address and evaluated specification expressions
 /// provided as a fir::ExtendedValue. Will optionally create fir.declare.
 void Fortran::lower::genDeclareSymbol(
     Fortran::lower::AbstractConverter &converter,
     Fortran::lower::SymMap &symMap, const Fortran::semantics::Symbol &sym,
     const fir::ExtendedValue &exv, bool force) {
   if (converter.getLoweringOptions().getLowerToHighLevelFIR() &&
-      !Fortran::semantics::IsProcedure(sym)) {
+      !Fortran::semantics::IsProcedure(sym) &&
+      !sym.detailsIf<Fortran::semantics::CommonBlockDetails>()) {
     fir::FirOpBuilder &builder = converter.getFirOpBuilder();
     const mlir::Location loc = genLocation(converter, sym);
     fir::FortranVariableFlagsAttr attributes =
         Fortran::lower::translateSymbolAttributes(builder.getContext(), sym);
     auto name = converter.mangleName(sym);
     hlfir::EntityWithAttributes declare =
         hlfir::genDeclare(loc, builder, exv, name, attributes);
     symMap.addVariableDefinition(sym, declare.getIfVariableInterface(), force);

flang/test/Lower/OpenMP/threadprivate-common-block-hlfir.f90

+! Simple test for lowering of OpenMP Threadprivate Directive with HLFIR.
+! Test for common block.
+
+!RUN: %flang_fc1 -flang-experimental-hlfir -emit-hlfir -fopenmp %s -o - | FileCheck %s
+!RUN: bbc -hlfir -emit-hlfir -fopenmp %s -o - | FileCheck %s
+
+
+!CHECK: %[[CBLK_ADDR:.*]] = fir.address_of(@_QCblk) : !fir.ref<!fir.array<4xi8>>
+!CHECK: {{.*}} = omp.threadprivate %[[CBLK_ADDR]] : !fir.ref<!fir.array<4xi8>> -> !fir.ref<!fir.array<4xi8>>
+!CHECK: omp.parallel   {
+!CHECK:   %[[TP_PARALLEL:.*]] = omp.threadprivate %[[CBLK_ADDR]] : !fir.ref<!fir.array<4xi8>> -> !fir.ref<!fir.array<4xi8>>
+!CHECK:   %[[TP_PARALLEL_ADDR:.*]] = fir.convert %[[TP_PARALLEL]] : (!fir.ref<!fir.array<4xi8>>) -> !fir.ref<!fir.array<?xi8>>
+!CHECK:   %[[A_ADDR:.*]] = fir.coordinate_of %[[TP_PARALLEL_ADDR]], %c0_1 : (!fir.ref<!fir.array<?xi8>>, index) -> !fir.ref<i8>
+!CHECK:   %[[A_ADDR_CVT:.*]] = fir.convert %[[A_ADDR]] : (!fir.ref<i8>) -> !fir.ref<i32>
+!CHECK:   %[[A_DECL:.*]]:2 = hlfir.declare %[[A_ADDR_CVT]] {uniq_name = "_QFsub_commonblockEa"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK:   %[[A_VAL:.*]] = fir.load %[[A_DECL]]#0 : !fir.ref<i32>
+!CHECK:   {{.*}} = fir.call @_FortranAioOutputInteger32({{.*}}, %[[A_VAL]]) fastmath<contract> : (!fir.ref<i8>, i32) -> i1
+!CHECK:   omp.terminator
+!CHECK: }
+
+subroutine sub_commonblock()
+  integer:: a
+  common /blk/ a
+  !$omp threadprivate(/blk/)
+
+  !$omp parallel
+    print *, a
+  !$omp end parallel
+end