Readability improvments

Author: tblah

flang/lib/Lower/OpenMP/DataSharingProcessor.cpp

@@ -168,7 +168,7 @@ void DataSharingProcessor::cloneSymbol(const semantics::Symbol *sym) {
 
   if (needInitClone()) {
     Fortran::lower::initializeCloneAtRuntime(converter, *sym, symTable);
-    mightHaveReadMoldArg = true;
+    mightHaveReadHostSym = true;
   }
 }
 
@@ -221,7 +221,7 @@ bool DataSharingProcessor::needBarrier() {
   for (const semantics::Symbol *sym : allPrivatizedSymbols) {
     if (sym->test(semantics::Symbol::Flag::OmpLastPrivate) &&
         (sym->test(semantics::Symbol::Flag::OmpFirstPrivate) ||
-         mightHaveReadMoldArg))
+         mightHaveReadHostSym))
       return true;
   }
   return false;
@@ -505,17 +505,15 @@ void DataSharingProcessor::doPrivatize(const semantics::Symbol *sym,
   lower::SymbolBox hsb = converter.lookupOneLevelUpSymbol(*sym);
   assert(hsb && "Host symbol box not found");
 
-  mlir::Value privVal = hsb.getAddr();
-  mlir::Type allocType;
-  if (mlir::isa<fir::PointerType>(privVal.getType()))
-    allocType = privVal.getType();
-  else
-    allocType = fir::unwrapRefType(privVal.getType());
-
   mlir::Location symLoc = hsb.getAddr().getLoc();
   std::string privatizerName = sym->name().ToString() + ".privatizer";
   bool isFirstPrivate = sym->test(semantics::Symbol::Flag::OmpFirstPrivate);
 
+  mlir::Value privVal = hsb.getAddr();
+  mlir::Type allocType = privVal.getType();
+  if (!mlir::isa<fir::PointerType>(privVal.getType()))
+    allocType = fir::unwrapRefType(privVal.getType());
+
   if (auto poly = mlir::dyn_cast<fir::ClassType>(allocType)) {
     if (!mlir::isa<fir::PointerType>(poly.getEleTy()) && isFirstPrivate)
       TODO(symLoc, "create polymorphic host associated copy");
@@ -566,6 +564,12 @@ void DataSharingProcessor::doPrivatize(const semantics::Symbol *sym,
     lower::SymMapScope outerScope(symTable);
 
     // Populate the `init` region.
+    // We need to initialize in the following cases:
+    // 1. The allocation was for a derived type which requires initialization
+    //    (this can be skipped if it will be initialized anyway by the copy
+    //    region, unless the derived type has allocatable components)
+    // 2. The allocation was for any kind of box
+    // 3. The allocation was for a boxed character
     const bool needsInitialization =
         (Fortran::lower::hasDefaultInitialization(sym->GetUltimate()) &&
          (!isFirstPrivate || hlfir::mayHaveAllocatableComponent(allocType))) ||
@@ -586,7 +590,7 @@ void DataSharingProcessor::doPrivatize(const semantics::Symbol *sym,
       // TODO: currently there are false positives from dead uses of the mold
       // arg
       if (!result.getInitMoldArg().getUses().empty())
-        mightHaveReadMoldArg = true;
+        mightHaveReadHostSym = true;
     }
 
     // Populate the `copy` region if this is a `firstprivate`.

flang/lib/Lower/OpenMP/DataSharingProcessor.h

@@ -86,7 +86,7 @@ class DataSharingProcessor {
   lower::pft::Evaluation &eval;
   bool shouldCollectPreDeterminedSymbols;
   bool useDelayedPrivatization;
-  bool mightHaveReadMoldArg = false;
+  bool mightHaveReadHostSym = false;
   lower::SymMap &symTable;
   OMPConstructSymbolVisitor visitor;
 

flang/lib/Lower/OpenMP/PrivateReductionUtils.cpp

@@ -160,6 +160,7 @@ static void initializeIfDerivedTypeBox(fir::FirOpBuilder &builder,
                                        mlir::Location loc, mlir::Value newBox,
                                        mlir::Value moldBox, bool hasInitializer,
                                        bool isFirstPrivate) {
+  assert(moldBox.getType() == newBox.getType());
   fir::BoxType boxTy = mlir::dyn_cast<fir::BoxType>(newBox.getType());
   fir::ClassType classTy = mlir::dyn_cast<fir::ClassType>(newBox.getType());
   if (!boxTy && !classTy)
@@ -177,7 +178,6 @@ static void initializeIfDerivedTypeBox(fir::FirOpBuilder &builder,
 
   if (!fir::isa_derived(derivedTy))
     return;
-  assert(moldBox.getType() == newBox.getType());
 
   if (hasInitializer)
     fir::runtime::genDerivedTypeInitialize(builder, loc, newBox);
@@ -218,17 +218,16 @@ isDerivedTypeNeedingInitialization(const Fortran::semantics::Symbol &sym) {
   if (const Fortran::semantics::DeclTypeSpec *declTypeSpec = sym.GetType())
     if (const Fortran::semantics::DerivedTypeSpec *derivedTypeSpec =
             declTypeSpec->AsDerived())
-      if (derivedTypeSpec->HasDefaultInitialization(
-              /*ignoreAllocatable=*/false, /*ignorePointer=*/true))
-        return true;
+      return derivedTypeSpec->HasDefaultInitialization(
+          /*ignoreAllocatable=*/false, /*ignorePointer=*/true);
   return false;
 }
 
 static mlir::Value generateZeroShapeForRank(fir::FirOpBuilder &builder,
                                             mlir::Location loc,
                                             mlir::Value moldArg) {
-  mlir::Type moldVal = fir::unwrapRefType(moldArg.getType());
-  mlir::Type eleType = fir::dyn_cast_ptrOrBoxEleTy(moldVal);
+  mlir::Type moldType = fir::unwrapRefType(moldArg.getType());
+  mlir::Type eleType = fir::dyn_cast_ptrOrBoxEleTy(moldType);
   fir::SequenceType seqTy =
       mlir::dyn_cast_if_present<fir::SequenceType>(eleType);
   if (!seqTy)
@@ -324,13 +323,8 @@ void Fortran::lower::omp::populateByRefInitAndCleanupRegions(
       // Just incase, do initialize the box with a null value
       mlir::Value null = builder.createNullConstant(loc, boxTy.getEleTy());
       mlir::Value nullBox;
-      if (shape)
-        nullBox = builder.create<fir::EmboxOp>(
-            loc, boxTy, null, shape, /*slice=*/mlir::Value{}, lenParams);
-      else
-        nullBox = builder.create<fir::EmboxOp>(
-            loc, boxTy, null, /*shape=*/mlir::Value{}, /*slice=*/mlir::Value{},
-            lenParams);
+      nullBox = builder.create<fir::EmboxOp>(
+          loc, boxTy, null, shape, /*slice=*/mlir::Value{}, lenParams);
       builder.create<fir::StoreOp>(loc, nullBox, boxAlloca);
       yield(boxAlloca);
       return;

flang/lib/Lower/OpenMP/PrivateReductionUtils.h

@@ -48,8 +48,8 @@ inline bool isReduction(DeclOperationKind kind) {
 
 /// Generate init and cleanup regions suitable for reduction or privatizer
 /// declarations. `scalarInitValue` may be nullptr if there is no default
-/// initialization (for privatization). If this is for a privatizer, set
-/// `isPrivate` to `true`.
+/// initialization (for privatization). `kind` should be set to indicate
+/// what kind of operation definition this initialization belongs to.
 void populateByRefInitAndCleanupRegions(
     AbstractConverter &converter, mlir::Location loc, mlir::Type argType,
     mlir::Value scalarInitValue, mlir::Block *initBlock,

flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp

@@ -65,7 +65,7 @@ class MapsForPrivatizedSymbolsPass
     // decalred needs a descriptor.
     // Some types are boxed immediately before privatization. These have other
     // operations in between the privatization and the declaration. It is safe
-    // to use var directly here because they will be boxed anyay.
+    // to use var directly here because they will be boxed anyway.
     if (auto declOp = llvm::dyn_cast<hlfir::DeclareOp>(definingOp))
       varPtr = declOp.getBase();
 

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

@@ -64,11 +64,17 @@ def PrivateClauseOp : OpenMP_Op<"private", [IsolatedFromAbove, RecipeInterface]>
     ```mlir
     omp.private {type = private} @x.privatizer : !some.type init {
     ^bb0(%arg0: !some.pointer<!some.type>, %arg1: !some.pointer<!some.type>):
-    // initialize %arg1, using %arg0 as a mold for allocations
+    // initialize %arg1, using %arg0 as a mold for allocations.
+    // For example if %arg0 is a heap allocated array with a runtime determined
+    // length and !some.type is a runtime type descriptor, the init region
+    // will read the array length from %arg0, and heap allocate an array of the
+    // right length and initialize %arg1 to contain the array allocation and
+    // length.
     omp.yield(%arg1 : !some.pointer<!some.type>)
     } dealloc {
     ^bb0(%arg0: !some.pointer<!some.type>):
-    ... deallocate allocated memory ...
+    // ... deallocate memory allocated by the init region...
+    // In the example above, this will free the heap allocated array data.
     omp.yield
     }
     ```