[PATCH] D133694: [Clang][OpenMP] Fix use_device_addr

[Deepak Eachempati via Phabricator via cfe-commits]

dreachem added inline comments.



Comment at: clang/test/OpenMP/target_data_use_device_addr_codegen_ptr.cpp:14
+{
+#pragma omp target data use_device_addr(x)
+{

ye-luo wrote:
> doru1004 wrote:
> > doru1004 wrote:
> > > ye-luo wrote:
> > > > doru1004 wrote:
> > > > > ye-luo wrote:
> > > > > > doru1004 wrote:
> > > > > > > ye-luo wrote:
> > > > > > > > doru1004 wrote:
> > > > > > > > > ye-luo wrote:
> > > > > > > > > > In my understanding of the spec.
> > > > > > > > > > `map(tofrom:x[0:256])` only maps the memory segment that x 
> > > > > > > > > > points to. x itself as a pointer scalar is not mapped.
> > > > > > > > > > use_device_addr(x) should fail to find the map of x scalar.
> > > > > > > > > > 5.2 spec.
> > > > > > > > > > If the list item is not a mapped list item, it is assumed 
> > > > > > > > > > to be accessible on the target device.
> > > > > > > > > > To me, it seems just keep &x as it was, in this case &x 
> > > > > > > > > > remains a host address.
> > > > > > > > > > 
> > > > > > > > > > But in your patch description, it seems treating x 
> > > > > > > > > > differently from a scalar.
> > > > > > > > > > 
> > > > > > > > > > I also applied your patch on main and got segfault because 
> > > > > > > > > > the x has a value of device address and x[0] fails. This 
> > > > > > > > > > should be the behavior of use_device_ptr instead of 
> > > > > > > > > > use_device_addr.
> > > > > > > > > > To me, it seems just keep &x as it was, in this case &x 
> > > > > > > > > > remains a host address.
> > > > > > > > > 
> > > > > > > > > So does this mean that if I do something like this in the 
> > > > > > > > > target data I should get different addresses for x:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > ```
> > > > > > > > > #pragma omp target data use_device_ptr(x)
> > > > > > > > > {
> > > > > > > > > fprintf(stderr, "x: %p\n", __LINE__, x);
> > > > > > > > > }
> > > > > > > > > 
> > > > > > > > > #pragma omp target data use_device_addr(x)
> > > > > > > > > {
> > > > > > > > > fprintf(stderr, "x: %p\n", __LINE__, x);
> > > > > > > > > }
> > > > > > > > > ```
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > > I also applied your patch on main and got segfault because 
> > > > > > > > > > the x has a value of device address and x[0] fails.
> > > > > > > > > 
> > > > > > > > > That's my fault x[0] was the wrong thing to use actually.
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > When you have an outer `target data map(x)`, then two printf 
> > > > > > > > differ. If there is no outer `map(x)`, two printf should be 
> > > > > > > > identical.
> > > > > > > > When you have an outer `target data map(x)`, then two printf 
> > > > > > > > differ. If there is no outer `map(x)`, two printf should be 
> > > > > > > > identical.
> > > > > > > 
> > > > > > > This is super helpful thank you! I'll make sure that happens.
> > > > > > > 
> > > > > > > In the case when an outer target data exists, the print of the x 
> > > > > > > which is under use_device_addr should print the same address as 
> > > > > > > printing x on the host? 
> > > > > > I need a correction. When outer map(x) exists, actually the 
> > > > > > address(not value) of x should be a device address, and the code 
> > > > > > cannot even print x. Printing &x should be fine.
> > > > > In the context of the above comment, should &x on the device be an 
> > > > > address I can verify, somehow, to make sure that it's correct or is 
> > > > > it a completely new device address?
> > > > > 
> > > > > So for example, should it be the same as when I do a use_device_ptr 
> > > > > but print the &x in that case? (With the current master those two 
> > > > > addresses are not the same.)
> > > > > 
> > > > > I guess what I need is an example of using use_device_addr that 
> > > > > actually does something meaningful because with the current main 
> > > > > branch printing the &x of a use_device_addr(x) is nil.
> > > > When an outer map(x) is placed, &x does print something meaningful.
> > > > I tried to access `omp_get_mapped_ptr(&x, omp_get_default_device())` 
> > > > but got link time error about missing omp_get_mapped_ptr definition. It 
> > > > seems missing an implementation of this OpenMP API.
> > > > 
> > > > When there is no map(x), I also got nil, I think this is a bug, &x 
> > > > should keep the host value.
> > > > 
> > > > I cannot think of a useful example with use_device_addr(x) where x is a 
> > > > pointer. But x can be a scalar float. 
> > > > and then call cublas gemm, the alpha/beta parameter can be &x.
> > > > When an outer map(x) is placed, &x does print something meaningful.
> > > 
> > > For me, in the same scenario, it prints nil.
> > > 
> > > Here's the full example to avoid any confusion:
> > > 
> > > ```
> > > float *x = (float *) malloc(10*sizeof(float));
> > > 
> > > #pragma omp target

[PATCH] D119440: [OpenMP][NFC] update status for 5.1 'nothing' directive to 'worked on'

[Deepak Eachempati via Phabricator via cfe-commits]

dreachem created this revision.
dreachem added reviewers: cchen, jdoerfert, koops.
Herald added subscribers: guansong, yaxunl.
dreachem requested review of this revision.
Herald added subscribers: cfe-commits, sstefan1.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D119440

Files:
  clang/docs/OpenMPSupport.rst


Index: clang/docs/OpenMPSupport.rst
===
--- clang/docs/OpenMPSupport.rst
+++ clang/docs/OpenMPSupport.rst
@@ -339,7 +339,7 @@
 
+--+--+--+---+
 | misc extension   | assume and assumes directives 
   | :part:`worked on`| 
  |
 
+--+--+--+---+
-| misc extension   | nothing directive 
   | :none:`unclaimed`| 
  |
+| misc extension   | nothing directive 
   | :part:`worked on`| 
  |
 
+--+--+--+---+
 | misc extension   | masked construct and related combined 
constructs | :part:`worked on`| D5, D100514 
  |
 
+--+--+--+---+


Index: clang/docs/OpenMPSupport.rst
===
--- clang/docs/OpenMPSupport.rst
+++ clang/docs/OpenMPSupport.rst
@@ -339,7 +339,7 @@
 +--+--+--+---+
 | misc extension   | assume and assumes directives| :part:`worked on`|   |
 +--+--+--+---+
-| misc extension   | nothing directive| :none:`unclaimed`|   |
+| misc extension   | nothing directive| :part:`worked on`|   |
 +--+--+--+---+
 | misc extension   | masked construct and related combined constructs | :part:`worked on`| D5, D100514   |
 +--+--+--+---+
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91944: OpenMP 5.0 metadirective

[Deepak Eachempati via Phabricator via cfe-commits]

dreachem added a comment.

In D91944#2414364 , @jdoerfert wrote:

> This looks close to an OpenMP 5.0 implementation. I left comments inlined.
>
> We need tests that show how non-selected alternatives *do not* impact the 
> program. As an example, a template instantiation inside of a non-selected 
> alternative is not actually performed.
>
> We also need test with ill-formed metadirectives.

@jdoerfert I'm still trying to understand this thing regarding template 
instantiations. The spec says that a directive variant associated with a when 
clause can only affect the program if it is a dynamic replacement candidate. I 
had assumed this is referring to the runtime behavior of the program, and not 
(for instance) whether a compiler error is emitted due to a failing static 
assert.

Also, in order to determine what are the dynamic replacement candidates, and 
their order, all specified score expressions would need to be evaluated.  Then, 
you'd need to evaluate the static context selectors, in decreasing order of 
their score, to find which one is the last dynamic replacement candidate. So I 
think template instantiations could be possible for those expressions, even if 
the corresponding variants aren't selected?




Comment at: clang/include/clang/AST/StmtOpenMP.h:373
+///
+class OMPMetaDirective final : public OMPExecutableDirective {
+  friend class ASTStmtReader;

ABataev wrote:
> alokmishra.besu wrote:
> > ABataev wrote:
> > > I think, metadirective should be a kind of a container for different 
> > > sub-directives. The problem is that that subdirectives could be 
> > > completely different, they may capture different variables, using 
> > > different capture kind (by value or by reference) etc.So, you need to 
> > > generate each possible sub-directive independently and store them in the 
> > > meta directive node. Otherwise you won't be able to generate the code 
> > > correctly.
> > In OpenMP 5.0, we do not need to generate every sub-directive. Rather we 
> > need to select one (or none) directive and replace metadirective with it. 
> > So this is not needed.
> > Yes with future specifications we will need to include a list of all valid 
> > directives which need to be resolved at runtime. That is when we will need 
> > to generate and store multiple sub-directives inside the OMPMetaDirective 
> > class.  
> I think you still need to do it even for 5.0. I don't mean y need to emit the 
> code for every sub-directive, but to generate them in the AST. Even in the 
> example above, `when(user={condition(N>10)}` is used in the template and `N` 
> is a template argument, the chosen directive can be selected on the 
> instantiation and, thus, the original templated directive has to contain all 
> possible sub-directives.
r


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91944

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

[PATCH] D90802: [OpenMP] [DOCS] Update OMP5.1 feature status table [NFC]

[Deepak Eachempati via Phabricator via cfe-commits]

dreachem created this revision.
dreachem added reviewers: kkwli0, jdoerfert, ABataev, RaviNarayanaswamy.
Herald added subscribers: cfe-commits, jfb, guansong, yaxunl.
Herald added a project: clang.
dreachem requested review of this revision.
Herald added a subscriber: sstefan1.

Adding features in OpenMP 5.1 specification, as documented in feature change 
history, to the 5.1 table. I alphabetized the rows of the table according to 
the category. For deprecating master construct, I just used 'other' as the 
category.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90802

Files:
  clang/docs/OpenMPSupport.rst

Index: clang/docs/OpenMPSupport.rst
===
--- clang/docs/OpenMPSupport.rst
+++ clang/docs/OpenMPSupport.rst
@@ -264,15 +264,99 @@
 +--+--+--+---+
 |Category  | Feature  | Status   | Reviews   |
 +==+==+==+===+
-| misc extension   | user-defined function variants with #ifdef protection| :part:`worked on`| D71179|
-+--+--+--+---+
-| misc extension   | default(firstprivate) & default(private) | :part:`partial`  | firstprivate done: D75591 |
+| atomic extension | 'compare' and 'fail' clauses on atomic construct | :none:`unclaimed`|   |
 +--+--+--+---+
-| loop extension   | Loop tiling transformation   | :part:`worked on`| D76342|
+| base language| C++ attribute specifier syntax   | :none:`unclaimed`|   |
 +--+--+--+---+
 | device extension | 'present' map type modifier  | :good:`done` | D83061, D83062, D84422|
 +--+--+--+---+
 | device extension | 'present' motion modifier| :good:`done` | D84711, D84712|
 +--+--+--+---+
+| device extension | 'present' in defaultmap clause   | :none:`unclaimed`|   |
++--+--+--+---+
 | device extension | map clause reordering reordering based on 'present' modifier | :none:`unclaimed`|   |
 +--+--+--+---+
+| device extension | device-specific environment variables| :none:`unclaimed`|   |
++--+--+--+---+
+| device extension | omp_target_is_accessib

[PATCH] D90802: [OpenMP] [DOCS] Update OMP5.1 feature status table [NFC]

[Deepak Eachempati via Phabricator via cfe-commits]

dreachem added inline comments.



Comment at: clang/docs/OpenMPSupport.rst:343
++--+--+--+---+
+| misc extension   | user-defined function variants with #ifdef 
protection| :part:`worked on`| D71179   
 |
++--+--+--+---+

jdoerfert wrote:
> What is missing here?
This is from the original table. Is this referring to begin/end declare variant 
feature, which I also added? If so, I should remove that one. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90802

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

[PATCH] D90802: [OpenMP] [DOCS] Update OMP5.1 feature status table [NFC]

[Deepak Eachempati via Phabricator via cfe-commits]

dreachem updated this revision to Diff 303258.
dreachem added a comment.

Marking the "begin/end declare variant" feature as 'done' in the 5.1 table.


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

https://reviews.llvm.org/D90802

Files:
  clang/docs/OpenMPSupport.rst

Index: clang/docs/OpenMPSupport.rst
===
--- clang/docs/OpenMPSupport.rst
+++ clang/docs/OpenMPSupport.rst
@@ -264,15 +264,97 @@
 +--+--+--+---+
 |Category  | Feature  | Status   | Reviews   |
 +==+==+==+===+
-| misc extension   | user-defined function variants with #ifdef protection| :part:`worked on`| D71179|
+| atomic extension | 'compare' and 'fail' clauses on atomic construct | :none:`unclaimed`|   |
 +--+--+--+---+
-| misc extension   | default(firstprivate) & default(private) | :part:`partial`  | firstprivate done: D75591 |
-+--+--+--+---+
-| loop extension   | Loop tiling transformation   | :part:`worked on`| D76342|
+| base language| C++ attribute specifier syntax   | :none:`unclaimed`|   |
 +--+--+--+---+
 | device extension | 'present' map type modifier  | :good:`done` | D83061, D83062, D84422|
 +--+--+--+---+
 | device extension | 'present' motion modifier| :good:`done` | D84711, D84712|
 +--+--+--+---+
+| device extension | 'present' in defaultmap clause   | :none:`unclaimed`|   |
++--+--+--+---+
 | device extension | map clause reordering reordering based on 'present' modifier | :none:`unclaimed`|   |
 +--+--+--+---+
+| device extension | device-specific environment variables| :none:`unclaimed`|   |
++--+--+--+---+
+| device extension | omp_target_is_accessible routine | :none:`unclaimed`|   |
++--+--+--+---+
+| device

[PATCH] D90802: [OpenMP] [DOCS] Update OMP5.1 feature status table [NFC]

[Deepak Eachempati via Phabricator via cfe-commits]

dreachem added a comment.

I don't have commit access. @jdoerfert, please commit the change if you think 
it's ready. Thanks.


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

https://reviews.llvm.org/D90802

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

[PATCH] D113359: [Libomptarget][WIP] Introduce VGPU Plugin

[Deepak Eachempati via Phabricator via cfe-commits]

dreachem added subscribers: dhruvachak, dreachem.
dreachem added a comment.
Herald added a subscriber: MaskRay.
Herald added a project: All.

@jdoerfert @tianshilei1992 @atmnpatel @dhruvachak

Is the target to get this merged in for LLVM 16? Does the VGPU implementation 
provide a way to support OMPT callbacks for various constructs (parallel, 
worksharing, barriers, etc.)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113359

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

[PATCH] D69923: [OPENMP][DOCS] Update OpenMP status (NFC)

[Deepak Eachempati via Phabricator via cfe-commits]

dreachem created this revision.
dreachem added reviewers: ABataev, hfinkel, jdoerfert, kkwli0.
Herald added subscribers: cfe-commits, guansong.
Herald added a project: clang.

This is updating the OpenMP status table. Cray has volunteered for `defaultmap` 
and supporting `in_reduction` on the `target` construct, so the status on those 
entries from was changed from "unclaimed". Also, a new entry was added for 
supporting non-contiguous arrays sections on the `target update` directive.


Repository:
  rC Clang

https://reviews.llvm.org/D69923

Files:
  clang/docs/OpenMPSupport.rst


Index: clang/docs/OpenMPSupport.rst
===
--- clang/docs/OpenMPSupport.rst
+++ clang/docs/OpenMPSupport.rst
@@ -197,13 +197,13 @@
 
+--+--+--+---+
 | device extension | OMP_TARGET_OFFLOAD environment variable   
   | :good:`done` | D50522  
  |
 
+--+--+--+---+
-| device extension | support full 'defaultmap' functionality   
   | :part:`worked on`| 
  |
+| device extension | support full 'defaultmap' functionality   
   | :part:`worked on`| D69204  
  |
 
+--+--+--+---+
 | device extension | device specific functions 
   | :none:`unclaimed`| 
  |
 
+--+--+--+---+
 | device extension | clause: device_type   
   | :good:`done` | 
  |
 
+--+--+--+---+
-| device extension | clause: in_reduction  
   | :none:`unclaimed`| r308768 
  |
+| device extension | clause: in_reduction  
   | :part:`worked on`| r308768 
  |
 
+--+--+--+---+
 | device extension | omp_get_device_num()  
   | :part:`worked on`| D54342  
  |
 
+--+--+--+---+
@@ -235,6 +235,8 @@
 
+--+--+--+---+
 | device extension | teams construct on the host device
   | :part:`worked on`| Clang part is done, r371553.
  |
 
+--+--+--+---+
+| device extension | support non-contiguous array sections for 
target update  | :part:`worked on`| 
  |
++--+--+--+---+
 | atomic extension | hints for the atomic construct
   | :part:`worked on`| D51233  
  |
 
+--+-

[PATCH] D69923: [OPENMP][DOCS] Update OpenMP status (NFC)

[Deepak Eachempati via Phabricator via cfe-commits]

dreachem added a comment.

I don't believe I have commit permissions on this. @ABataev, can you commit 
this? Thanks.


Repository:
  rC Clang

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

https://reviews.llvm.org/D69923



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

[PATCH] D72901: [OpenMP] [DOCS] Update OMP5.0 feature status table [NFC]

[Deepak Eachempati via Phabricator via cfe-commits]

dreachem added inline comments.



Comment at: clang/docs/OpenMPSupport.rst:210
 
+--+--+--+---+
-| device extension | map(replicate) or map(local) when requires 
unified_shared_me | :part:`worked on`| D55719,D55892
 |
+| device extension | map(local) when requires 
unified_shared_memory   | :part:`worked on`| D55719,D55892  
   |
 
+--+--+--+---+

ABataev wrote:
> I assume, it is `done`.
I think the description here should be something like "support close modifier 
on map clause". There is no "local" option for the map clause, and also the use 
of "close" is not dependent on the specification of the unified_shared_memory 
requirement.


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

https://reviews.llvm.org/D72901



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

[PATCH] D115901: [OpenMP][NFC] update status for 5.1 'fail' atomic extension

[Deepak Eachempati via Phabricator via cfe-commits]

dreachem created this revision.
dreachem added reviewers: jdoerfert, koops, cchen.
Herald added subscribers: guansong, yaxunl.
dreachem requested review of this revision.
Herald added subscribers: cfe-commits, sstefan1.
Herald added a project: clang.

Update status for the atomic 'fail' clause to "worked on".


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D115901

Files:
  clang/docs/OpenMPSupport.rst


Index: clang/docs/OpenMPSupport.rst
===
--- clang/docs/OpenMPSupport.rst
+++ clang/docs/OpenMPSupport.rst
@@ -266,7 +266,7 @@
 
+==+==+==+===+
 | atomic extension | 'compare' clause on atomic construct  
   | :good:`worked on`| 
  |
 
+--+--+--+---+
-| atomic extension | 'fail' clause on atomic construct 
   | :none:`unclaimed`| 
  |
+| atomic extension | 'fail' clause on atomic construct 
   | :part:`worked on`| 
  |
 
+--+--+--+---+
 | base language| C++ attribute specifier syntax
   | :good:`done` | D105648 
  |
 
+--+--+--+---+


Index: clang/docs/OpenMPSupport.rst
===
--- clang/docs/OpenMPSupport.rst
+++ clang/docs/OpenMPSupport.rst
@@ -266,7 +266,7 @@
 +==+==+==+===+
 | atomic extension | 'compare' clause on atomic construct | :good:`worked on`|   |
 +--+--+--+---+
-| atomic extension | 'fail' clause on atomic construct| :none:`unclaimed`|   |
+| atomic extension | 'fail' clause on atomic construct| :part:`worked on`|   |
 +--+--+--+---+
 | base language| C++ attribute specifier syntax   | :good:`done` | D105648   |
 +--+--+--+---+
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112292: [Clang][OpenMP] Allow loop iteration var with threadprivate directive

[Deepak Eachempati via Phabricator via cfe-commits]

dreachem added a comment.

There still exists a restriction that the loop variable must not be 
threadprivate in OpenMP 5.1. See Canonical Loop Nest restrictions, p125, line 7:

> The loop iteration variable may not appear in a threadprivate directive.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112292

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