jdenny added a comment. In D86119#2330339 <https://reviews.llvm.org/D86119#2330339>, @ABataev wrote:
> In D86119#2330310 <https://reviews.llvm.org/D86119#2330310>, @jdenny wrote: > >> Thanks for working on this. Sorry to take so long to review. Before I try >> to digest the code, I have a few high-level questions. >> >> Based on the test suite changes, `TARGET_PARAM` is disappearing from many >> cases. Can you explain a bit how that supports overlapping and reordering? > > `TARGET_PARAM` is used only for marking the data that should be passed to the > kernel function as an argument. We just generate it in many cases but the > runtime actually does not use them. Thу patch relies on this thing, > otherwise, we may pass an incorrect number of arguments to the kernel > functions. Is it reasonable to extract that change into a parent patch? That would at least make the test suite changes easier to follow. >> Have you considered issue 2337 for the OpenMP spec and how your >> implementation handles the ambiguous cases cited there? > > Can you provide the details about this issue? It discusses ambiguities in the way the spec describes map clause ordering. Here's one example relevant to `present`: #pragma omp target exit data map(from: x) map(present, release: x) One statement in the spec says a map clause with `from` is effectively ordered before one with `release`. Another statement says a map clause with `present` is effectively ordered before one without. Which applies in the above example? In some discussions, people agreed the statement about `present` was intended to have precedence, but the spec doesn't say that. That resolution probably makes sense at entry to a region, but does it make sense at exit? Would it suppress `from` in this example? (Actually, should there be two reference counter decrements in this example or just one?) These ambiguities are the reason I punted on this issue when implementing `present`. If we can come up with a reasonable implementation for all cases, perhaps we can propose a new wording for the spec. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86119/new/ https://reviews.llvm.org/D86119 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
