[PATCH] D147218: [OpenMP][Flang][MLIR] Lowering of OpenMP requires directive from parse tree to MLIR

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

Could you add to the summary that the `atomic` related handling is done elsewhere.

Could you expand the tests to cover the various `if` conditions that are used in the code?



================
Comment at: flang/include/flang/Lower/OpenMP.h:16
 
+#include "mlir/Dialect/OpenMP/OpenMPDialect.h"
 #include <cinttypes>
----------------
Is this include required here?


================
Comment at: flang/lib/Lower/Bridge.cpp:315
+                     [&](Fortran::lower::pft::BlockDataUnit &b) {
+                       if (!globalOmpRequiresSymbol)
+                         globalOmpRequiresSymbol = b.symTab.symbol();
----------------
Is this handling required for `Block Data`? If so, could you add a test?


================
Comment at: flang/lib/Lower/Bridge.cpp:4779
+    if (ompDeviceCodeFound)
+      Fortran::lower::genOpenMPRequires(getModuleOp().getOperation(),
+                                        globalOmpRequiresSymbol);
----------------
If this is specific for device code, might be worth renaming it to something specific to device.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147218