[Openmp-commits] [PATCH] D138389: [OpenMP][libomptarget] Add AMDGPU NextGen plugin with asynchronous behavior

Mon Nov 28 10:29:29 PST 2022

jdoerfert added a comment.

In D138389#3950741 <https://reviews.llvm.org/D138389#3950741>, @JonChesterfield wrote:

>> For functional tests I've used the LLVM OpenMP target tests (`make check`) and miniQMC. For performance experiments, the miniQMC directly. Is there any test suite you use for functional/performance testing?
>
> This plugin introduces new functionality which seems unlikely to be covered by existing make check, particularly given quite a lot of that is disabled on amdgpu anyway.
>
> AMD's aomp and rocm forks have more aggressive runtime testing but presumably don't make heavy use of async given it doesn't exist before this patch.
>
> Code review isn't likely to catch problems in thousands of lines of async c++. Can we split this into a non-functional change where the current plugin is refactored to use the new interface, or equivalently split all the async feature gain out of this to get a more boring alternative plugin which is expected to behave identically to the current one? That lets us distinguish between unintentional behaviour changes (when the boring/refactor doesn't work) and intentional ones (which ideally would have tests).
>
> The big bang replace a large functional unit with a totally new one that behaves differently tends to leave intel and amd's product forks behind, which is unfortunate when upstream testing is relatively superficial and code review struggles to spot bugs in large commits.

1. Intel and AMD not upstreaming their tests is not our problem per se. I'd love for them to do it but they would also need to upstreaming/using the functionality in their plugins. They had 2+ years to do that, "waiting" doesn't help. Maybe if we diverge more they finally have some incentive to work closer to/with upstream.
2. We should not apply two different sets of review rules only to make it easier for AMD. Your initial drop of the "old" plugin (https://reviews.llvm.org/D85742) has all the problems you listed above and more (including and lots of more lines with dead and untested code). Most of the things you argue are bad about this patch are literally states as the state of the "old plugin" in the commit message of D85742 <https://reviews.llvm.org/D85742>. Still, then the sentiment was to merge it "as-is and then improve it in-tree", with an explanation of why that is better.
3. The splits of non-AMD related parts and the actual AMD plugin was already part of my prior review and I'm sure @kevinsala will address the last of those comments soon (=before we merge this).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138389/new/

https://reviews.llvm.org/D138389