[PATCH] D115030: [mlir][OpenMP] omp.sections and omp.section lowering to LLVM IR

[Alex Zinenko via Phabricator via llvm-commits]

ftynse accepted this revision.
ftynse added a comment.
This revision is now accepted and ready to land.

MLIR part LGTM with comments addressed. Please have the OpenMP part reviewed by @jdoerfert or @Meinersbur, or factor it out into a separate patch.



================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:566
+      !sectionsOp.allocators_vars().empty())
+    emitWarning(sectionsOp.getLoc())
+        << "private, firstprivate, lastprivate, reduction and allocate clauses "
----------------
You probably want to `return failure()` and stop the conversion process.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:573
+
+  for (auto &inst : *sectionsOp.region().begin()) {
+    auto sectionOp = dyn_cast<omp::SectionOp>(inst);
----------------
Nit: please expand auto here. Also, let's not perpetuate the very outdated `inst`/instruction mlir terminology.


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:578
+
+    auto &region = sectionOp.region();
+    auto SectionCB = [&region, &builder, &moduleTranslation, &bodyGenStatus](
----------------
Nit: expand auto here too please. MLIR prefers explicit types unless they are too long (iterators) or impossible to spell (lambdas), or if the type is obvious from the RHS (essentially if the RHS ends with a cast).


================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:579
+    auto &region = sectionOp.region();
+    auto SectionCB = [&region, &builder, &moduleTranslation, &bodyGenStatus](
+                         InsertPointTy AllocaIP, InsertPointTy CodeGenIP,
----------------
Nit: lowerCamelCase in MLIR please, here and below.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115030/new/

https://reviews.llvm.org/D115030