LG

LG

Looks good.

Rebase + added analysis of target register width.

In D62393#1699629, @jlebar wrote:

No, I don't try to add a workaround for the bug in LLVM since there are no problems in LLVM optimizations. The same code works correctly with Cuda9.2. But I would like to add it for cuda8 since we need to fully support it unless we drop the support for cuda8.

Based on this and my comment above, I strongly oppose this patch.

ptxas from CUDA 8 has many, many known bugs. I oppose working around them in LLVM; it will eat up a huge amount of maintainers' time, and you will never cover all or even many of the bugs.

If you want to un-support CUDA 8 in LLVM, fine by me. If in your own personal setup you want to use the ptxas from CUDA 10.1 with the rest of the CUDA 8 toolkit, that should also work.

Fixed title and description, rebase.

In D62393#1699534, @jlebar wrote:

Hi, jumping in here. I haven't read the whole bug, apologies.

ptxas from CUDA 8 has *many* known bugs. I would also strongly oppose attempting to work around them in LLVM.

If you cannot upgrade from CUDA 8, you can still take a newer ptxas binary and use it in combination with the rest of CUDA 8. We have done this at Google for years now with no problems, and with a blessing from nvidia.

If you encounter bugs in the latest ptxas and can provide a reproducer, we can file bugs against nvidia if you cannot. (Well, I guess I'm volunteering tra, I don't work on this anymore. :) I'm not opposed to checking in workarounds for bugs in *the latest* ptxas if we have a process to remove these workarounds soon after a newer ptxas is available (i.e. we don't say, "remove after three ptxas releases" or something). I would strongly oppose keeping workarounds for old ptxas versions because that would greatly complicate the NVPTX backend and have little benefit.

In D62393#1698778, @JonChesterfield wrote:

The byte array should be volatile qualified, as it was in your initial patch. And you should use inline asm or equivalent to prevent memory reordering for ptx. And the comment should probably reflect that the code requires both ptx volatile and the constraint on memory order.

Not again! We don't need to make it atomic,

I didn't say it should be atomic. I said the data should be volatile and the comment updated.

This patch prevents llvm from moving memory accesses across other memory operations by using a memory clobber in the inline assembly. That the asm also contains the word volatile is distracting but irrelevant to the transforms in llvm.

In D62393#1698621, @JonChesterfield wrote:

In D62393#1698559, @ABataev wrote:

It means that you missed something. Initially, patch marked the whole array as volatile but after some discussion it was decided to use ptx instructions directly to avoid some side effects.

I don't think so. Phabricator doesn't make the discussion easy to follow relative to the changes in code, but I'm pretty sure the sequence was:

• Mark the array volatile in the initial patch
• Long discussion about volatile vs atomic
• Agreement that the control flow requires atomic semantics
• Discussion about how to express that in cuda
• Suggestion to "declare the variable volatile, and then use inline asm ld.volatile/st.volatile to increment it"
• Drop the volatile qualification on the array and use inline asm to increment it
• Write a comment saying a volatile access is used to avoid dangerous optimizations
• We start this exchange
  
  The byte array should be volatile qualified, as it was in your initial patch. And you should use inline asm or equivalent to prevent memory reordering for ptx. And the comment should probably reflect that the code requires both ptx volatile and the constraint on memory order.

In D62393#1698531, @JonChesterfield wrote:

as tot he volatile modifi3r, read the whole thread. There was a big discussion about it.

I have read the thread. There's a long discussion about volatile vs atomic. There is no justification for marking the accesses volatile via asm instead of marking the byte array as volatile.

In D62393#1698433, @JonChesterfield wrote:

I'm *very* unhappy to claim "The cuda8 compiler is too optimistic and makes some incorrect optimizations" without actually understanding what the problem is

That seems fair. At the least, the comment could be more specific.

I thought the salient part of the patch was "memory" working around a compiler bug that reordered memory accesses incorrectly. On a closer look though, the comment suggests the fix is actually the .volatile. qualifier on the instruction influencing ptxas. If so, a volatile qualifier on the byte array would presumably also be a fix. Why is the underlying byte array not volatile qualified?

Rebase

Moved target-specific code to target_impl.h

In D62393#1696341, @jdoerfert wrote:

So this changed over time, we for sure need to update the name and maybe commit message.

As @JonChesterfield pointed out, the wrapping of the accesses in function calls is fine.
The two functions with inline assembly should live in the target_impl.h though.

At some point it was said this fixes a problem that only occurs with D62318 applied.
Is this still the case? If not, we need to specify exactly how we can reproduce this issue, e.g., in a comment next to the target offload region.

Yes, it still the case. The cuda8 compiler is too optimistic and makes some incorrect optimizations with D62318. This patch prevents these optimizations. Cuda9 and later have no such problems.

@grokos , Georgios, could you approve the patch if there are no more comments?

Ping!

I'm good with this unless there are no other opinions.

In D68367#1693116, @JonChesterfield wrote:

I think it needs to be initialized to zero to handle the case where there is no work, though I'm not the original author of the patch.

Iirc cuda/nvcc will implicitly zero initialize this and hip/clang does not.

Why do we need this?

In D68355#1691963, @sdmitriev wrote:

In D68355#1691961, @ABataev wrote:

Mark the patch as NFC.

I think I have already done that by adding [NFC] to the commit message. Do I need to do anything in addition to that?

Mark the patch as NFC.

Ping!!!

In D68100#1686480, @lildmh wrote:

In D68100#1686458, @grokos wrote:

Lingda is right, we had faced the same issue in the loop trip count implementation. The loop trip count should be set per task but libomptarget has no notion of tasks, so we ended up engaging the host runtime (libomp) to store per-task information. Although it involves more work, I still believe that will be a more elegant solution.

Btw, I never understand why we have a separate function to push loop trip count. Why is that?

In D68100#1686458, @grokos wrote:

Lingda is right, we had faced the same issue in the loop trip count implementation. The loop trip count should be set per task but libomptarget has no notion of tasks, so we ended up engaging the host runtime (libomp) to store per-task information. Although it involves more work, I still believe that will be a more elegant solution.

In D68100#1686451, @lildmh wrote:

In D68100#1686437, @ABataev wrote:

In D68100#1686432, @lildmh wrote:

Sorry, you are right. I didn't think about the case to always clean up the mappers after finishing using it.

Another possible problem: what if a task is scheduled out after __tgt_mapper and before __tgt_target, for example. I don't think we can keep a per task/thread mapper storage in the current implementation.

tgt_mapper must be called immediately before tgt_target in the same task context.

Yes, but I think it cannot solve this problem. For example, after a task executes __tgt_mapper, it is scheduled out and a new task is scheduled to execute. After the previous task resumes execution, the mapper information it stored has lost, and the execution of the __tgt_target will not get the mapper. I don't think there is a per-task context in libomptarget.

In D68100#1686432, @lildmh wrote:

Sorry, you are right. I didn't think about the case to always clean up the mappers after finishing using it.

Another possible problem: what if a task is scheduled out after __tgt_mapper and before __tgt_target, for example. I don't think we can keep a per task/thread mapper storage in the current implementation.

In D68100#1686416, @lildmh wrote:

In D68100#1686413, @lildmh wrote:

In D68100#1686397, @ABataev wrote:

In D68100#1686395, @lildmh wrote:

In D68100#1686337, @grokos wrote:

In D68100#1686044, @ABataev wrote:

In D68100#1685975, @lildmh wrote:

Please review when you have time, thanks

Maybe, it is better to add a single function to pass the list of mappers to the runtime and use the old functions rather than just duplicate them? Something like __tgt_mappers(...) and store them in the runtime. Plus, modify the original functions to use mappers if they were provided. Thoughts?

I'm in favor of this solution, as it is less intrusive than the one posted in this patch. I think a revised patch will be quite smaller and the changes it introduces will be clearer.

I think Alexey and George's concern is that this patch introduces many new runtime apis. My arguments are:

1. This patch will deprecate the old interfaces as we discussed before in the meeting. E.g., __tgt_target_teams will no longer be used. They are kept there just for legacy code support. So I didn't actually introduce any new runtime apis.
2. If we have something like __tgt_mapper instead, and integrate all mapper functionalities in it, we will need to pass extra arguments to it to distinguish whether it is for __tgt_targt, __tgt_target_data_begin, etc. On the other hand, the current runtime has an interface for each case. For instance, we have __tgt_targt, __tgt_target_data_begin, __tgt_target_data_update_nowait, etc., instead of a single __tgt_target function which does it all. Therefore, I think the current design fits the overall design of OpenMP runtime better: have a function for each case.

Why we will need this extra argument? It just provides a list of mapper functions and stores them in the runtime before we call any __tgt_... function. Each particular __tgt_... runtime function will know what do with all these mappers if they were stored.

Okay, I think I understand your idea now. Then in this case, we will have a call to __tgt_mapper before every call to __tgt_target*, because we need to overwrite the mappers written for previous calls. I don't particularly like this idea, since this will introduce implicit dependencies between different runtime calls and a program will have twice runtime calls. But if most people like it, I'm okay.

I think another problem is this may not work with legacy code, since they don't have calls to __tgt_mapper. This may be a bigger problem when legacy code and code compiled with new Clang are mixed together: a legacy call to __tgt_target may get a mapper which is intended for some new code.

In D68100#1686395, @lildmh wrote:

In D68100#1686337, @grokos wrote:

In D68100#1686044, @ABataev wrote:

In D68100#1685975, @lildmh wrote:

Please review when you have time, thanks

Maybe, it is better to add a single function to pass the list of mappers to the runtime and use the old functions rather than just duplicate them? Something like __tgt_mappers(...) and store them in the runtime. Plus, modify the original functions to use mappers if they were provided. Thoughts?

I'm in favor of this solution, as it is less intrusive than the one posted in this patch. I think a revised patch will be quite smaller and the changes it introduces will be clearer.

I think Alexey and George's concern is that this patch introduces many new runtime apis. My arguments are:

1. This patch will deprecate the old interfaces as we discussed before in the meeting. E.g., __tgt_target_teams will no longer be used. They are kept there just for legacy code support. So I didn't actually introduce any new runtime apis.
2. If we have something like __tgt_mapper instead, and integrate all mapper functionalities in it, we will need to pass extra arguments to it to distinguish whether it is for __tgt_targt, __tgt_target_data_begin, etc. On the other hand, the current runtime has an interface for each case. For instance, we have __tgt_targt, __tgt_target_data_begin, __tgt_target_data_update_nowait, etc., instead of a single __tgt_target function which does it all. Therefore, I think the current design fits the overall design of OpenMP runtime better: have a function for each case.

In D68100#1685975, @lildmh wrote:

Please review when you have time, thanks

In D67978#1684845, @lildmh wrote:

Thanks Alexey! Please check the other 2 mapper patches at https://reviews.llvm.org/D67833 and https://reviews.llvm.org/D68100 when you have time. They should be the last mapper patches.

LG.