[MLIR] Fix LLVMIRTransforms build failure

[RoboTux]

lib/libMLIRLLVMIRTransforms.a fails to build from scratch with the
following error:
In file included from llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:19,
from llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:19,
from mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h:26,
from mlir/include/mlir/Dialect/LLVMIR/NVVMDialect.h:24,
from mlir/lib/Dialect/LLVMIR/Transforms/InlinerInterfaceImpl.cpp:17:
llvm/include/llvm/Frontend/OpenMP/OMP.h:16:10:
fatal error: llvm/Frontend/OpenMP/OMP.h.inc: No such file or directory

Solve issue by adding missing dependency on MLIRTargetLLVMIRExport and
splitting MLIRLLVMIRTransforms to make MLIRTargetLLVMIRExport depend on
a new MLIRLLVMIRTransformsLegalizeForExport instead of
MLIRLLVMIRTransforms and thus avoid a dependency cycle.

[llvmbot]

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-llvm

Author: Thomas Preud'homme (RoboTux)

Changes

lib/libMLIRLLVMIRTransforms.a fails to build from scratch with the
following error:
In file included from llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:19,
from llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:19,
from mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h:26,
from mlir/include/mlir/Dialect/LLVMIR/NVVMDialect.h:24,
from mlir/lib/Dialect/LLVMIR/Transforms/InlinerInterfaceImpl.cpp:17:
llvm/include/llvm/Frontend/OpenMP/OMP.h:16:10:
fatal error: llvm/Frontend/OpenMP/OMP.h.inc: No such file or directory

Solve issue by adding missing dependency on MLIRTargetLLVMIRExport and
splitting MLIRLLVMIRTransforms to make MLIRTargetLLVMIRExport depend on
a new MLIRLLVMIRTransformsLegalizeForExport instead of
MLIRLLVMIRTransforms and thus avoid a dependency cycle.

---

Full diff: https://github.com/llvm/llvm-project/pull/125485.diff

3 Files Affected:

• (modified) mlir/lib/Dialect/LLVMIR/CMakeLists.txt (+1)
• (modified) mlir/lib/Dialect/LLVMIR/Transforms/CMakeLists.txt (+25-1)
• (modified) mlir/lib/Target/LLVMIR/CMakeLists.txt (+1-1)

diff --git a/mlir/lib/Dialect/LLVMIR/CMakeLists.txt b/mlir/lib/Dialect/LLVMIR/CMakeLists.txt
index c9a3b97294562a1..eba52ed54729b05 100644
--- a/mlir/lib/Dialect/LLVMIR/CMakeLists.txt
+++ b/mlir/lib/Dialect/LLVMIR/CMakeLists.txt
@@ -62,6 +62,7 @@ add_mlir_dialect_library(MLIRNVVMDialect
   MLIRLLVMDialect
   MLIRSideEffectInterfaces
   MLIRInferIntRangeInterface
+  MLIRTargetLLVMIRExport
   )
 
 add_mlir_dialect_library(MLIRROCDLDialect
diff --git a/mlir/lib/Dialect/LLVMIR/Transforms/CMakeLists.txt b/mlir/lib/Dialect/LLVMIR/Transforms/CMakeLists.txt
index d4ff0955c5d0e2f..ff7e09e7c3c8c6a 100644
--- a/mlir/lib/Dialect/LLVMIR/Transforms/CMakeLists.txt
+++ b/mlir/lib/Dialect/LLVMIR/Transforms/CMakeLists.txt
@@ -1,4 +1,4 @@
-add_mlir_dialect_library(MLIRLLVMIRTransforms
+set(LLVM_OPTIONAL_SOURCES
   AddComdats.cpp
   DIExpressionLegalization.cpp
   DIExpressionRewriter.cpp
@@ -7,6 +7,29 @@ add_mlir_dialect_library(MLIRLLVMIRTransforms
   LegalizeForExport.cpp
   OptimizeForNVVM.cpp
   RequestCWrappers.cpp
+)
+
+add_mlir_dialect_library(MLIRLLVMIRTransformsLegalizeForExport
+  LegalizeForExport.cpp
+
+  DEPENDS
+  MLIRLLVMPassIncGen
+
+  LINK_LIBS PUBLIC
+  MLIRIR
+  MLIRLLVMDialect
+  MLIRPass
+  MLIRTransforms
+)
+
+add_mlir_dialect_library(MLIRLLVMIRTransforms
+  AddComdats.cpp
+  DIExpressionLegalization.cpp
+  DIExpressionRewriter.cpp
+  DIScopeForLLVMFuncOp.cpp
+  InlinerInterfaceImpl.cpp
+  OptimizeForNVVM.cpp
+  RequestCWrappers.cpp
 
   DEPENDS
   MLIRLLVMPassIncGen
@@ -15,6 +38,7 @@ add_mlir_dialect_library(MLIRLLVMIRTransforms
   MLIRIR
   MLIRFuncDialect
   MLIRLLVMDialect
+  MLIRLLVMIRTransformsLegalizeForExport
   MLIRPass
   MLIRTransforms
   MLIRNVVMDialect
diff --git a/mlir/lib/Target/LLVMIR/CMakeLists.txt b/mlir/lib/Target/LLVMIR/CMakeLists.txt
index 93032c3ce103875..764c030f5bd5bf4 100644
--- a/mlir/lib/Target/LLVMIR/CMakeLists.txt
+++ b/mlir/lib/Target/LLVMIR/CMakeLists.txt
@@ -37,7 +37,7 @@ add_mlir_translation_library(MLIRTargetLLVMIRExport
   LINK_LIBS PUBLIC
   MLIRDLTIDialect
   MLIRLLVMDialect
-  MLIRLLVMIRTransforms
+  MLIRLLVMIRTransformsLegalizeForExport
   MLIRTranslateLib
   MLIRTransformUtils
   )

[Dinistro]

Dialects should not depend on specific tranformations. Can the NVVMDialect not transitively depend on the target that produces the problematic .h.inc file?
In general, this is probably something that @grypp or someone else from the Nvidia side should look at.

[RoboTux]

It would be odd for NVVMDialect to depend on omp_gen. Besides I don't think omp_gen would be defined in the CMakeLists defining NVVMDialect.

[Dinistro]

NVM, it's the inliner interface that causes the issue. In that case the LLVMDialect should depend on that gen target, I think. Otherwise, this will explode again in an unfortunate order for the LLVMDialect.

[RoboTux]

The dependency on the new MLIRLLVMIRTransformsLegalizeForExport is because ModuleTranslation.cpp in MLIRTargetLLVMIRExport includes LegalizeForExport.h specifically. So it felt natural to depend on that transform specifically.

I'm not sure what you mean by having LLVMDialect depending on omp_gen, but if you mean MLIRTargetLLVMIRExport which correspond includes ModuleTranslation it already depends on the component FrontendOpenMP. The build issue is solved by adding the missing depend of NVVMDialect over MLIRTargetLLVMIRExport but that creates a dependency cycle. The rest of the patch is meant to break that dependency cycle.

[RoboTux]

Note: I've updated the PR to fix the CI issue but somehow github does not seem to process the updated PR. You can see the update here: RoboTux@70ac95b

[RoboTux]

Any opinion on how this cyclic dependency could be broken @joker-eph ?

[RoboTux]

Ping?

[Dinistro]

I now looked at this again. The actual issue is the dependency on OMPIRBuilder.h, which transitively pulls in a generated header file. CMake does not really allow to express dependencies on header files, which is problematic in this case. Ideally, we would just replace the usages from this file with forward declarations, but that doesn't work due to the usage inside a unique pointer.

I'm honestly not sure how this can be achieved nicely, but your approach seems more like a workaround than a proper solution.

NVM: unique pointers work with forward declarations (explicit exception https://stackoverflow.com/questions/13414652/forward-declaration-with-unique-ptr). In this case, remove the include from OMPIRBuilder in ModuleTranslation.h and replace it with forward declarations to get rid of this issue.

Pinging @kiranchandramohan and @jeanPerier from the flang side, as they rely on the OpenMP IR builder, maybe they have an idea.

[RoboTux]

Thanks for the suggestion @Dinistro, much appreciated. I've tried it and it works.

[Dinistro]

LGTM!
Make sure to adjust the PR/commit description😊