[OpenMP][OMPIRBuilder] Mirgrate getName from clang to OMPIRBuilder

[jsjodin]

Summary

This change moves the getName function from clang and moves the separator class
members from CGOpenMPRuntime into OpenMPIRBuilder.

There is a question if we want this kind of state in the OpenMPIRBuilder, there
are other parameters that could be class members like IsEmbedded (IsDevice),
IsTargetCodegen etc. These are currently derived from the CodeGenModule class in clang.
Moving more of the code generation into OpenMPIRBuilder requires knowledge of these parameters,
which have so far been passed in as arguments to the functions, but the number of parameters
might become too many to pass around all the time. There is a case where these parameters should
be made part of the OMPIRBuilder, and used in CGOpenMPRuntime.

[jdoerfert]

Maybe we want a configuration object that is populated once by the user and passed to the OMPIRBuilder to cluster all such configurations, e.g., device, embedded, etc. We can also choose separators based on that internally.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

182

Rename to something meaningful.

[jsjodin]

In D137725#3919652, @jdoerfert wrote:

Maybe we want a configuration object that is populated once by the user and passed to the OMPIRBuilder to cluster all such configurations, e.g., device, embedded, etc. We can also choose separators based on that internally.

Yes, I think that sounds good. Formalizing the attributes that affect codgen (in a configuration object) should make the code easier to understand/maintain. I can start working on a patch to add the configuration object.

[jsjodin]

In D137725#3921567, @jsjodin wrote:

In D137725#3919652, @jdoerfert wrote:

Maybe we want a configuration object that is populated once by the user and passed to the OMPIRBuilder to cluster all such configurations, e.g., device, embedded, etc. We can also choose separators based on that internally.

Yes, I think that sounds good. Formalizing the attributes that affect codgen (in a configuration object) should make the code easier to understand/maintain. I can start working on a patch to add the configuration object.

After looking a bit more closely into this, there are a few factors to consider. There are uses of the OpenMPIRBuilder, for example in mlir with the OpenACC frontend (it uses the OpenMPIRBuilder for some reason) and in the OMP optimizer, where I do not know if the configuration makes sense. The OffloadEntriesInfoManager is dependent on these flags, which means that the configuration must be available somehow.
I am working on creating the new class OpenMPIRBuilderConfig (or maybe OpenMPIRBuilderContext). The config can be passed in to both the OpenMPIRBuilder and OffloadEntriesInfoManager, constructors. The config can be made optional either by using a pointer which could be null so that the uses of OpenMPIRBuilder in mlir and opt do not have to create a config, howerver using functions that require it would cause an error (segfault). Alternatively we could make the various flags in the Config into llvm::Optional<Flag> so that the configuration can be done flexibly, and use of any functions in the Builder would give a nicer error if the value is not set. It will also make the code look more uniform with "Config." instead of "Config->".

I'm currently working on using llvm::Optional for the various attiibutes, since that is more flexible. Let me know if you have any other ideas how to handle this.

[jdoerfert]

In D137725#3930985, @jsjodin wrote:

I'm currently working on using llvm::Optional for the various attiibutes, since that is more flexible. Let me know if you have any other ideas how to handle this.

Optional is good, for the fields or the config object. Alternatively defaults as we can derive them would be OK too. A mix might be good, if we can derive a sensible default, we use it, if not, it's Optional and None.

[jsjodin]

In D137725#3930994, @jdoerfert wrote:

In D137725#3930985, @jsjodin wrote:

I'm currently working on using llvm::Optional for the various attiibutes, since that is more flexible. Let me know if you have any other ideas how to handle this.

Optional is good, for the fields or the config object. Alternatively defaults as we can derive them would be OK too. A mix might be good, if we can derive a sensible default, we use it, if not, it's Optional and None.

Just for reference the patch is here: https://reviews.llvm.org/D138220

[jdoerfert]

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

160

This will move into the config object now, right? Can we also avoid the user telling us the string refs and instead determining them on the rest of the config?

[jsjodin]

Use the new config class. Perhaps the separators could be inferred from isTargetCodgen, but keeping them explicit for now.

[jsjodin]

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

160

This will move into the config object now, right? Can we also avoid the user telling us the string refs and instead determining them on the rest of the config?

160

This will move into the config object now, right? Can we also avoid the user telling us the string refs and instead determining them on the rest of the config?

Yes, I updated the patch just to see that everything builds okay, but we should be able to infer the separators.

182

Rename to something meaningful.

Forgot to address this, will fix together with inferring separators.

[jdoerfert]

clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp

895

Why do we need to pass them here at all? What's the second argument for? Doesn't it effectively tell us what the separators are?

[tschuett]

I just wanted to point you at https://reviews.llvm.org/D136743 (BuiltinTypeDeclBuilder). They use the Rust Builder Pattern. If the configuration of the OpenMPIRBuilder becomes to complex, maybe there is space for something similar.

[jdoerfert]

In D137725#3944918, @tschuett wrote:

I just wanted to point you at https://reviews.llvm.org/D136743 (BuiltinTypeDeclBuilder). They use the Rust Builder Pattern. If the configuration of the OpenMPIRBuilder becomes to complex, maybe there is space for something similar.

Will be kept in mind. Thx!

[jsjodin]

Make firstSeparator() and separator() return the correct default values when the optional values are not set. This will allow overriding the default, and will avoid inconsistencies in the case IsTargetCodegen is modified in the config. Change name of getName to something more descriptive. Maybe getPlatformSpecificName is better than createPlatformSpecificName, feedback is welcome.

[jdoerfert]

LG, thx

clang/lib/CodeGen/CGOpenMPRuntime.h

static LastprivateConditionalRAII disable(CodeGenFunction &CGF,
                                               const OMPExecutableDirective &S);
     ~LastprivateConditionalRAII();
   };
 
   llvm::OpenMPIRBuilder &getOMPBuilder() { return OMPBuilder; }
 
 protected:
   CodeGenModule &CGM;
-  StringRef FirstSeparator, Separator;
 
   /// An OpenMP-IR-Builder instance.
   llvm::OpenMPIRBuilder OMPBuilder;
 
-  /// Constructor allowing to redefine the name separator for the variables.
-  explicit CGOpenMPRuntime(CodeGenModule &CGM, StringRef FirstSeparator,
-                           StringRef Separator);
-
   /// Helper to emit outlined function for 'target' directive.
   /// \param D Directive to emit.
   /// \param ParentName Name of the function that encloses the target region.
   /// \param OutlinedFn Outlined function value to be defined by this call.
   /// \param OutlinedFnID Outlined function ID value to be defined by this call.
   /// \param IsOffloadEntry True if the outlined function is an offload entry.
   /// \param CodeGen Lambda codegen specific to an accelerator device.
   /// An outlined function may not be an entry if, e.g. the if clause always
protected:
   emitDepobjElementsSizes(CodeGenFunction &CGF, QualType &KmpDependInfoTy,
                           const OMPTaskDataTy::DependData &Data);
 
   void emitDepobjElements(CodeGenFunction &CGF, QualType &KmpDependInfoTy,
                           LValue PosLVal, const OMPTaskDataTy::DependData &Data,
                           Address DependenciesArray);
 
 public:
-  explicit CGOpenMPRuntime(CodeGenModule &CGM)
-      : CGOpenMPRuntime(CGM, ".", ".") {}
+  explicit CGOpenMPRuntime(CodeGenModule &CGM);
   virtual ~CGOpenMPRuntime() {}
   virtual void clear();
 
   /// Returns true if the current target is a GPU.
   virtual bool isTargetCodegen() const { return false; }
 
   /// Emits code for OpenMP 'if' clause using specified \a CodeGen
   /// function. Here is the logic:

clang/lib/CodeGen/CGOpenMPRuntime.cpp

auto *Field = FieldDecl::Create(
       C, DC, SourceLocation(), SourceLocation(), /*Id=*/nullptr, FieldTy,
       C.getTrivialTypeSourceInfo(FieldTy, SourceLocation()),
       /*BW=*/nullptr, /*Mutable=*/false, /*InitStyle=*/ICIS_NoInit);
   Field->setAccess(AS_public);
   DC->addDecl(Field);
   return Field;
 }
 
-CGOpenMPRuntime::CGOpenMPRuntime(CodeGenModule &CGM, StringRef FirstSeparator,
-                                 StringRef Separator)
-    : CGM(CGM), FirstSeparator(FirstSeparator), Separator(Separator),
-      OMPBuilder(CGM.getModule()), OffloadEntriesInfoManager() {
+CGOpenMPRuntime::CGOpenMPRuntime(CodeGenModule &CGM)
+    : CGM(CGM), OMPBuilder(CGM.getModule()), OffloadEntriesInfoManager() {
   KmpCriticalNameTy = llvm::ArrayType::get(CGM.Int32Ty, /*NumElements*/ 8);
   llvm::OpenMPIRBuilderConfig Config(CGM.getLangOpts().OpenMPIsDevice, false,
                                      hasRequiresUnifiedSharedMemory());
-
   // Initialize Types used in OpenMPIRBuilder from OMPKinds.def
   OMPBuilder.initialize();
   OMPBuilder.setConfig(Config);
   OffloadEntriesInfoManager.setConfig(Config);
   loadOffloadInfoMetadata();
 }
 
 void CGOpenMPRuntime::clear() {
   InternalVars.clear();
   // Clean non-target variable declarations possibly used only in debug info.
   for (const auto &Data : EmittedNonTargetVariables) {
     if (!Data.getValue().pointsToAliveValue())
       continue;
     auto *GV = dyn_cast<llvm::GlobalVariable>(Data.getValue());
     if (!GV)
       continue;
     if (!GV->isDeclaration() || GV->getNumUses() > 0)
       continue;
     GV->eraseFromParent();
   }
 }
 
 std::string CGOpenMPRuntime::getName(ArrayRef<StringRef> Parts) const {
-  SmallString<128> Buffer;
-  llvm::raw_svector_ostream OS(Buffer);
-  StringRef Sep = FirstSeparator;
-  for (StringRef Part : Parts) {
-    OS << Sep << Part;
-    Sep = Separator;
-  }
-  return std::string(OS.str());
+  return OMPBuilder.createPlatformSpecificName(Parts);
 }
 
 static llvm::Function *
 emitCombinerOrInitializer(CodeGenModule &CGM, QualType Ty,
                           const Expr *CombinerInitializer, const VarDecl *In,
                           const VarDecl *Out, bool IsCombiner) {
   // void .omp_combiner.(Ty *in, Ty *out);
   ASTContext &C = CGM.getContext();

clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp

case EM_NonSPMD:
     return (~KMP_IDENT_SPMD_MODE) & (~KMP_IDENT_SIMPLE_RT_MODE);
   case EM_Unknown:
     return UndefinedMode;
   }
   llvm_unreachable("Unknown flags are requested.");
 }
 
 CGOpenMPRuntimeGPU::CGOpenMPRuntimeGPU(CodeGenModule &CGM)
-    : CGOpenMPRuntime(CGM, "_", "$") {
+    : CGOpenMPRuntime(CGM) {
   llvm::OpenMPIRBuilderConfig Config(CGM.getLangOpts().OpenMPIsDevice, true,
                                      hasRequiresUnifiedSharedMemory());
   OMPBuilder.setConfig(Config);
   OffloadEntriesInfoManager.setConfig(Config);
 
   if (!CGM.getLangOpts().OpenMPIsDevice)
     llvm_unreachable("OpenMP can only handle device code.");
 
   llvm::OpenMPIRBuilder &OMPBuilder = getOMPBuilder();
   if (CGM.getLangOpts().NoGPULib || CGM.getLangOpts().OMPHostIRFile.empty())
     return;

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

public:
   /// Flag for specifying if the compilation is done for an offloading target,
   /// like GPU.
   Optional<bool> IsTargetCodegen;
 
   /// Flag for specifying weather a requires unified_shared_memory
   /// directive is present or not.
   Optional<bool> HasRequiresUnifiedSharedMemory;
 
+  /// First separator used between the initial two parts of a name.
+  Optional<StringRef> FirstSeparator;
+  /// Separator used between all of the rest consecutive parts of s name
+  Optional<StringRef> Separator;
+
   OpenMPIRBuilderConfig() {}
   OpenMPIRBuilderConfig(bool IsEmbedded, bool IsTargetCodegen,
                         bool HasRequiresUnifiedSharedMemory)
       : IsEmbedded(IsEmbedded), IsTargetCodegen(IsTargetCodegen),
         HasRequiresUnifiedSharedMemory(HasRequiresUnifiedSharedMemory) {}
 
-  // Convenience getter functions that assert if the value is not present.
-  bool isEmbedded() {
+  // Getters functions that assert if the required values are not present.
+  bool isEmbedded() const {
     assert(IsEmbedded.has_value() && "IsEmbedded is not set");
     return IsEmbedded.value();
   }
 
-  bool isTargetCodegen() {
+  bool isTargetCodegen() const {
     assert(IsTargetCodegen.has_value() && "IsTargetCodegen is not set");
     return IsTargetCodegen.value();
   }
 
-  bool hasRequiresUnifiedSharedMemory() {
+  bool hasRequiresUnifiedSharedMemory() const {
     assert(HasRequiresUnifiedSharedMemory.has_value() &&
            "HasUnifiedSharedMemory is not set");
     return HasRequiresUnifiedSharedMemory.value();
   }
 
+  // Returns the FirstSeparator if set, otherwise use the default
+  // separator depending on isTargetCodegen
+  StringRef firstSeparator() const {
+    if (FirstSeparator.has_value())
+      return FirstSeparator.value();
+    if (isTargetCodegen())
+      return "_";
+    return ".";
+  }
+
+  // Returns the Separator if set, otherwise use the default
+  // separator depending on isTargetCodegen
+  StringRef separator() const {
+    if (Separator.has_value())
+      return Separator.value();
+    if (isTargetCodegen())
+      return "$";
+    return ".";
+  }
+
   void setIsEmbedded(bool Value) { IsEmbedded = Value; }
   void setIsTargetCodegen(bool Value) { IsTargetCodegen = Value; }
   void setHasRequiresUnifiedSharedMemory(bool Value) {
     HasRequiresUnifiedSharedMemory = Value;
   }
+  void setFirstSeparator(StringRef FS) { FirstSeparator = FS; }
+  void setSeparator(StringRef S) { Separator = S; }
 };
 
 /// An interface to create LLVM-IR for OpenMP directives.
 ///
 /// Each OpenMP directive has a corresponding public generator method.
 class OpenMPIRBuilder {
 public:
   /// Create a new OpenMPIRBuilder operating on the given module \p M. This will
   /// not have an effect on \p M (see initialize)
   OpenMPIRBuilder(Module &M) : M(M), Builder(M.getContext()) {}
   ~OpenMPIRBuilder();
 
   /// Initialize the internal state, this will put structures types and
   /// potentially other helpers into the underlying module. Must be called
   /// before any other method and only once!
   void initialize();
 
   void setConfig(OpenMPIRBuilderConfig C) { Config = C; }
 
   /// Finalize the underlying module, e.g., by outlining regions.
   /// \param Fn                    The function to be finalized. If not used,
   ///                              all functions are finalized.
   void finalize(Function *Fn = nullptr);
 
   /// Add attributes known for \p FnID to \p Fn.
   void addAttributes(omp::RuntimeFunction FnID, Function &Fn);
 
   /// Type used throughout for insertion points.
   using InsertPointTy = IRBuilder<>::InsertPoint;
 
+  /// Get the create a name using the platform specific separators.
+  /// \param Parts parts of the final name that needs separation
+  /// The created name has a first separator between the first and second part
+  /// and a second separator between all other parts.
+  /// E.g. with FirstSeparator "$" and Separator "." and
+  /// parts: "p1", "p2", "p3", "p4"
+  /// The resulting name is "p1$p2.p3.p4"
+  /// The separators are retrieved from the OpenMPIRBuilderConfig.
+  std::string createPlatformSpecificName(ArrayRef<StringRef> Parts) const;
+
   /// Callback type for variable finalization (think destructors).
   ///
   /// \param CodeGenIP is the insertion point at which the finalization code
   ///                  should be placed.
   ///
   /// A finalize callback knows about all objects that need finalization, e.g.
   /// destruction, when the scope of the currently generated construct is left
   /// at the time, and location, the callback is invoked.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

std::string OpenMPIRBuilder::getNameWithSeparators(ArrayRef<StringRef> Parts,
   StringRef Sep = FirstSeparator;
   for (StringRef Part : Parts) {
     OS << Sep << Part;
     Sep = Separator;
   }
   return OS.str().str();
 }
 
+std::string
+OpenMPIRBuilder::createPlatformSpecificName(ArrayRef<StringRef> Parts) const {
+  return OpenMPIRBuilder::getNameWithSeparators(Parts, Config.firstSeparator(),
+                                                Config.separator());
+}
+
 GlobalVariable *
 OpenMPIRBuilder::getOrCreateInternalVariable(Type *Ty, const StringRef &Name,
                                              unsigned AddressSpace) {
   auto &Elem = *InternalVars.try_emplace(Name, nullptr).first;
   if (Elem.second) {
     assert(cast<PointerType>(Elem.second->getType())
                ->isOpaqueOrPointeeTypeMatches(Ty) &&
            "OMP internal variable has different type than requested");