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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to