[Openmp-commits] [PATCH] D86119: [OPENMP50]Allow overlapping mapping in target constrcuts.
Ye Luo via Phabricator via Openmp-commits
openmp-commits at lists.llvm.org
Fri Dec 4 16:26:53 PST 2020
ye-luo added inline comments.
================
Comment at: openmp/libomptarget/src/omptarget.cpp:233
MapperComponents
- .Components[target_data_function == targetDataEnd ? I : E - I - 1];
+ .Components[target_data_function == targetDataEnd ? E - I - 1 : I];
MapperArgsBase[I] = C.Base;
----------------
ABataev wrote:
> ABataev wrote:
> > ye-luo wrote:
> > > ye-luo wrote:
> > > > ye-luo wrote:
> > > > > ABataev wrote:
> > > > > > ye-luo wrote:
> > > > > > > ABataev wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > ye-luo wrote:
> > > > > > > > > > ye-luo wrote:
> > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > ye-luo wrote:
> > > > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > > > grokos wrote:
> > > > > > > > > > > > > > > What is the current status of the order of the arguments clang emits? Is it still necessary to traverse arguments in reverse order here?
> > > > > > > > > > > > > > Yes, still required
> > > > > > > > > > > > > Based on the conversation in
> > > > > > > > > > > > > https://reviews.llvm.org/D85216
> > > > > > > > > > > > > This line of code neither before nor after the change plays well.
> > > > > > > > > > > > > Shall we fix the order in targetDataEnd first?
> > > > > > > > > > > > This change is part of this patch and cannot be committed separately.
> > > > > > > > > > > I mean could you fix that issue as a parent of this patch?
> > > > > > > > > > > This change is part of this patch and cannot be committed separately.
> > > > > > > > > >
> > > > > > > > > > If fixing the reordering is part of this patch, I should have seen "target_data_function == targetDataEnd ?" branches disappear.
> > > > > > > > > Nope, just with this patch. It reorders the maps and need to change the cleanup order too.
> > > > > > > > It works just like constructors/destructors: allocate in direct order, deallocate in reversed to correctly handle map order.
> > > > > > > The description says that "present and alloc mappings are processed first and then all others."
> > > > > > > Why the order of arguments in targetDataBegin, targetDataEnd and targetDataUpdate all get reversed.
> > > > > > Because this is for mappers. Mapper maps are ordered by the compiler in the direct order (alloc, maps, delete) but when we need to do exit, we need to release the data in reversed order (deletes, maps, allocs).
> > > > > I was not making the question clear. My question about "reverse" is not about having a reverse order for targetDataBegin. My question was about "reversing" from the the old code. Your change put the opposite order for targetDataBegin, targetDataEnd and targetDataUpdate cases.
> > > > > I was not making the question clear. My question about "reverse" is not about having a reverse order for targetDataBegin. My question was about "reversing" from the the old code. Your change put the opposite order for targetDataBegin, targetDataEnd and targetDataUpdate cases.
> > > >
> > > > typo correction
> > > > I was not making the question clear. My question about "reverse" is not about having a reverse order for **targetDataEnd**. My question was about "reversing" from the the old code. Your change put the opposite order for targetDataBegin, targetDataEnd and targetDataUpdate cases.
> > > My separate question specifically for targetDataEnd is the following.
> > >
> > > In target(), we call
> > > ```
> > > targetDataBegin(args)
> > > { // forward order
> > > for (int32_t i = 0; i < arg_num; ++i) { ... }
> > > }
> > > launch_kernels
> > > targetDataEnd(args)
> > > { // reverse order
> > > for (int32_t I = ArgNum - 1; I >= 0; --I) { }
> > > }
> > > ```
> > >
> > > At a mapper,
> > > ```
> > > targetDataMapper
> > > {
> > > // generate args_reverse in reverse order for targetDataEnd
> > > targetDataEnd(args_reverse)
> > > }
> > > ```
> > > Are we actually getting the original forward order due to one reverse in targetDataMapper and second reverse in targetDataEnd? Is this the desired behavior? This part confused me. Do I miss something? Could you explain a bit?
> > Yes, something like this. targetDataEnd reverses the order of mapping arrays. But mapper generator always generates mapping arrays in the direct order (it fills mapping arrays that later processed by the targetDataEnd function). We could fix this by passing extra Boolean flag to the generator function but it means the redesign of the mappers. That's why we have to reverse it in the libomptarget.
> You can check it yourself. Apply the patch, restore the original behavior in libomptarget and run libomptarget tests. Mapper related tests will crash.
Stick with mapper generator always generating mapping arrays in the direct order. The targetDataMapper reverse the mapping array and then passes args_reverse into targetDataEnd. Inside targetDataEnd, mapping
================
Comment at: openmp/libomptarget/src/omptarget.cpp:233
MapperComponents
- .Components[target_data_function == targetDataEnd ? I : E - I - 1];
+ .Components[target_data_function == targetDataEnd ? E - I - 1 : I];
MapperArgsBase[I] = C.Base;
----------------
ye-luo wrote:
> ABataev wrote:
> > ABataev wrote:
> > > ye-luo wrote:
> > > > ye-luo wrote:
> > > > > ye-luo wrote:
> > > > > > ABataev wrote:
> > > > > > > ye-luo wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > ABataev wrote:
> > > > > > > > > > ye-luo wrote:
> > > > > > > > > > > ye-luo wrote:
> > > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > > ye-luo wrote:
> > > > > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > > > > grokos wrote:
> > > > > > > > > > > > > > > > What is the current status of the order of the arguments clang emits? Is it still necessary to traverse arguments in reverse order here?
> > > > > > > > > > > > > > > Yes, still required
> > > > > > > > > > > > > > Based on the conversation in
> > > > > > > > > > > > > > https://reviews.llvm.org/D85216
> > > > > > > > > > > > > > This line of code neither before nor after the change plays well.
> > > > > > > > > > > > > > Shall we fix the order in targetDataEnd first?
> > > > > > > > > > > > > This change is part of this patch and cannot be committed separately.
> > > > > > > > > > > > I mean could you fix that issue as a parent of this patch?
> > > > > > > > > > > > This change is part of this patch and cannot be committed separately.
> > > > > > > > > > >
> > > > > > > > > > > If fixing the reordering is part of this patch, I should have seen "target_data_function == targetDataEnd ?" branches disappear.
> > > > > > > > > > Nope, just with this patch. It reorders the maps and need to change the cleanup order too.
> > > > > > > > > It works just like constructors/destructors: allocate in direct order, deallocate in reversed to correctly handle map order.
> > > > > > > > The description says that "present and alloc mappings are processed first and then all others."
> > > > > > > > Why the order of arguments in targetDataBegin, targetDataEnd and targetDataUpdate all get reversed.
> > > > > > > Because this is for mappers. Mapper maps are ordered by the compiler in the direct order (alloc, maps, delete) but when we need to do exit, we need to release the data in reversed order (deletes, maps, allocs).
> > > > > > I was not making the question clear. My question about "reverse" is not about having a reverse order for targetDataBegin. My question was about "reversing" from the the old code. Your change put the opposite order for targetDataBegin, targetDataEnd and targetDataUpdate cases.
> > > > > > I was not making the question clear. My question about "reverse" is not about having a reverse order for targetDataBegin. My question was about "reversing" from the the old code. Your change put the opposite order for targetDataBegin, targetDataEnd and targetDataUpdate cases.
> > > > >
> > > > > typo correction
> > > > > I was not making the question clear. My question about "reverse" is not about having a reverse order for **targetDataEnd**. My question was about "reversing" from the the old code. Your change put the opposite order for targetDataBegin, targetDataEnd and targetDataUpdate cases.
> > > > My separate question specifically for targetDataEnd is the following.
> > > >
> > > > In target(), we call
> > > > ```
> > > > targetDataBegin(args)
> > > > { // forward order
> > > > for (int32_t i = 0; i < arg_num; ++i) { ... }
> > > > }
> > > > launch_kernels
> > > > targetDataEnd(args)
> > > > { // reverse order
> > > > for (int32_t I = ArgNum - 1; I >= 0; --I) { }
> > > > }
> > > > ```
> > > >
> > > > At a mapper,
> > > > ```
> > > > targetDataMapper
> > > > {
> > > > // generate args_reverse in reverse order for targetDataEnd
> > > > targetDataEnd(args_reverse)
> > > > }
> > > > ```
> > > > Are we actually getting the original forward order due to one reverse in targetDataMapper and second reverse in targetDataEnd? Is this the desired behavior? This part confused me. Do I miss something? Could you explain a bit?
> > > Yes, something like this. targetDataEnd reverses the order of mapping arrays. But mapper generator always generates mapping arrays in the direct order (it fills mapping arrays that later processed by the targetDataEnd function). We could fix this by passing extra Boolean flag to the generator function but it means the redesign of the mappers. That's why we have to reverse it in the libomptarget.
> > You can check it yourself. Apply the patch, restore the original behavior in libomptarget and run libomptarget tests. Mapper related tests will crash.
> Stick with mapper generator always generating mapping arrays in the direct order. The targetDataMapper reverse the mapping array and then passes args_reverse into targetDataEnd. Inside targetDataEnd, mapping
> Yes, something like this. targetDataEnd reverses the order of mapping arrays. But mapper generator always generates mapping arrays in the direct order (it fills mapping arrays that later processed by the targetDataEnd function). We could fix this by passing extra Boolean flag to the generator function but it means the redesign of the mappers. That's why we have to reverse it in the libomptarget.
Stick with mapper generator always generating mapping arrays in the direct order.
In the targetDataBegin case, targetDataMapper keep direct order args and calls targetDataBegin(args) and targetDataBegin process args in direct order.
In the targetDataEnd case, targetDataMapper reverses the mapping array and then passes args_reverse into targetDataEnd. Inside targetDataEnd, args_reverse are processed in reverse order. So targetDataEnd is actually processing the args in original direct order. This seems contradictory to the constructor/deconstructor like behavior that all the mappings must be processed in the actual reverse order in targetDataEnd.
This is my understanding. The current code should be wrong but obviously the current code is working. So why the current code is working? what is inconsistent in my analysis. Could you point out the missing piece.
================
Comment at: openmp/libomptarget/src/omptarget.cpp:233
MapperComponents
- .Components[target_data_function == targetDataEnd ? I : E - I - 1];
+ .Components[target_data_function == targetDataEnd ? E - I - 1 : I];
MapperArgsBase[I] = C.Base;
----------------
ye-luo wrote:
> ye-luo wrote:
> > ABataev wrote:
> > > ABataev wrote:
> > > > ye-luo wrote:
> > > > > ye-luo wrote:
> > > > > > ye-luo wrote:
> > > > > > > ABataev wrote:
> > > > > > > > ye-luo wrote:
> > > > > > > > > ABataev wrote:
> > > > > > > > > > ABataev wrote:
> > > > > > > > > > > ye-luo wrote:
> > > > > > > > > > > > ye-luo wrote:
> > > > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > > > ye-luo wrote:
> > > > > > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > > > > > grokos wrote:
> > > > > > > > > > > > > > > > > What is the current status of the order of the arguments clang emits? Is it still necessary to traverse arguments in reverse order here?
> > > > > > > > > > > > > > > > Yes, still required
> > > > > > > > > > > > > > > Based on the conversation in
> > > > > > > > > > > > > > > https://reviews.llvm.org/D85216
> > > > > > > > > > > > > > > This line of code neither before nor after the change plays well.
> > > > > > > > > > > > > > > Shall we fix the order in targetDataEnd first?
> > > > > > > > > > > > > > This change is part of this patch and cannot be committed separately.
> > > > > > > > > > > > > I mean could you fix that issue as a parent of this patch?
> > > > > > > > > > > > > This change is part of this patch and cannot be committed separately.
> > > > > > > > > > > >
> > > > > > > > > > > > If fixing the reordering is part of this patch, I should have seen "target_data_function == targetDataEnd ?" branches disappear.
> > > > > > > > > > > Nope, just with this patch. It reorders the maps and need to change the cleanup order too.
> > > > > > > > > > It works just like constructors/destructors: allocate in direct order, deallocate in reversed to correctly handle map order.
> > > > > > > > > The description says that "present and alloc mappings are processed first and then all others."
> > > > > > > > > Why the order of arguments in targetDataBegin, targetDataEnd and targetDataUpdate all get reversed.
> > > > > > > > Because this is for mappers. Mapper maps are ordered by the compiler in the direct order (alloc, maps, delete) but when we need to do exit, we need to release the data in reversed order (deletes, maps, allocs).
> > > > > > > I was not making the question clear. My question about "reverse" is not about having a reverse order for targetDataBegin. My question was about "reversing" from the the old code. Your change put the opposite order for targetDataBegin, targetDataEnd and targetDataUpdate cases.
> > > > > > > I was not making the question clear. My question about "reverse" is not about having a reverse order for targetDataBegin. My question was about "reversing" from the the old code. Your change put the opposite order for targetDataBegin, targetDataEnd and targetDataUpdate cases.
> > > > > >
> > > > > > typo correction
> > > > > > I was not making the question clear. My question about "reverse" is not about having a reverse order for **targetDataEnd**. My question was about "reversing" from the the old code. Your change put the opposite order for targetDataBegin, targetDataEnd and targetDataUpdate cases.
> > > > > My separate question specifically for targetDataEnd is the following.
> > > > >
> > > > > In target(), we call
> > > > > ```
> > > > > targetDataBegin(args)
> > > > > { // forward order
> > > > > for (int32_t i = 0; i < arg_num; ++i) { ... }
> > > > > }
> > > > > launch_kernels
> > > > > targetDataEnd(args)
> > > > > { // reverse order
> > > > > for (int32_t I = ArgNum - 1; I >= 0; --I) { }
> > > > > }
> > > > > ```
> > > > >
> > > > > At a mapper,
> > > > > ```
> > > > > targetDataMapper
> > > > > {
> > > > > // generate args_reverse in reverse order for targetDataEnd
> > > > > targetDataEnd(args_reverse)
> > > > > }
> > > > > ```
> > > > > Are we actually getting the original forward order due to one reverse in targetDataMapper and second reverse in targetDataEnd? Is this the desired behavior? This part confused me. Do I miss something? Could you explain a bit?
> > > > Yes, something like this. targetDataEnd reverses the order of mapping arrays. But mapper generator always generates mapping arrays in the direct order (it fills mapping arrays that later processed by the targetDataEnd function). We could fix this by passing extra Boolean flag to the generator function but it means the redesign of the mappers. That's why we have to reverse it in the libomptarget.
> > > You can check it yourself. Apply the patch, restore the original behavior in libomptarget and run libomptarget tests. Mapper related tests will crash.
> > Stick with mapper generator always generating mapping arrays in the direct order. The targetDataMapper reverse the mapping array and then passes args_reverse into targetDataEnd. Inside targetDataEnd, mapping
> > Yes, something like this. targetDataEnd reverses the order of mapping arrays. But mapper generator always generates mapping arrays in the direct order (it fills mapping arrays that later processed by the targetDataEnd function). We could fix this by passing extra Boolean flag to the generator function but it means the redesign of the mappers. That's why we have to reverse it in the libomptarget.
>
> Stick with mapper generator always generating mapping arrays in the direct order.
>
> In the targetDataBegin case, targetDataMapper keep direct order args and calls targetDataBegin(args) and targetDataBegin process args in direct order.
>
> In the targetDataEnd case, targetDataMapper reverses the mapping array and then passes args_reverse into targetDataEnd. Inside targetDataEnd, args_reverse are processed in reverse order. So targetDataEnd is actually processing the args in original direct order. This seems contradictory to the constructor/deconstructor like behavior that all the mappings must be processed in the actual reverse order in targetDataEnd.
>
> This is my understanding. The current code should be wrong but obviously the current code is working. So why the current code is working? what is inconsistent in my analysis. Could you point out the missing piece.
> You can check it yourself. Apply the patch, restore the original behavior in libomptarget and run libomptarget tests. Mapper related tests will crash.
For sure without this line, tests would crash and that is why you included this line of change in the patch. Since you made the change, you could explain why, right?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86119/new/
https://reviews.llvm.org/D86119
More information about the Openmp-commits
mailing list