[Mlir-commits] [flang] [llvm] [mlir] [Flang][MLIR][OpenMP] Allow setting OMP_MAP_PTR_AND_OBJ by frontends (PR #84328)

[llvmlistbot at llvm.org]

agozillon wrote:

> > While varPtrPtr makes sense for use with descriptors, I am not sure it makes sense for declare target, where the type at a FIR/LLVM dialect level has nothing to do with a pointer/pointee coupling, more to do with the end state of declare target. So, I am not so sure correlating the two things is ideal, there may be cases where the map flag is necessary (i.e. declare target) but varPtrPtr isn't and vice versa.
> 
> I see. I guess it would not make sense for the declare targets. Thanks for the explanation!

That's no problem, the discussion made me think of another way that this PR may be do-able without the addition of OMP_MAP_PTR_AND_OBJ being frontend assignable as well at least for the moment. So thank you very much for that. There's a possibility I will go down that route instead as it's a lesser amount of changes, if that's the case and it works out, I'll close this one for the time being. While I think it's useful to have the flexibility to add this map type via other dialects or frontends, it's likely not worth the added complexity for the time being if it is avoidable, but it could be a requirement in the future.

> 
> > The tracing back was required, as when you are mapping a structure and member pointer pairing (or presumably any member of a structure) the address of the member needs to be part of the originating structure (or in it's address range) to be correctly attached by the runtime (at least that's how I understood the issue at the time, perhaps I misinterpreted it). So, the dislocation of the two mappings (member and parent) to seperate allocations was not ideal. However, this is not the case when using BoxAddrOf.
> 
> I might be wrong, but I think all the information for invoking libomptarget should be available in `varPtr` and `varPtrPtr`, i.e. the corresponding `Args` pointer is the value of in `varPtr` (which is the value of the data pointer potentially offset'ed due to array section), and the `ArgBases` pointer is the value in `varPtrPtr` (which is the address of the member within the data structure, be it a descriptor or a C structure).
> 

This is (I think roughly, with some differences that can be corrected over time) how it works for descriptors currently, the base address map will have both varPtr and varPtrPtr set, however, the varPtr currently is the same as the one set for the descriptor map_info for the moment (wasn't sure what to set it to at the time, we do the offset calculations in MLIR -> LLVM-IR lowering currently as well) and the varPtrPtr is the address of the member within the descriptor data structure (retrieved via `BoxAddrOf`). We currently only use one or the other in the lowering unfortunately, if varPtrPtr is provided we select it for the moment, otherwise we select the varPtr field. This is something that could do with a little work, but there's only one case currently that the varPtrPtr field is utilised so I've been unsure how to better integrate it.

> Okay, I guess I missed my chance to get more details, since there have been a lot of rework, as you said :)

Not at all, I am always happy to make changes when they're suggested, so if you do catch anything you think could be done better please do and I will do my best to incorporate it, if not initially in another iteration :-) 

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