[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

117

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

84

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

84

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?

84

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.

117

Rename to something meaningful.

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

[jdoerfert]

clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp

895 ↗

(On Diff #477257)

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

clang/lib/CodeGen/CGOpenMPRuntime.cpp

auto *Field = FieldDecl::Create(
       /*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() {
+    : CGM(CGM), OMPBuilder(CGM.getModule(), FirstSeparator, Separator),
+      OffloadEntriesInfoManager() {
   KmpCriticalNameTy = llvm::ArrayType::get(CGM.Int32Ty, /*NumElements*/ 8);
 
   // Initialize Types used in OpenMPIRBuilder from OMPKinds.def
   OMPBuilder.initialize();
   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.getName(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();

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

 /// Like splitBB, but reuses the current block's name for the new name.
 BasicBlock *splitBBWithSuffix(IRBuilderBase &Builder, bool CreateBranch,
                               llvm::Twine Suffix = ".split");
 
 /// An interface to create LLVM-IR for OpenMP directives.
 ///
 /// Each OpenMP directive has a corresponding public generator method.
 class OpenMPIRBuilder {
+protected:
+  /// First separator used between the initial two parts of a name.
+  StringRef FirstSeparator;
+  /// Separator used between all of the rest consecutive parts of s name
+  StringRef Separator;
+
 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(Module &M)
+      : FirstSeparator(""), Separator(""), M(M), Builder(M.getContext()) {}
+  /// Create a new OpenMPIRBuilder operating on the given module \p M and
+  /// strings \p FirstSeparator and \p Separator. This will not have an effect
+  /// on \p M (see initialize).
+  OpenMPIRBuilder(Module &M, StringRef FirstSeparator, StringRef Separator)
+      : FirstSeparator(FirstSeparator), Separator(Separator), 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();
 
   /// 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 platform-specific name separator.
+  /// \param Parts different parts of the final name that needs separation
+  std::string getName(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::getName(ArrayRef<StringRef> Parts) const {
+  return OpenMPIRBuilder::getNameWithSeparators(Parts, FirstSeparator,
+                                                Separator);
+}
+
 Constant *OpenMPIRBuilder::getOrCreateOMPInternalVariable(
     llvm::Type *Ty, const llvm::Twine &Name, unsigned AddressSpace) {
   // TODO: Replace the twine arg with stringref to get rid of the conversion
   // logic. However This is taken from current implementation in clang as is.
   // Since this method is used in many places exclusively for OMP internal use
   // we will keep it as is for temporarily until we move all users to the
   // builder and then, if possible, fix it everywhere in one go.
   SmallString<256> Buffer;