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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
