[PATCH] D51378: [OPENMP] Add support for nested 'declare target' directives

[Ravi Narayanaswamy via Phabricator via cfe-commits]

RaviNarayanaswamy added a comment.

Is there a way to tell if the header files have matching omp declare target/omp 
end declare target.  
The reason is if one of the header files is missing a matching omp end declare 
target all files which include it will end up having everything marked with 
declare target


Repository:
  rC Clang

https://reviews.llvm.org/D51378



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51378: [OPENMP] Add support for nested 'declare target' directives

[Ravi Narayanaswamy via Phabricator via cfe-commits]

RaviNarayanaswamy added a comment.

We should just go with generating an error if the DeclareTargetNestingLevel is 
not 0 at the end of compilation unit.  
Hard to detect if user accidentally forgot to have end declare in header file 
and had it in the include file or it was intentional.


Repository:
  rC Clang

https://reviews.llvm.org/D51378



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51378: [OPENMP] Add support for nested 'declare target' directives

[Ravi Narayanaswamy via Phabricator via cfe-commits]

RaviNarayanaswamy added a comment.

I did not see the code where check is done if Nestingdepth is 0 at end of 
compilation.


Repository:
  rC Clang

https://reviews.llvm.org/D51378



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51378: [OPENMP] Add support for nested 'declare target' directives

[Ravi Narayanaswamy via Phabricator via cfe-commits]

RaviNarayanaswamy accepted this revision.
RaviNarayanaswamy added a comment.

Ok. 
Thanks.


Repository:
  rC Clang

https://reviews.llvm.org/D51378



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63010: [OpenMP] Add task alloc function

[Ravi Narayanaswamy via Phabricator via cfe-commits]

RaviNarayanaswamy added a comment.

LGTM


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D63010



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64375: [OpenMP][Docs] Provide implementation status details

[Ravi Narayanaswamy via Phabricator via cfe-commits]

RaviNarayanaswamy added inline comments.



Comment at: clang/docs/OpenMPSupport.rst:222-226
+| device extension | clause: reverse_offload   
   | :good:`done` | D52780  
   |
++--+--+--++
+| device extension | clause: atomic_default_mem_order  
   | :good:`done` | D53513  
   |
++--+--+--++
+| device extension | clause: dynamic_allocators
   | :good:`done` | D53079  
   |

ABataev wrote:
> Hahnfeld wrote:
> > Do we have CodeGen for this? I only remember Sema support...
> > 
> > Also, `reverse_offload` is listed again below as `unclaimed`.
> We don't have codegen for this. Same for the reverse_offload and 
> atomic_default_mem_order
As Alexey said codegen is not done so maybe change it to partial.
Regardng reverse_offload, it is debate able whether keep it unclaimed or make 
it partial.  Yes Sema is done but the hard part for  which nobody has claimed 
is codegen and libomptarget.  At least for the getting this initial document 
checked in we should keep it as unclaimed.  Do you have a strong objection 
against this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64375



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

[Ravi Narayanaswamy via Phabricator via cfe-commits]

RaviNarayanaswamy added a comment.

In D84422#2170285 , @grokos wrote:

> What confuses me about this interpretation of the standard is the 
> inconsistency at `data exit`. So if we have an explicit `omp target exit data 
> map(present...)` then we should respect the "present" semantics, whereas when 
> we have a scoped data exit:
>
>   #pragma omp target data map(present,...)
>   {
> ...
>   } // implicit "exit data" here
>
>
> then "present" should be ignored.
>
> I agree that the paragraph from the standard leaves little room for other 
> interpretations, I'd just like to point out that it looks inconsistent - at 
> least to me.


When you  use present on a  variable on a scoped target data region,  you  
cannot delete that object in the scope.  I would say this  is a test case 
error.  It should still be present on exit, checking for that is maybe redundant


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84422



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

[Ravi Narayanaswamy via Phabricator via cfe-commits]

RaviNarayanaswamy added a comment.

In D84422#2170667 , @grokos wrote:

> So is the test case that motivated this patch illegal OpenMP code?
>
>   #pragma omp target enter data map(alloc:i)
>   #pragma omp target data map(present, alloc: i)
>   {
> #pragma omp target exit data map(delete:i) // you cannot delete that 
> object in the scope, illegal code?
>   } // fails presence check here
>   ```Sent mail to OpenMP to clarify.





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84422



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

[Ravi Narayanaswamy via Phabricator via cfe-commits]

RaviNarayanaswamy added a comment.

In D84422#2173372 , @jdenny wrote:

> In D84422#2172898 , @jdenny wrote:
>
> > Has anyone clarified the motivation for this behavior?
>
>
> I meant, is there any insight into why the spec specifies this behavior?
>
> In D84422#2172926 , @grokos wrote:
>
> > Instead of introducing new API functions and making all these changes in 
> > all these files, wouldn't it be easier if we just unset the `PRESENT` flag 
> > from arg_types in clang when we generate the call to 
> > `__tgt_target_data_end_*` if we are exiting from a scoped environment?
>
>
> Ah, that does sound simpler.  Thanks.  I'll look into it.
>
> Suppressing the presence check on exit from `omp target` would require a 
> runtime change in addition to the Clang change you suggest for `omp target 
> data`.  However, I've so far failed to formulate a reasonable test case.  
> Specifically, I don't yet see a way to guarantee that the data will 
> definitely be present at the start of `omp target` but might not be present 
> by the end.  Is it possible?  If not, then maybe we should leave the check in 
> place for `omp target`.


I would rather not have a check if not required by the spec as it would an 
unnecessary overhead to performance.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84422



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79972: [OpenMP5.0] map item can be non-contiguous for target update

[Ravi Narayanaswamy via Phabricator via cfe-commits]

RaviNarayanaswamy added a comment.

How do you plan to support 
#pragma omp target update to (arr[1:2][1:2][0:2], x, b[1:5][0:2])
Are you going to split this into 3 updates since your are using the arg fields.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79972



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106674: Runtime for Interop directive

[Ravi Narayanaswamy via Phabricator via cfe-commits]

RaviNarayanaswamy added inline comments.



Comment at: openmp/libomptarget/src/interop.cpp:194-196
+  if (device_id == -1) {
+device_id = omp_get_default_device();
+  }

Need to check if device is available for all 3 __tgt_interop_init/use/destroy



Comment at: openmp/libomptarget/src/interop.cpp:198-201
+  if (interop_type == kmp_interop_type_tasksync) {
+__kmpc_omp_wait_deps(loc_ref, gtid, ndeps, dep_list, ndeps_noalias,
+ noalias_dep_list);
+  }

Interop object does not wait for any task from libomp.  
Init just initializes the interop object.
The initialization of interop object should  be done by the plugin as only the 
plugin knows what properties are supported



Comment at: openmp/libomptarget/src/interop.cpp:239-242
+  if (interop_val->interop_type == kmp_interop_type_tasksync) {
+__kmpc_omp_wait_deps(loc_ref, gtid, ndeps, dep_list, ndeps_noalias,
+ noalias_dep_list);
+  }

Need to flush the queue if interop object was created with targetsync



Comment at: openmp/libomptarget/src/interop.cpp:260-263
+  if (interop_val->interop_type == kmp_interop_type_tasksync) {
+__kmpc_omp_wait_deps(loc_ref, gtid, ndeps, dep_list, ndeps_noalias,
+ noalias_dep_list);
+  }

You don't wait for the omp tasks.  Need to flush the queue associated with the 
interop through the plugin



Comment at: openmp/runtime/src/kmp_ftn_entry.h:1449-1494
+/// TODO: Include the `omp.h` of the current build
+/* OpenMP 5.1 interop */
+typedef intptr_t omp_intptr_t;
+
+/* 0..omp_get_num_interop_properties()-1 are reserved for 
implementation-defined
+ * properties */
+typedef enum omp_interop_property {

Why do you have all this in openmp/runtime.  Openmp should call libomptarget to 
get interop properties. if libomptarget is not loaded it should return 0


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106674

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106674: Runtime for Interop directive

[Ravi Narayanaswamy via Phabricator via cfe-commits]

RaviNarayanaswamy accepted this revision.
RaviNarayanaswamy added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106674

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157738: [OpenMP] Emit offloading entries for indirect target variables

[Ravi Narayanaswamy via Phabricator via cfe-commits]

RaviNarayanaswamy added a comment.

In D157738#4586465 , @JonChesterfield 
wrote:

>> calling device functions via their associated host pointer
>
> What does this mean? Defining a function foo such that the host and each 
> individual target each have their own machine code for it, such that &foo on 
> the host can be copied over to the target and then invoked to mean call the 
> function on the local target with the same name?
>
> If so, calling through the pointer &foo on the GPU doing a logarithmic search 
> through a table to choose a function address to branch to sounds like 
> something that will codegen into very slow code. Does it do that search on 
> every call?
>
> Is there an ambition to have &foo on the host and &foo on the (each) target 
> return the same value, in the pointer equality sense?
>
> Searching the linked spec for indirect finds the following
>
>> If the indirect clause is present and invoked-by-fptr evaluates to true, any 
>> procedures that appear in a to clause on the directive may be called with an 
>> indirect device invocation. If the indirect clause is present and 
>> invoked-by-fptr does not evaluate to true, any procedures that appear in a 
>> to clause on the directive may not be called with an indirect device 
>> invocation. Unless otherwise specified by an indirect clause, procedures may 
>> not be called with an indirect device invocation.
>
> Which tells me that the indirect clause means procedures can be called with 
> an indirect device invocation. Searching for the expression "indirect device 
> invocation" finds that paragraph and nothing else. So... where does the spec 
> say what this thing is?

Only indirect calls on the device will do a search the table.  The spec does 
not say how it should be implemented.  One could do the  translation at the 
target region when it is mapped on the host but this  will not handle all the  
cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157738

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D157135: [OpenMP][Docs] Update OpenMP supported features table

[Ravi Narayanaswamy via Phabricator via cfe-commits]

RaviNarayanaswamy added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157135

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106674: Runtime for Interop directive

[Ravi Narayanaswamy via Phabricator via cfe-commits]

RaviNarayanaswamy added inline comments.



Comment at: openmp/libomptarget/src/interop.cpp:198-201
+  if (interop_type == kmp_interop_type_tasksync) {
+__kmpc_omp_wait_deps(loc_ref, gtid, ndeps, dep_list, ndeps_noalias,
+ noalias_dep_list);
+  }

jdoerfert wrote:
> RaviNarayanaswamy wrote:
> > Interop object does not wait for any task from libomp.  
> > Init just initializes the interop object.
> > The initialization of interop object should  be done by the plugin as only 
> > the plugin knows what properties are supported
> > Interop object does not wait for any task from libomp. 
> 
> I don't know why you think we would not wait for libomp tasks. If we have 
> dependences we need to wait for them.
> 
> > The initialization of interop object should be done by the plugin as only 
> > the plugin knows what properties are supported.
> 
> It is, below. This is the generic part that then redirects to the plugin.
Libomp would have not invoked the task which calls this routine if there are 
dependences.   They must be executed before the task containing  the interop 
creation is scheduled.

The interop_type should be passed to plugin and let it initialize the common 
for all interop-types and then add based on the interop_type 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106674

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106674: Runtime for Interop directive

[Ravi Narayanaswamy via Phabricator via cfe-commits]

RaviNarayanaswamy added inline comments.



Comment at: openmp/libomptarget/src/interop.cpp:198-201
+  if (interop_type == kmp_interop_type_tasksync) {
+__kmpc_omp_wait_deps(loc_ref, gtid, ndeps, dep_list, ndeps_noalias,
+ noalias_dep_list);
+  }

jdoerfert wrote:
> RaviNarayanaswamy wrote:
> > jdoerfert wrote:
> > > RaviNarayanaswamy wrote:
> > > > Interop object does not wait for any task from libomp.  
> > > > Init just initializes the interop object.
> > > > The initialization of interop object should  be done by the plugin as 
> > > > only the plugin knows what properties are supported
> > > > Interop object does not wait for any task from libomp. 
> > > 
> > > I don't know why you think we would not wait for libomp tasks. If we have 
> > > dependences we need to wait for them.
> > > 
> > > > The initialization of interop object should be done by the plugin as 
> > > > only the plugin knows what properties are supported.
> > > 
> > > It is, below. This is the generic part that then redirects to the plugin.
> > Libomp would have not invoked the task which calls this routine if there 
> > are dependences.   They must be executed before the task containing  the 
> > interop creation is scheduled.
> > 
> > The interop_type should be passed to plugin and let it initialize the 
> > common for all interop-types and then add based on the interop_type 
> > Libomp would have not invoked the task which calls this routine if there 
> > are dependences. They must be executed before the task containing the 
> > interop creation is scheduled.
> 
> To me it seems you are assuming that we create a task in which this routine 
> is called. We do not, as far as I can tell. See D105876.
> 
> > The interop_type should be passed to plugin and let it initialize the 
> > common for all interop-types and then add based on the interop_type
> 
> So what you are trying to say is that `init_device_info` should take the 
> `interop_type` too? That makes sense to me. But as discussed in other reviews 
> recently, we should not extend the API for "future use cases" but extend it 
> as use cases become relevant. For now it seems we can simply set up the 
> `tgt_device_info` part of the `omp_interop_val_t` without knowing the 
> `interop_type`.
Then need to change this code since the interop_type  can be both target_sync 
and  target which will not be handled correctly.  target_sync and target have 
common initialization + additional  property base don the interop_type requested


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106674

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D106674: Runtime for Interop directive

[Ravi Narayanaswamy via Phabricator via cfe-commits]

RaviNarayanaswamy added inline comments.



Comment at: openmp/libomptarget/src/interop.cpp:198-201
+  if (interop_type == kmp_interop_type_tasksync) {
+__kmpc_omp_wait_deps(loc_ref, gtid, ndeps, dep_list, ndeps_noalias,
+ noalias_dep_list);
+  }

jdoerfert wrote:
> RaviNarayanaswamy wrote:
> > jdoerfert wrote:
> > > RaviNarayanaswamy wrote:
> > > > jdoerfert wrote:
> > > > > RaviNarayanaswamy wrote:
> > > > > > Interop object does not wait for any task from libomp.  
> > > > > > Init just initializes the interop object.
> > > > > > The initialization of interop object should  be done by the plugin 
> > > > > > as only the plugin knows what properties are supported
> > > > > > Interop object does not wait for any task from libomp. 
> > > > > 
> > > > > I don't know why you think we would not wait for libomp tasks. If we 
> > > > > have dependences we need to wait for them.
> > > > > 
> > > > > > The initialization of interop object should be done by the plugin 
> > > > > > as only the plugin knows what properties are supported.
> > > > > 
> > > > > It is, below. This is the generic part that then redirects to the 
> > > > > plugin.
> > > > Libomp would have not invoked the task which calls this routine if 
> > > > there are dependences.   They must be executed before the task 
> > > > containing  the interop creation is scheduled.
> > > > 
> > > > The interop_type should be passed to plugin and let it initialize the 
> > > > common for all interop-types and then add based on the interop_type 
> > > > Libomp would have not invoked the task which calls this routine if 
> > > > there are dependences. They must be executed before the task containing 
> > > > the interop creation is scheduled.
> > > 
> > > To me it seems you are assuming that we create a task in which this 
> > > routine is called. We do not, as far as I can tell. See D105876.
> > > 
> > > > The interop_type should be passed to plugin and let it initialize the 
> > > > common for all interop-types and then add based on the interop_type
> > > 
> > > So what you are trying to say is that `init_device_info` should take the 
> > > `interop_type` too? That makes sense to me. But as discussed in other 
> > > reviews recently, we should not extend the API for "future use cases" but 
> > > extend it as use cases become relevant. For now it seems we can simply 
> > > set up the `tgt_device_info` part of the `omp_interop_val_t` without 
> > > knowing the `interop_type`.
> > Then need to change this code since the interop_type  can be both 
> > target_sync and  target which will not be handled correctly.  target_sync 
> > and target have common initialization + additional  property base don the 
> > interop_type requested
> > Then need to change this code since the interop_type can be both 
> > target_sync and target which will not be handled correctly. target_sync and 
> > target have common initialization + additional property base don the 
> > interop_type requested
> 
> Could you please elaborate what needs to be changed exactly. What information 
> is currently not available in the setup as is? What properties would be 
> different?
Should be something like this

// NEED to add _kmp_interop_type_target  to represent interop target
//  interop_ptr->device_info would initialize the following: device handle, 
device_context, platform.
if (interop_type == kmp_interop_type_target) { 
 if (!Device.RTL || !Device.RTL->init_device_info ||
Device.RTL->init_device_info(device_id, &(interop_ptr)->device_info,
 &(interop_ptr)->err_str)) {
  delete interop_ptr;
  interop_ptr = omp_interop_none;
 }
// Add target sync if  request.
if (interop_type == kmp_interop_type_tasksync) {
if (!Device.RTL || !Device.RTL->init_async_info ||
Device.RTL->init_async_info(device_id, &(interop_ptr)->async_info)) {
  delete interop_ptr;
  interop_ptr = omp_interop_none;
  }





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106674

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits