[flang-commits] [flang] [llvm] [mlir] [Flang][OpenMP][MLIR] Fix common block mapping for regular and declare target link (PR #91829)

[via flang-commits]

agozillon wrote:

> > @kiranchandramohan No real need to review this, but I've added you as a reviewer incase you wish to feed into it, but also if you know someone else that might be quite helpful to review the change set external to AMD! In particular for this change: https://github.com/llvm/llvm-project/pull/91829/files#diff-496a295679ae3c43f8651c944a1bd9dca177ad2b5e4d7121f96938024e292bc1R841 it seems reasonable to me to do, but someone with more knowledge on common blocks and their implementation may know otherwise.
> 
> The link you have given here refers to existing code. Is that correct?
> 
> ```
> mlir::Operation *clonedOp = bound.getDefiningOp()->clone();
> ```
> 
> If you can phrase your exact question on the issue with the common block then we can request Jean or someone else to have a look.

Ah, the link location seems to have changed after the rebase and updates, sorry about that. This is the location now (the definition of genIntermediateCommonBlockAccessors) : https://github.com/llvm/llvm-project/pull/91829/files#diff-496a295679ae3c43f8651c944a1bd9dca177ad2b5e4d7121f96938024e292bc1R787 

I am wondering if it is reasonable for me to generate common block member accesses within the target region and then bind the symbol for the common block members to them for the duration of the target region lowering. This or something very similar is currently done for another piece of OpenMP code related to privatization, so it's borrowing a similar concept. It results in the appropriate member accesses to the mapped common block as well as the correct usage of the newly generated local member accesses throughout the target operation. Without this step you end up with member accesses external to the IFA target region which results in verification issues, an alternative more complex and less fool proof (for me at least) method would be to clone the existing member accesses into the region and rebind them to the corresponding blocks argument for the mapped common block. This is an issue when using the common block symbol (e.g. map(tofrom: /common_block/)to map the entirety of a common block. Other methods would be creating a map for each individual member (similarly to if we were mapping individual elements of the common block), but this led to issues later in lowering as you don't have the parent symbol and a few other issues that I unfortunately can't recollect right this moment (but can dig back up with some testing if desired).

But in essence, I thought this method might be reasonable as it's been utilised before (albeit in a slightly different scenario) and it was a fair bit simpler than the alternatives. I'd just like to verify if it seemed reasonable to those more familiar with FIR and Fortran :-)! 

The above method results in the following IR when mapping a common block symbol (not individual or multiple members, that is the intent at least): https://github.com/llvm/llvm-project/pull/91829/files#diff-291898b3de7308afe65947c1029376201d17c7598f96adc09d43548cd9364548R9 

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