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

[Sergio Afonso via Phabricator via cfe-commits]

skatrak added a comment.

Thank you Kiran for having a look at this!

In D147218#4643257 <https://reviews.llvm.org/D147218#4643257>, @kiranchandramohan wrote:

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

Done.

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

I created a test to gather the flags from the symbol for a `FunctionLikeUnit` and for a `BlockDataUnit`, and checked that these flags are only added for the module when there's a device construct in the compilation unit, regardless of whether we're compiling for host or device and whether the device construct is a target region or a declare target subroutine. I think that covers all main aspects of this patch.

I have also detected that there is a situation where this approach doesn't currently work, so I created an expected-fail test for unnamed block data. The issue is that there is no symbol created for those, so there's nowhere to store/find these flags. I have seen this happen for main programs as well, but I've not been able to create a reproducer for which that happened that contained device constructs. In my opinion, this edge case should be addressed as its own patch.



================
Comment at: flang/include/flang/Lower/OpenMP.h:16
 
+#include "mlir/Dialect/OpenMP/OpenMPDialect.h"
 #include <cinttypes>
----------------
kiranchandramohan wrote:
> Is this include required here?
It was, but only because I had made public a function that wasn't supposed to be, thanks for noticing.


================
Comment at: flang/lib/Lower/Bridge.cpp:4779
+    if (ompDeviceCodeFound)
+      Fortran::lower::genOpenMPRequires(getModuleOp().getOperation(),
+                                        globalOmpRequiresSymbol);
----------------
kiranchandramohan wrote:
> If this is specific for device code, might be worth renaming it to something specific to device.
This is for both host and device, it's just that it's only generated if there are device constructs in the compilation unit. Let me know if you still suggest renaming it to something else.

I modified the unit tests to run them for the target device as well, so that it's clear that the same behavior is expected.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147218