[flang-commits] [flang] [openmp] Changes to support link clause of declare target with common block (PR #84825)

[via flang-commits]

================
@@ -1037,6 +1037,31 @@ static void genBodyOfTargetOp(
     genNestedEvaluations(converter, eval);
 }
 
+// If the symbol is specified in declare target directive, the function returns
+// the corresponding declare target operation.
+static mlir::omp::DeclareTargetInterface
+getDeclareTargetOp(const Fortran::semantics::Symbol &sym,
----------------
agozillon wrote:

Nice PR so far! :-)

However, I have a feeling this is only part of the puzzle with common blocks. I don't know enough about common blocks and OpenMPs interaction with them enough yet though, so I could most definitely be wrong. @mjklemm or @kiranchandramohan may perhaps know better in this case. 

However, if you emit the LLVM-IR for the two cases you've added to: openmp/libomptarget/test/offloading/fortran/declare-target-vars-in-target-region.f90 

You will see that the variables used in the regions are being treated as regular (implicitly) mapped variables, rather than declare target variables, and that the common block global is actually generated as a declare target global variable but unused on device. The elements of the common block are being accessed via a GEP and then passed to the target region, which seems reasonable from the perspective of infrastructure that's in place currently but I am not sure that's what we'd like in these cases.

I do not know if members of a declare target'd common block themselves should be treated as if they were declare target or not though (this looks from IR inspection to be something that could be quite difficult from the way common blocks are represented). From this specification exert on page 61 of the OpenMP 5.2 specification: 

"When a named common block appears in an OpenMP argument list, it has the same meaning and restrictions as if every explicit member of the common block appeared in the list. An explicit member of a common block is a variable that is named in a COMMON statement that specifies the common block name and is declared in the same scoping unit in which the clause appears. Named common blocks do not include the blank common block"

It seems like that may be the case, but my OpenMP-fu is trainee level at best, so please do verify for yourself and then get a second opinion! 

I think referring to a common block symbol directly in a map clause e.g. `map(tofrom: /CB/)` might be another good initial test if it is legal. From what I can tell it is, based on the definition of the map clause and locator lists (the latter also on page 61) but please do verify for yourself and if in doubt ask someone internally (or externally) who has more OpenMP specification knowledge than I do! The initial issue you will likely face with that test is that I do not believe it will go through the parser as I think it will complain about the symbol, I think it needs taught to deal with it like declare target currently does...  

https://github.com/llvm/llvm-project/pull/84825