[Clang][Flang][OpenMP] Add loadOffloadInfoMetadata and createOffloadEntriesAndInfoMetadata into OMPIRBuilder's finalize and initialize

[agozillon]

Summary

This allows the generation of OpenMP offload metadata for the OpenMP
dialect when lowering to LLVM-IR and moves some of the shared logic
between the OpenMP Dialect and Clang into the IRBuilder.

This patch has a dependency on: https://reviews.llvm.org/D148038

As it requires the host ir file during lowering.

[agozillon]

A little unsure how to test this change on its own for Flang-new + OpenMP, as there currently isn't anything that utilises it upstream at the moment. It's part of the declare target work that I'm beginning to upstream, so eventually it'll be tested on the Flang side through it, and the Target region work will also eventually utilise it.

As for Clang OpenMP, it currently doesn't break any existing tests on my machine for OpenMP on AMDGPU or host tests, however, I can't speak for other targets but I expect they will pass happily as well.

If there is additional tests I can add that might contribute to the patch please do ask away!

[agozillon]

mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h

174 ↗

(On Diff #513723)

This will break the patch buildbot unfortunately until the patch this patch is dependent on is accepted and upstreamed

[jsjodin]

mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h

174 ↗

(On Diff #513723)

This will break the patch buildbot unfortunately until the patch this patch is dependent on is accepted and upstreamed

You can always skip this part and add it in a separate patch, or add it in a patch where it is needed.

[agozillon]

The patch this is dependent on has now been accepted and landed, so this should be ready for review now if anyone is available!

[agozillon]

• Move hostIRFilePath initialize invocation to ModuleTranslation.cpp to respect TargetOp patch

[agozillon]

• Move hostIRFilePath initialize invocation to ModuleTranslation.cpp to respect TargetOp patch

[agozillon]

rebase to see if it clears up the buildbot issue (currently the failing test appears to be unrelated and passes on my machine after a rebase)

[agozillon]

Small ping to ask for some reviewer attention on this patch if at all possible!

[jdoerfert]

clang/lib/CodeGen/CGOpenMPRuntime.cpp

1063

Move the additional comment to the initialize function instead to explain what the argument is for.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

452

early exit plz.

453

No llvm::

455

no assert false please. Use unreachable or report fatal error.

[agozillon]

• Move hostIRFilePath initialize invocation to ModuleTranslation.cpp to respect TargetOp patch
• Apply reviewer feedback

[agozillon]

Thank you very much for the quick response time on the review and the review @jdoerfert! I believe I have applied all of your current feedback in the last update.

[jdoerfert]

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

563

no llvm::
style: ErrorReportFn

similarly elsewhere.

[agozillon]

• Move hostIRFilePath initialize invocation to ModuleTranslation.cpp to respect TargetOp patch
• Apply reviewer feedback
• Tidy up missed style guidelines i sillily forgot about

[agozillon]

• Remove unneccessary OMPIRBuilder scope as well

[agozillon]

Updated to fix the style issues that were present and pointed out by @jdoerfert thank you!

[agozillon]

Hey @jdoerfert sorry to bother you, would it be possible to have this signed off on if there is no further issues with the current patch? Thank you for your time!

[agozillon]

If it would be possible to get some poor reviewers precious time on this I would greatly appreciate it! Thank you very much ahead of time.

[kiranchandramohan]

LG. Please wait a day before submitting.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

455–468

Nit: Might be good to spell the types and use braces for multi-line code inside if.

mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h

18 ↗

(On Diff #519050)

Nit: Is this required here? Can it be in the CPP File?

[agozillon]

Thank you very much @kiranchandramohan! More than happy to wait a day incase anyone else wishes to do a final review. I'll add the Nits where possible when I commit the patch upstream and it should hopefully update the review with the alterations!

[agozillon]

I am going to upstream this within the next hour, unless anyone has some final comments they wish to make

clang/lib/CodeGen/CGOpenMPRuntime.h

protected:
 
   /// Flag for keeping track of weather a target region has been emitted.
   bool HasEmittedTargetRegion = false;
 
   /// Flag for keeping track of weather a device routine has been emitted.
   /// Device routines are specific to the
   bool HasEmittedDeclareTargetRegion = false;
 
-  /// Loads all the offload entries information from the host IR
-  /// metadata.
-  void loadOffloadInfoMetadata();
-
   /// Start scanning from statement \a S and emit all target regions
   /// found along the way.
   /// \param S Starting statement.
   /// \param ParentName Name of the function declaration that is being scanned.
   void scanForTargetRegionsFunctions(const Stmt *S, StringRef ParentName);
 
   /// Build type kmp_routine_entry_t (if not built yet).
   void emitKmpRoutineEntryT(QualType KmpInt32Ty);

clang/lib/CodeGen/CGOpenMPRuntime.cpp

 }
 
 CGOpenMPRuntime::CGOpenMPRuntime(CodeGenModule &CGM)
     : CGM(CGM), OMPBuilder(CGM.getModule()) {
   KmpCriticalNameTy = llvm::ArrayType::get(CGM.Int32Ty, /*NumElements*/ 8);
   llvm::OpenMPIRBuilderConfig Config(CGM.getLangOpts().OpenMPIsDevice, false,
                                      hasRequiresUnifiedSharedMemory(),
                                      CGM.getLangOpts().OpenMPOffloadMandatory);
-  // Initialize Types used in OpenMPIRBuilder from OMPKinds.def
-  OMPBuilder.initialize();
+  OMPBuilder.initialize(CGM.getLangOpts().OpenMPIsDevice
+                            ? CGM.getLangOpts().OMPHostIRFile
+                            : StringRef{});
   OMPBuilder.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;
case llvm::OpenMPIRBuilder::EMIT_MD_GLOBAL_VAR_LINK_ERROR: {
       CGM.getDiags().Report(DiagID);
     } break;
     }
   };
 
   OMPBuilder.createOffloadEntriesAndInfoMetadata(ErrorReportFn);
 }
 
-/// Loads all the offload entries information from the host IR
-/// metadata.
-void CGOpenMPRuntime::loadOffloadInfoMetadata() {
-  // If we are in target mode, load the metadata from the host IR. This code has
-  // to match the metadaata creation in createOffloadEntriesAndInfoMetadata().
-
-  if (!CGM.getLangOpts().OpenMPIsDevice)
-    return;
-
-  if (CGM.getLangOpts().OMPHostIRFile.empty())
-    return;
-
-  auto Buf = llvm::MemoryBuffer::getFile(CGM.getLangOpts().OMPHostIRFile);
-  if (auto EC = Buf.getError()) {
-    CGM.getDiags().Report(diag::err_cannot_open_file)
-        << CGM.getLangOpts().OMPHostIRFile << EC.message();
-    return;
-  }
-
-  llvm::LLVMContext C;
-  auto ME = expectedToErrorOrAndEmitErrors(
-      C, llvm::parseBitcodeFile(Buf.get()->getMemBufferRef(), C));
-
-  if (auto EC = ME.getError()) {
-    unsigned DiagID = CGM.getDiags().getCustomDiagID(
-        DiagnosticsEngine::Error, "Unable to parse host IR file '%0':'%1'");
-    CGM.getDiags().Report(DiagID)
-        << CGM.getLangOpts().OMPHostIRFile << EC.message();
-    return;
-  }
-
-  OMPBuilder.loadOffloadInfoMetadata(*ME.get());
-}
-
 void CGOpenMPRuntime::emitKmpRoutineEntryT(QualType KmpInt32Ty) {
   if (!KmpRoutineEntryPtrTy) {
     // Build typedef kmp_int32 (* kmp_routine_entry_t)(kmp_int32, void *); type.
     ASTContext &C = CGM.getContext();
     QualType KmpRoutineEntryTyArgs[] = {KmpInt32Ty, C.VoidPtrTy};
     FunctionProtoType::ExtProtoInfo EPI;
     KmpRoutineEntryPtrQTy = C.getPointerType(
         C.getFunctionType(KmpInt32Ty, KmpRoutineEntryTyArgs, EPI));

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

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()), OffloadInfoManager(this) {}
   ~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();
+  /// before any other method and only once! This internal state includes
+  /// Types used in the OpenMPIRBuilder generated from OMPKinds.def as well
+  /// as loading offload metadata for device from the OpenMP host IR file
+  /// passed in as the HostFilePath argument.
+  /// \param HostFilePath The path to the host IR file, used to load in
+  /// offload metadata for the device, allowing host and device to
+  /// maintain the same metadata mapping.
+  void initialize(StringRef HostFilePath = {});
 
   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);
 

llvm/lib/Frontend/OpenMP/CMakeLists.txt

add_llvm_component_library(LLVMFrontendOpenMP
   LINK_COMPONENTS
   Core
   Support
   TargetParser
   TransformUtils
   Analysis
   MC
   Scalar
+  BitReader
   )

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Analysis/AssumptionCache.h"
 #include "llvm/Analysis/CodeMetrics.h"
 #include "llvm/Analysis/LoopInfo.h"
 #include "llvm/Analysis/OptimizationRemarkEmitter.h"
 #include "llvm/Analysis/ScalarEvolution.h"
 #include "llvm/Analysis/TargetLibraryInfo.h"
+#include "llvm/Bitcode/BitcodeReader.h"
 #include "llvm/IR/CFG.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/GlobalVariable.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/MDBuilder.h"
 #include "llvm/IR/PassManager.h"

 
 Function *OpenMPIRBuilder::getOrCreateRuntimeFunctionPtr(RuntimeFunction FnID) {
   FunctionCallee RTLFn = getOrCreateRuntimeFunction(M, FnID);
   auto *Fn = dyn_cast<llvm::Function>(RTLFn.getCallee());
   assert(Fn && "Failed to create OpenMP runtime function pointer");
   return Fn;
 }
 
-void OpenMPIRBuilder::initialize() { initializeTypes(M); }
+void OpenMPIRBuilder::initialize(StringRef HostFilePath) {
+  initializeTypes(M);
+
+  if (HostFilePath.empty())
+    return;
+
+  auto Buf = MemoryBuffer::getFile(HostFilePath);
+  if (std::error_code Err = Buf.getError()) {
+    report_fatal_error(("error opening host file from host file path inside of "
+                        "OpenMPIRBuilder: " +
+                        Err.message())
+                           .c_str());
+  }
+
+  LLVMContext Ctx;
+  auto M = expectedToErrorOrAndEmitErrors(
+      Ctx, parseBitcodeFile(Buf.get()->getMemBufferRef(), Ctx));
+  if (std::error_code Err = M.getError()) {
+    report_fatal_error(
+        ("error parsing host file inside of OpenMPIRBuilder: " + Err.message())
+            .c_str());
+  }
+
+  loadOffloadInfoMetadata(*M.get());
+}
 
 void OpenMPIRBuilder::finalize(Function *Fn) {
   SmallPtrSet<BasicBlock *, 32> ParallelRegionBlockSet;
   SmallVector<BasicBlock *, 32> Blocks;
   SmallVector<OutlineInfo, 16> DeferredOutlines;
   for (OutlineInfo &OI : OutlineInfos) {
     // Skip functions that have not finalized yet; may happen with nested
     // function generation.
for (OutlineInfo &OI : OutlineInfos) {
 
     // Run a user callback, e.g. to add attributes.
     if (OI.PostOutlineCB)
       OI.PostOutlineCB(*OutlinedFn);
   }
 
   // Remove work items that have been completed.
   OutlineInfos = std::move(DeferredOutlines);
+
+  EmitMetadataErrorReportFunctionTy &&ErrorReportFn =
+      [](EmitMetadataErrorKind Kind,
+         const TargetRegionEntryInfo &EntryInfo) -> void {
+    errs() << "Error of kind: " << Kind
+           << " when emitting offload entries and metadata during "
+              "OMPIRBuilder finalization \n";
+  };
+
+  if (!OffloadInfoManager.empty())
+    createOffloadEntriesAndInfoMetadata(ErrorReportFn);
 }
 
 OpenMPIRBuilder::~OpenMPIRBuilder() {
   assert(OutlineInfos.empty() && "There must be no outstanding outlinings");
 }
 
 GlobalValue *OpenMPIRBuilder::createGlobalFlag(unsigned Value, StringRef Name) {
   IntegerType *I32Ty = Type::getInt32Ty(M.getContext());

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

SmallVector<llvm::Value *> ModuleTranslation::lookupValues(ValueRange values) {
   for (Value v : values)
     remapped.push_back(lookupValue(v));
   return remapped;
 }
 
 llvm::OpenMPIRBuilder *ModuleTranslation::getOpenMPBuilder() {
   if (!ompBuilder) {
     ompBuilder = std::make_unique<llvm::OpenMPIRBuilder>(*llvmModule);
-    ompBuilder->initialize();
 
     bool isDevice = false;
+    llvm::StringRef hostIRFilePath = "";
     if (auto offloadMod =
-            dyn_cast<mlir::omp::OffloadModuleInterface>(mlirModule))
+            dyn_cast<mlir::omp::OffloadModuleInterface>(mlirModule)) {
       isDevice = offloadMod.getIsDevice();
+      hostIRFilePath = offloadMod.getHostIRFilePath();
+    }
+
+    ompBuilder->initialize(hostIRFilePath);
 
     // TODO: set the flags when available
-    llvm::OpenMPIRBuilderConfig Config(
+    llvm::OpenMPIRBuilderConfig config(
         isDevice, /* IsTargetCodegen */ false,
         /* HasRequiresUnifiedSharedMemory */ false,
         /* OpenMPOffloadMandatory */ false);
-    ompBuilder->setConfig(Config);
+    ompBuilder->setConfig(config);
   }
   return ompBuilder.get();
 }
 
 llvm::DILocation *ModuleTranslation::translateLoc(Location loc,
                                                   llvm::DILocalScope *scope) {
   return debugTranslation->translateLoc(loc, scope);
 }