[flang-commits] [flang] [Flang][OpenMP] Optimize target updates of derived-type scalars (PR #219488)

[via flang-commits]

https://github.com/agozillon commented:

Could we please have a more detailed breakdown of the intent here, I believe I understand it, but just on the off-chance I am missing something I'd love a description if that's reasonable! From what I can gather we're basically generating a target region which we send a packed array of our non-contiguous scalars from the derived type to, alongside map arguments for the corresponding original copies of the variable that are hopefully already on device, so we can then update them directly using load/stores, which seems reasonable enough. I wonder if firstprivatizing the scalar payload in this case would increase the performance further as they'd be batched alongside the target launch payload, probably negligible but would be interesting to see.  

There's a good chance I missed these things, so sorry if they're in there and I missed it. Do we remove the scalar maps we're now assigning via target region from the update to the target region? Would it make sense to verify if the scalars (and other mapped components) being mapped make up a contiguous block of the derived type before performing the optimization? 

This seems like something we should also inhibit from being done in USM mode as we're in theory doing extra work in those cases (in theory, as zero-copy mode isn't always available, you could perhaps specialise to the specific cases the runtime switches it on for). In general there's some USM cases that I'd be a bit curious about the interactions with this patch (e.g. if we emit an update carrying a target region before it's ever enter mapped, I actually don't know how this is handled regularly either though). 

There is a scenario where I think this might lead to a slowdown but I could be wrong, in particular if we're updating something that isn't device resident for whatever reason, the update is no longer a "no-op" (quotations as the runtime still has book-keeping), we're now launching a kernel that's doing some processing and actually allocating data on device for each scalar rather than just being ref-count blocked from action. I don't know how common a case this would be, but it does seem to be a downside of this approach.

Does something like this make more sense at the LLVM-IR lowering level as opposed to here? This seems like a generally applicable optimization to other languages, perhaps it's not so easy to do at that level though, there's a lot more legwork to get the kernel built, a middle-ground might be an OpenMP dialect pass.  

Another, approach could be to batch the update maps for the derived type into contiguous blocks/slices to shrink the overall number of maps, the upside would be this would likely work for more than just scalars, the downside would be a user could still cripple themselves by making their mapping incredibly porous, e.g. with elements a through g, they could do map(a,c,e,g) and there isn't anything we can do in this scenario (outside of your selected method).

Gonna tag Michael incase he can concoct any scenario that this might break some behavior. I can't think of any off the top of my head other than the aforementioned USM stuff :-)

Long story short, I'm fine with the approach minus nits and questions, however, could we have a more in-depth FIR lowering test that uses a slightly more elaborate update map with a number of non-contiguous scalars (3-4) and then verify the entirety of the target region and it's inputs, plus the target_update map. Annoying I know, sorry about that, but it's a good illustrating example and helps make sure it isn't inadvertently broken in future changes. 

Thank you very much for the PR @TIFitis and looking into this!




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