[PATCH] D154130: [lit] Avoid os.path.realpath on Windows due to MAX_PATH limitations

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D154130#4502036 , @MrTrillian 
wrote:

> In D154130#4487292 , @jdenny wrote:
>
>> 3. Extend lit's own test suite to cover it.
>
> I submitted an update with your suggestions #1 and #2 but this #3 is much 
> more difficult because of the nature of `%{t:real}`.

Thanks for doing that. I'll try to review more carefully soon, hopefully early 
next week.

> I'm not sure I can have a source/target path that includes symlinks in a way 
> that allows me to test this, and even more so with substitute drives since we 
> can't know which drive letters are unallocated.

Not being a windows person, it's hard for me to answer about substitute drives, 
but I understand your concern.

One possible approach is to verify the relationships among the various flavors 
of a substitution (e.g., `%t`, `%{t:real}`, etc.), and hope (or ensure) that 
some CI configs that run check-lit will use substitute drives for the source 
and/or build directory.  The tests should pass regardless.

A less desirable approach is just to do minimal sanity checks, such as checking 
that the base file name is the same across all flavors of a substitution.

Of course, the least desirable approach is to depend on other subprojects' test 
suites to catch lit bugs.  It's easier for lit developers to identify lit bugs 
when check-lit itself shows them, preferably locally but possibly in CI.

Thanks for taking a look at this.


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

https://reviews.llvm.org/D154130

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

[PATCH] D154130: [lit] Avoid os.path.realpath on Windows due to MAX_PATH limitations

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

3. Extend lit's own test suite to cover it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154130

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

[PATCH] D154130: [lit] Avoid os.path.realpath on Windows due to MAX_PATH limitations

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

If the `%>` feature is going to remain (depending on, for example, the answer 
to @tahonermann's question  about 
modulemap), please:

1. Add it to the lit documentation at 
https://llvm.org/docs/TestingGuide.html#substitutions and 
https://llvm.org/docs/CommandGuide/lit.html#substitutions.  (Aside: I wish 
someone would reconcile those lists.)
2. Consider whether there's a clearer syntax.  For example, we already have 
`%{/t:regex_replacement}`.  What about `%{t:real}` and `%{/t:real}`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154130

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

[PATCH] D151350: [OpenMP] Extend omp teams to permit nested omp tile

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

Ping.


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

https://reviews.llvm.org/D151350

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

[PATCH] D151356: [OpenMP] Fix transformed loop's var privacy

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

Thanks for the review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151356

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

[PATCH] D151356: [OpenMP] Fix transformed loop's var privacy

[Joel E. Denny via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG19841e4dcaab: [OpenMP] Fix transformed loop's var 
privacy (authored by jdenny).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151356

Files:
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/unroll_codegen_parallel_for_factor.cpp
  openmp/libomptarget/test/offloading/target-tile.c

Index: openmp/libomptarget/test/offloading/target-tile.c
===
--- /dev/null
+++ openmp/libomptarget/test/offloading/target-tile.c
@@ -0,0 +1,62 @@
+// Check that omp tile (introduced in OpenMP 5.1) is permitted and behaves when
+// strictly nested within omp target.
+
+// RUN: %libomptarget-compile-generic -fopenmp-version=51
+// RUN: %libomptarget-run-generic 2>&1 | %fcheck-generic
+
+#include 
+
+#define I_NTILES 8
+#define J_NTILES 9
+#define I_NELEMS 2
+#define J_NELEMS 3
+
+int main() {
+  int order[I_NTILES][J_NTILES][I_NELEMS][J_NELEMS];
+  int i, j;
+  #pragma omp target map(tofrom: i, j)
+  {
+int next = 0;
+#pragma omp tile sizes(I_NELEMS, J_NELEMS)
+for (i = 0; i < I_NTILES * I_NELEMS; ++i) {
+  for (j = 0; j < J_NTILES * J_NELEMS; ++j) {
+int iTile = i / I_NELEMS;
+int jTile = j / J_NELEMS;
+int iElem = i % I_NELEMS;
+int jElem = j % J_NELEMS;
+order[iTile][jTile][iElem][jElem] = next++;
+  }
+}
+  }
+  int expected = 0;
+  for (int iTile = 0; iTile < I_NTILES; ++iTile) {
+for (int jTile = 0; jTile < J_NTILES; ++jTile) {
+  for (int iElem = 0; iElem < I_NELEMS; ++iElem) {
+for (int jElem = 0; jElem < J_NELEMS; ++jElem) {
+  int actual = order[iTile][jTile][iElem][jElem];
+  if (expected != actual) {
+printf("error: order[%d][%d][%d][%d] = %d, expected %d\n",
+   iTile, jTile, iElem, jElem, actual, expected);
+return 1;
+  }
+  ++expected;
+}
+  }
+}
+  }
+  // Tiling leaves the loop variables with their values from the final iteration
+  // rather than with the usual +1.
+  expected = I_NTILES * I_NELEMS - 1;
+  if (i != expected) {
+printf("error: i = %d, expected %d\n", i, expected);
+return 1;
+  }
+  expected = J_NTILES * J_NELEMS - 1;
+  if (j != expected) {
+printf("error: j = %d, expected %d\n", j, expected);
+return 1;
+  }
+  // CHECK: success
+  printf("success\n");
+  return 0;
+}
Index: clang/test/OpenMP/unroll_codegen_parallel_for_factor.cpp
===
--- clang/test/OpenMP/unroll_codegen_parallel_for_factor.cpp
+++ clang/test/OpenMP/unroll_codegen_parallel_for_factor.cpp
@@ -21,7 +21,7 @@
 // IR-NEXT:store i32 %[[START:.+]], ptr %[[START_ADDR]], align 4
 // IR-NEXT:store i32 %[[END:.+]], ptr %[[END_ADDR]], align 4
 // IR-NEXT:store i32 %[[STEP:.+]], ptr %[[STEP_ADDR]], align 4
-// IR-NEXT:call void (ptr, i32, ptr, ...) @__kmpc_fork_call(ptr @2, i32 3, ptr @func.omp_outlined, ptr %[[END_ADDR]], ptr %[[STEP_ADDR]], ptr %[[START_ADDR]])
+// IR-NEXT:call void (ptr, i32, ptr, ...) @__kmpc_fork_call(ptr @2, i32 3, ptr @func.omp_outlined, ptr %[[START_ADDR]], ptr %[[END_ADDR]], ptr %[[STEP_ADDR]])
 // IR-NEXT:ret void
 // IR-NEXT:  }
 extern "C" void func(int start, int end, int step) {
@@ -36,9 +36,9 @@
 // IR-NEXT:  [[ENTRY:.*]]:
 // IR-NEXT:%[[DOTGLOBAL_TID__ADDR:.+]] = alloca ptr, align 8
 // IR-NEXT:%[[DOTBOUND_TID__ADDR:.+]] = alloca ptr, align 8
+// IR-NEXT:%[[START_ADDR:.+]] = alloca ptr, align 8
 // IR-NEXT:%[[END_ADDR:.+]] = alloca ptr, align 8
 // IR-NEXT:%[[STEP_ADDR:.+]] = alloca ptr, align 8
-// IR-NEXT:%[[START_ADDR:.+]] = alloca ptr, align 8
 // IR-NEXT:%[[DOTOMP_IV:.+]] = alloca i32, align 4
 // IR-NEXT:%[[TMP:.+]] = alloca i32, align 4
 // IR-NEXT:%[[I:.+]] = alloca i32, align 4
@@ -57,12 +57,12 @@
 // IR-NEXT:%[[DOTUNROLL_INNER_IV_I:.+]] = alloca i32, align 4
 // IR-NEXT:store ptr %[[DOTGLOBAL_TID_:.+]], ptr %[[DOTGLOBAL_TID__ADDR]], align 8
 // IR-NEXT:store ptr %[[DOTBOUND_TID_:.+]], ptr %[[DOTBOUND_TID__ADDR]], align 8
+// IR-NEXT:store ptr %[[START:.+]], ptr %[[START_ADDR]], align 8
 // IR-NEXT:store ptr %[[END:.+]], ptr %[[END_ADDR]], align 8
 // IR-NEXT:store ptr %[[STEP:.+]], ptr %[[STEP_ADDR]], align 8
-// IR-NEXT:store ptr %[[START:.+]], ptr %[[START_ADDR]], align 8
+// IR-NEXT:%[[TMP2:.+]] = load ptr, ptr %[[START_ADDR]], align 8
 // IR-NEXT:%[[TMP0:.+]] = load ptr, ptr %[[END_ADDR]], align 8
 // IR-NEXT:%[[TMP1:.+]] = load ptr, ptr %[[STEP_ADDR]], align 8
-// IR-NEXT:%[[TMP2:.+]] = load ptr, ptr %[[START_ADDR]], align 8
 // IR-NEXT:%[[TMP3:.+]] = load i32, ptr %[[TMP2]], align 4
 // IR-NEXT:store i32 %[[TMP3]], ptr %[[I]], align 4
 // IR-

[PATCH] D151320: [clang] Add `// expected-maybe-no-diagnostics` comment to VerifyDiagnosticConsumer

[Joel E. Denny via Phabricator via cfe-commits]

jdenny accepted this revision.
jdenny added inline comments.



Comment at: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp:468
+Status = VerifyDiagnosticConsumer::HasExpectedMaybeNoDiagnostics;
+  continue;
+} else if (DToken.endswith(DType="-no-diagnostics")) {

Endill wrote:
> jdenny wrote:
> > Endill wrote:
> > > jdenny wrote:
> > > > Endill wrote:
> > > > > jdenny wrote:
> > > > > > Endill wrote:
> > > > > > > jdenny wrote:
> > > > > > > > This `continue` skips the prefix checking below, which is 
> > > > > > > > important when there are multiple prefixes active (e.g., 
> > > > > > > > `-verify=foo,bar`).  That is, any old 
> > > > > > > > `BOGUS-maybe-no-diagnostics` will be effective then.
> > > > > > > This should be fixed now. Thank you for spotting this!
> > > > > > Thanks for the fix.  Please add a test so this bug doesn't pop up 
> > > > > > again.
> > > > > Done as A2 test
> > > > Does A2 trigger the original bug?  I think there must be multiple 
> > > > prefixes for this fix to matter.
> > > I think prefix checking below works for a single prefix as well: 
> > > https://github.com/llvm/llvm-project/blob/5217498dc88aa2de2b728462205ffa8b01d96cab/clang/lib/Frontend/CompilerInvocation.cpp#LL2382C47-L2382C47
> > I downloaded this version of your patch: 
> > .  That version has the 
> > bug discussed in this comment thread.
> > 
> > I applied that patch and added test A2.  A2 passed.  That means A2 does not 
> > reproduce the bug it was intended to reproduce.
> > 
> > Then I changed A2 to have `-verify=foo,bar`.  A2 then failed because it 
> > then reproduced the bug.  (And of course with the current version of your 
> > patch, it passes because you've fixed the bug.)
> > 
> > Please add such a test.
> I replaced A2 test with more comprehensive E set of tests.
> 
> Surprisingly to me, `-verify` and `-verify=foo,bar` work quite differently 
> because of `PH.Search()` call 40 lines above. I can't say I have full 
> understanding what's going on, so I wrote a more comprehensive set of tests. 
> Does this address your concern?
> Surprisingly to me, `-verify` and `-verify=foo,bar` work quite differently 
> because of `PH.Search()` call 40 lines above. I can't say I have full 
> understanding what's going on,

That line is optimized for the case of a single prefix: it ignores text without 
that prefix.  If there are multiple prefixes, then it searches for a more 
general pattern and waits until the `std::binary_search` call below to worry 
about the prefix.

> so I wrote a more comprehensive set of tests. Does this address your concern?

Yes, and thanks for the extra testing.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151320

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

[PATCH] D151320: [clang] Add `// expected-maybe-no-diagnostics` comment to VerifyDiagnosticConsumer

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp:468
+Status = VerifyDiagnosticConsumer::HasExpectedMaybeNoDiagnostics;
+  continue;
+} else if (DToken.endswith(DType="-no-diagnostics")) {

Endill wrote:
> jdenny wrote:
> > Endill wrote:
> > > jdenny wrote:
> > > > Endill wrote:
> > > > > jdenny wrote:
> > > > > > This `continue` skips the prefix checking below, which is important 
> > > > > > when there are multiple prefixes active (e.g., `-verify=foo,bar`).  
> > > > > > That is, any old `BOGUS-maybe-no-diagnostics` will be effective 
> > > > > > then.
> > > > > This should be fixed now. Thank you for spotting this!
> > > > Thanks for the fix.  Please add a test so this bug doesn't pop up again.
> > > Done as A2 test
> > Does A2 trigger the original bug?  I think there must be multiple prefixes 
> > for this fix to matter.
> I think prefix checking below works for a single prefix as well: 
> https://github.com/llvm/llvm-project/blob/5217498dc88aa2de2b728462205ffa8b01d96cab/clang/lib/Frontend/CompilerInvocation.cpp#LL2382C47-L2382C47
I downloaded this version of your patch: 
.  That version has the bug 
discussed in this comment thread.

I applied that patch and added test A2.  A2 passed.  That means A2 does not 
reproduce the bug it was intended to reproduce.

Then I changed A2 to have `-verify=foo,bar`.  A2 then failed because it then 
reproduced the bug.  (And of course with the current version of your patch, it 
passes because you've fixed the bug.)

Please add such a test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151320

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

[PATCH] D151320: [clang] Add `// expected-maybe-no-diagnostics` comment to VerifyDiagnosticConsumer

[Joel E. Denny via Phabricator via cfe-commits]

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

Thanks for addressing all my concerns.

LGTM except for a test issue I just commented on.




Comment at: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp:468
+Status = VerifyDiagnosticConsumer::HasExpectedMaybeNoDiagnostics;
+  continue;
+} else if (DToken.endswith(DType="-no-diagnostics")) {

Endill wrote:
> jdenny wrote:
> > Endill wrote:
> > > jdenny wrote:
> > > > This `continue` skips the prefix checking below, which is important 
> > > > when there are multiple prefixes active (e.g., `-verify=foo,bar`).  
> > > > That is, any old `BOGUS-maybe-no-diagnostics` will be effective then.
> > > This should be fixed now. Thank you for spotting this!
> > Thanks for the fix.  Please add a test so this bug doesn't pop up again.
> Done as A2 test
Does A2 trigger the original bug?  I think there must be multiple prefixes for 
this fix to matter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151320

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

[PATCH] D151320: [clang] Add `// expected-maybe-no-diagnostics` comment to VerifyDiagnosticConsumer

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/include/clang/Frontend/VerifyDiagnosticConsumer.h:186-202
+/// Additionally, you can use:
+///
+/// \code
+///   // expected-maybe-no-diagnostics
+/// \endcode
+///
+/// to specify that a file with no "expected-*" comments should pass when no

Thanks for adding documentation.

I feel that this edit makes the behavior a little clearer, and it clarifies 
what happens when "expected-no-diagnostics" and "expected-maybe-no-diagnostics" 
are combined.

Also, the original text had:

> but they do not fail automatically due to a combination of 
> "expected-no-diagnostics" and "expected-*" within the same test

That reads to me like it's ok to combine "expected-no-diagnostics" and 
"expected-*" directives specifying diagnostics.  Hopefully this edit clarifies 
that point.




Comment at: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp:470-471
   NoDiag = true;
   if (D.RegexKind)
 continue;
 }

Shouldn't `expected-maybe-no-diagnostics` have this too so that 
`expected-maybe-no-diagnostics-re` is skipped?  Please add a test.



Comment at: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp:468
+Status = VerifyDiagnosticConsumer::HasExpectedMaybeNoDiagnostics;
+  continue;
+} else if (DToken.endswith(DType="-no-diagnostics")) {

Endill wrote:
> jdenny wrote:
> > This `continue` skips the prefix checking below, which is important when 
> > there are multiple prefixes active (e.g., `-verify=foo,bar`).  That is, any 
> > old `BOGUS-maybe-no-diagnostics` will be effective then.
> This should be fixed now. Thank you for spotting this!
Thanks for the fix.  Please add a test so this bug doesn't pop up again.



Comment at: clang/test/Frontend/verify-maybe-no-diagnostics.c:104
+//  D6-CHECK: error: 'error' diagnostics seen but not expected:
+// D6-CHECK-NEXT:   {{.*}} 'expected-no-diagnostics' directive cannot follow 
other expected directives
+// D6-CHECK-NEXT: 1 error generated.

Endill wrote:
> jdenny wrote:
> > This diagnostic is confusing.  Should we add "except 
> > 'expected-maybe-no-diagnostics'"?
> As I mentioned in another comment, `maybe-no-diagnostics` has the lowest 
> priority, and doesn't have strict and declarative nature, unlike any other 
> directive. That's why it should never be expected (and ideally very rarely 
> used).
> 
> The purpose of all the tests I added is to ensure `expected-no-diagnostic` 
> doesn't affect existing directives and their interaction in any way.
I don't see how that addresses my concern.  Maybe it's because, after the 
latest edits, phab shifted my comment to the wrong test.  I was originally 
commenting on this:

> 'expected-no-diagnostics' directive cannot follow other expected directives

This message is now incorrect.  `expected-no-diagnostics` //can// follow 
`expected-maybe-no-diagnostics`.  What if we reword as follows?

> 'expected-no-diagnostics' directive cannot follow directives that expect 
> diagnostics


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151320

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

[PATCH] D151320: [clang] Add `// expected-maybe-no-diagnostics` comment to VerifyDiagnosticConsumer

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

Thanks for working on this.  I've been wanting something like it too.  In 
downstream work, I've used a hack that seems to accomplish the same thing but 
probably shouldn't: `expected-error 0 {{}}`.

Anyway, please add documentation for the new directive here: 
https://clang.llvm.org/doxygen/classclang_1_1VerifyDiagnosticConsumer.html#details




Comment at: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp:468
+Status = VerifyDiagnosticConsumer::HasExpectedMaybeNoDiagnostics;
+  continue;
+} else if (DToken.endswith(DType="-no-diagnostics")) {

This `continue` skips the prefix checking below, which is important when there 
are multiple prefixes active (e.g., `-verify=foo,bar`).  That is, any old 
`BOGUS-maybe-no-diagnostics` will be effective then.



Comment at: clang/test/Frontend/verify-maybe-no-diagnostics.c:38
+#endif
+
+#ifdef TEST_D1

Please test this case:

```
#ifdef TEST_C3
// expected-maybe-no-diagnostics
#error test_c3
#endif
```

That is, does Clang actually fail in this case as expected because there's an 
error and no corresponding `expected-error` directive?  Or does it ignore the 
error because of the `expected-maybe-no-diagnostics` directive?



Comment at: clang/test/Frontend/verify-maybe-no-diagnostics.c:51-52
+#ifdef TEST_D2
+// expected-maybe-no-diagnostics
+// expected-no-diagnostics
+#error test_d2

So `expected-no-diagnostics` overrides `expected-maybe-no-diagnostics`.  In 
your use case, omitting one or the other is not always possible?



Comment at: clang/test/Frontend/verify-maybe-no-diagnostics.c:104
+//  D6-CHECK: error: 'error' diagnostics seen but not expected:
+// D6-CHECK-NEXT:   {{.*}} 'expected-no-diagnostics' directive cannot follow 
other expected directives
+// D6-CHECK-NEXT: 1 error generated.

This diagnostic is confusing.  Should we add "except 
'expected-maybe-no-diagnostics'"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151320

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

[PATCH] D151356: [OpenMP] Fix transformed loop's var privacy

[Joel E. Denny via Phabricator via cfe-commits]

jdenny created this revision.
jdenny added reviewers: Meinersbur, ABataev.
jdenny added a project: OpenMP.
Herald added subscribers: sunshaoce, zzheng, guansong, yaxunl.
Herald added a project: All.
jdenny requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added subscribers: openmp-commits, cfe-commits, jplehr, sstefan1.
Herald added a project: clang.

Without this patch, the following example crashes Clang:

  #pragma omp target map(i)
  #pragma omp tile sizes(2)
  for (i = 0; i < N; ++i)
;

This patch fixes the crash by changing `Sema::isOpenMPPrivateDecl` not 
to identify `i` as private just because it's the loop variable of a
`tile` construct.

While OpenMP TR11 and earlier do specify privacy for loop variables of
loops *generated* from a `tile` construct, I haven't found text
stating that the original loop variable must be private in the above
example, so this patch leaves it shared.  Even so, it is a bit 
unexpected that value of `i` after the loop is `N - 1` instead of `N`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D151356

Files:
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/unroll_codegen_parallel_for_factor.cpp
  openmp/libomptarget/test/offloading/target-tile.c

Index: openmp/libomptarget/test/offloading/target-tile.c
===
--- /dev/null
+++ openmp/libomptarget/test/offloading/target-tile.c
@@ -0,0 +1,62 @@
+// Check that omp tile (introduced in OpenMP 5.1) is permitted and behaves when
+// strictly nested within omp target.
+
+// RUN: %libomptarget-compile-generic -fopenmp-version=51
+// RUN: %libomptarget-run-generic 2>&1 | %fcheck-generic
+
+#include 
+
+#define I_NTILES 8
+#define J_NTILES 9
+#define I_NELEMS 2
+#define J_NELEMS 3
+
+int main() {
+  int order[I_NTILES][J_NTILES][I_NELEMS][J_NELEMS];
+  int i, j;
+  #pragma omp target map(tofrom: i, j)
+  {
+int next = 0;
+#pragma omp tile sizes(I_NELEMS, J_NELEMS)
+for (i = 0; i < I_NTILES * I_NELEMS; ++i) {
+  for (j = 0; j < J_NTILES * J_NELEMS; ++j) {
+int iTile = i / I_NELEMS;
+int jTile = j / J_NELEMS;
+int iElem = i % I_NELEMS;
+int jElem = j % J_NELEMS;
+order[iTile][jTile][iElem][jElem] = next++;
+  }
+}
+  }
+  int expected = 0;
+  for (int iTile = 0; iTile < I_NTILES; ++iTile) {
+for (int jTile = 0; jTile < J_NTILES; ++jTile) {
+  for (int iElem = 0; iElem < I_NELEMS; ++iElem) {
+for (int jElem = 0; jElem < J_NELEMS; ++jElem) {
+  int actual = order[iTile][jTile][iElem][jElem];
+  if (expected != actual) {
+printf("error: order[%d][%d][%d][%d] = %d, expected %d\n",
+   iTile, jTile, iElem, jElem, actual, expected);
+return 1;
+  }
+  ++expected;
+}
+  }
+}
+  }
+  // Tiling leaves the loop variables with their values from the final iteration
+  // rather than with the usual +1.
+  expected = I_NTILES * I_NELEMS - 1;
+  if (i != expected) {
+printf("error: i = %d, expected %d\n", i, expected);
+return 1;
+  }
+  expected = J_NTILES * J_NELEMS - 1;
+  if (j != expected) {
+printf("error: j = %d, expected %d\n", j, expected);
+return 1;
+  }
+  // CHECK: success
+  printf("success\n");
+  return 0;
+}
Index: clang/test/OpenMP/unroll_codegen_parallel_for_factor.cpp
===
--- clang/test/OpenMP/unroll_codegen_parallel_for_factor.cpp
+++ clang/test/OpenMP/unroll_codegen_parallel_for_factor.cpp
@@ -21,7 +21,7 @@
 // IR-NEXT:store i32 %[[START:.+]], ptr %[[START_ADDR]], align 4
 // IR-NEXT:store i32 %[[END:.+]], ptr %[[END_ADDR]], align 4
 // IR-NEXT:store i32 %[[STEP:.+]], ptr %[[STEP_ADDR]], align 4
-// IR-NEXT:call void (ptr, i32, ptr, ...) @__kmpc_fork_call(ptr @2, i32 3, ptr @func.omp_outlined, ptr %[[END_ADDR]], ptr %[[STEP_ADDR]], ptr %[[START_ADDR]])
+// IR-NEXT:call void (ptr, i32, ptr, ...) @__kmpc_fork_call(ptr @2, i32 3, ptr @func.omp_outlined, ptr %[[START_ADDR]], ptr %[[END_ADDR]], ptr %[[STEP_ADDR]])
 // IR-NEXT:ret void
 // IR-NEXT:  }
 extern "C" void func(int start, int end, int step) {
@@ -36,9 +36,9 @@
 // IR-NEXT:  [[ENTRY:.*]]:
 // IR-NEXT:%[[DOTGLOBAL_TID__ADDR:.+]] = alloca ptr, align 8
 // IR-NEXT:%[[DOTBOUND_TID__ADDR:.+]] = alloca ptr, align 8
+// IR-NEXT:%[[START_ADDR:.+]] = alloca ptr, align 8
 // IR-NEXT:%[[END_ADDR:.+]] = alloca ptr, align 8
 // IR-NEXT:%[[STEP_ADDR:.+]] = alloca ptr, align 8
-// IR-NEXT:%[[START_ADDR:.+]] = alloca ptr, align 8
 // IR-NEXT:%[[DOTOMP_IV:.+]] = alloca i32, align 4
 // IR-NEXT:%[[TMP:.+]] = alloca i32, align 4
 // IR-NEXT:%[[I:.+]] = alloca i32, align 4
@@ -57,12 +57,12 @@
 // IR-NEXT:%[[DOTUNROLL_INNER_IV_I:.+]] = alloca i32, align 4
 // IR-NEXT:store ptr %[[DOTGLOBAL_TID_:.+]], ptr %[[DOTGLOBAL_TID__ADDR]], align 8
 // IR-NEXT:store ptr %[[DOTBO

[PATCH] D151350: [OpenMP] Extend omp teams to permit nested omp tile

[Joel E. Denny via Phabricator via cfe-commits]

jdenny updated this revision to Diff 525250.
jdenny added a comment.

Added phab review number to status doc.


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

https://reviews.llvm.org/D151350

Files:
  clang/docs/OpenMPSupport.rst
  clang/lib/Sema/SemaOpenMP.cpp
  openmp/docs/openacc/OpenMPExtensions.rst
  openmp/libomptarget/test/offloading/target-teams-tile.c
  openmp/runtime/test/teams/teams-tile.c

Index: openmp/runtime/test/teams/teams-tile.c
===
--- /dev/null
+++ openmp/runtime/test/teams/teams-tile.c
@@ -0,0 +1,59 @@
+// Check that omp tile (introduced in OpenMP 5.1) is permitted and behaves when
+// strictly nested within omp teams.  This is an extension to OpenMP 5.2 and is
+// enabled by default.
+
+// RUN: %libomp-compile -fopenmp-version=51
+// RUN: %libomp-run 2>&1 | FileCheck %s
+
+#include 
+#include 
+
+#define NUM_TEAMS_UPPER 256
+#define I_NTILES 8
+#define J_NTILES 9
+#define I_NELEMS 2
+#define J_NELEMS 3
+
+int main() {
+  int numTeams;
+  int order[NUM_TEAMS_UPPER][I_NTILES][J_NTILES][I_NELEMS][J_NELEMS];
+  #pragma omp teams num_teams(NUM_TEAMS_UPPER)
+  {
+int team = omp_get_team_num();
+if (team == 0)
+  numTeams = omp_get_num_teams();
+int next = 0;
+#pragma omp tile sizes(I_NELEMS, J_NELEMS)
+for (int i = 0; i < I_NTILES * I_NELEMS; ++i) {
+  for (int j = 0; j < J_NTILES * J_NELEMS; ++j) {
+int iTile = i / I_NELEMS;
+int jTile = j / J_NELEMS;
+int iElem = i % I_NELEMS;
+int jElem = j % J_NELEMS;
+order[team][iTile][jTile][iElem][jElem] = next++;
+  }
+}
+  }
+  printf("numTeams = %d\n", numTeams);
+  for (int team = 0; team < numTeams; ++team) {
+int expected = 0;
+for (int iTile = 0; iTile < I_NTILES; ++iTile) {
+  for (int jTile = 0; jTile < J_NTILES; ++jTile) {
+for (int iElem = 0; iElem < I_NELEMS; ++iElem) {
+  for (int jElem = 0; jElem < J_NELEMS; ++jElem) {
+int actual = order[team][iTile][jTile][iElem][jElem];
+if (expected != actual) {
+  printf("error: order[%d][%d][%d][%d][%d] = %d, expected %d\n",
+ team, iTile, jTile, iElem, jElem, actual, expected);
+  return 1;
+}
+++expected;
+  }
+}
+  }
+}
+  }
+  // CHECK: success
+  printf("success\n");
+  return 0;
+}
Index: openmp/libomptarget/test/offloading/target-teams-tile.c
===
--- /dev/null
+++ openmp/libomptarget/test/offloading/target-teams-tile.c
@@ -0,0 +1,59 @@
+// Check that omp tile (introduced in OpenMP 5.1) is permitted and behaves when
+// strictly nested within omp target teams.  This is an extension to OpenMP 5.2
+// and is enabled by default.
+
+// RUN: %libomptarget-compile-generic -fopenmp-version=51
+// RUN: %libomptarget-run-generic 2>&1 | %fcheck-generic
+
+#include 
+#include 
+
+#define NUM_TEAMS_UPPER 256
+#define I_NTILES 8
+#define J_NTILES 9
+#define I_NELEMS 2
+#define J_NELEMS 3
+
+int main() {
+  int numTeams;
+  int order[NUM_TEAMS_UPPER][I_NTILES][J_NTILES][I_NELEMS][J_NELEMS];
+  #pragma omp target teams num_teams(NUM_TEAMS_UPPER) map(from : numTeams)
+  {
+int team = omp_get_team_num();
+if (team == 0)
+  numTeams = omp_get_num_teams();
+int next = 0;
+#pragma omp tile sizes(I_NELEMS, J_NELEMS)
+for (int i = 0; i < I_NTILES * I_NELEMS; ++i) {
+  for (int j = 0; j < J_NTILES * J_NELEMS; ++j) {
+int iTile = i / I_NELEMS;
+int jTile = j / J_NELEMS;
+int iElem = i % I_NELEMS;
+int jElem = j % J_NELEMS;
+order[team][iTile][jTile][iElem][jElem] = next++;
+  }
+}
+  }
+  printf("numTeams = %d\n", numTeams);
+  for (int team = 0; team < numTeams; ++team) {
+int expected = 0;
+for (int iTile = 0; iTile < I_NTILES; ++iTile) {
+  for (int jTile = 0; jTile < J_NTILES; ++jTile) {
+for (int iElem = 0; iElem < I_NELEMS; ++iElem) {
+  for (int jElem = 0; jElem < J_NELEMS; ++jElem) {
+int actual = order[team][iTile][jTile][iElem][jElem];
+if (expected != actual) {
+  printf("error: order[%d][%d][%d][%d][%d] = %d, expected %d\n",
+ team, iTile, jTile, iElem, jElem, actual, expected);
+  return 1;
+}
+++expected;
+  }
+}
+  }
+}
+  }
+  // CHECK: success
+  printf("success\n");
+  return 0;
+}
Index: openmp/docs/openacc/OpenMPExtensions.rst
===
--- openmp/docs/openacc/OpenMPExtensions.rst
+++ openmp/docs/openacc/OpenMPExtensions.rst
@@ -138,10 +138,10 @@
 OpenMP's dynamic reference count, and OpenACC's structured reference
 count is our OpenMP hold reference count extension.
 
-.. _atomicWithinTeams:
+.. _withinTeams:
 
-``atomic`` Strictly Nested Within ``teams``
---

[PATCH] D151350: [OpenMP] Extend omp teams to permit nested omp tile

[Joel E. Denny via Phabricator via cfe-commits]

jdenny created this revision.
jdenny added reviewers: Meinersbur, ABataev, jhuber6, jdoerfert.
jdenny added projects: OpenMP, OpenACC.
Herald added subscribers: sunshaoce, guansong, yaxunl.
Herald added a reviewer: clementval.
Herald added a project: All.
jdenny requested review of this revision.
Herald added subscribers: openmp-commits, cfe-commits, jplehr, sstefan1.
Herald added a project: clang.

OpenMP 5.2, sec. 10.2 "teams Construct", p. 232, L9-12 restricts what
regions can be strictly nested within a `teams` construct.  This patch
relaxes Clang's enforcement of this restriction in the case of nested
`tile` constructs unless `-fno-openmp-extensions` is specified.  Cases
like the following then seem to work fine with no additional
implementation changes:

  #pragma omp target teams
  #pragma omp tile sizes(N, M)
  for (int i = 0; i < I; ++i) {
for (int j = 0; j < J; ++j) {
  ...
}
  }

This commit is similar to D126323  
(48ca3a5ebb15 
) plus 
D126547 
(4a368136693b 
), which 
relaxed the restriction for an `atomic`
construct in a `teams` construct.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D151350

Files:
  clang/docs/OpenMPSupport.rst
  clang/lib/Sema/SemaOpenMP.cpp
  openmp/docs/openacc/OpenMPExtensions.rst
  openmp/libomptarget/test/offloading/target-teams-tile.c
  openmp/runtime/test/teams/teams-tile.c

Index: openmp/runtime/test/teams/teams-tile.c
===
--- /dev/null
+++ openmp/runtime/test/teams/teams-tile.c
@@ -0,0 +1,59 @@
+// Check that omp tile (introduced in OpenMP 5.1) is permitted and behaves when
+// strictly nested within omp teams.  This is an extension to OpenMP 5.2 and is
+// enabled by default.
+
+// RUN: %libomp-compile -fopenmp-version=51
+// RUN: %libomp-run 2>&1 | FileCheck %s
+
+#include 
+#include 
+
+#define NUM_TEAMS_UPPER 256
+#define I_NTILES 8
+#define J_NTILES 9
+#define I_NELEMS 2
+#define J_NELEMS 3
+
+int main() {
+  int numTeams;
+  int order[NUM_TEAMS_UPPER][I_NTILES][J_NTILES][I_NELEMS][J_NELEMS];
+  #pragma omp teams num_teams(NUM_TEAMS_UPPER)
+  {
+int team = omp_get_team_num();
+if (team == 0)
+  numTeams = omp_get_num_teams();
+int next = 0;
+#pragma omp tile sizes(I_NELEMS, J_NELEMS)
+for (int i = 0; i < I_NTILES * I_NELEMS; ++i) {
+  for (int j = 0; j < J_NTILES * J_NELEMS; ++j) {
+int iTile = i / I_NELEMS;
+int jTile = j / J_NELEMS;
+int iElem = i % I_NELEMS;
+int jElem = j % J_NELEMS;
+order[team][iTile][jTile][iElem][jElem] = next++;
+  }
+}
+  }
+  printf("numTeams = %d\n", numTeams);
+  for (int team = 0; team < numTeams; ++team) {
+int expected = 0;
+for (int iTile = 0; iTile < I_NTILES; ++iTile) {
+  for (int jTile = 0; jTile < J_NTILES; ++jTile) {
+for (int iElem = 0; iElem < I_NELEMS; ++iElem) {
+  for (int jElem = 0; jElem < J_NELEMS; ++jElem) {
+int actual = order[team][iTile][jTile][iElem][jElem];
+if (expected != actual) {
+  printf("error: order[%d][%d][%d][%d][%d] = %d, expected %d\n",
+ team, iTile, jTile, iElem, jElem, actual, expected);
+  return 1;
+}
+++expected;
+  }
+}
+  }
+}
+  }
+  // CHECK: success
+  printf("success\n");
+  return 0;
+}
Index: openmp/libomptarget/test/offloading/target-teams-tile.c
===
--- /dev/null
+++ openmp/libomptarget/test/offloading/target-teams-tile.c
@@ -0,0 +1,59 @@
+// Check that omp tile (introduced in OpenMP 5.1) is permitted and behaves when
+// strictly nested within omp target teams.  This is an extension to OpenMP 5.2
+// and is enabled by default.
+
+// RUN: %libomptarget-compile-generic -fopenmp-version=51
+// RUN: %libomptarget-run-generic 2>&1 | %fcheck-generic
+
+#include 
+#include 
+
+#define NUM_TEAMS_UPPER 256
+#define I_NTILES 8
+#define J_NTILES 9
+#define I_NELEMS 2
+#define J_NELEMS 3
+
+int main() {
+  int numTeams;
+  int order[NUM_TEAMS_UPPER][I_NTILES][J_NTILES][I_NELEMS][J_NELEMS];
+  #pragma omp target teams num_teams(NUM_TEAMS_UPPER) map(from : numTeams)
+  {
+int team = omp_get_team_num();
+if (team == 0)
+  numTeams = omp_get_num_teams();
+int next = 0;
+#pragma omp tile sizes(I_NELEMS, J_NELEMS)
+for (int i = 0; i < I_NTILES * I_NELEMS; ++i) {
+  for (int j = 0; j < J_NTILES * J_NELEMS; ++j) {
+int iTile = i / I_NELEMS;
+int jTile = j / J_NELEMS;
+int iElem = i % I_NELEMS;
+int jElem = j % J_NELEMS;
+order[team][iTile][jTile][iElem][jElem] = next++;
+  }
+}
+  }
+  printf("numTeams = %d\n", numTeams);
+  for (int team = 0; te

[PATCH] D132513: [lit] Implement DEFINE and REDEFINE directives

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

Thanks for reviewing!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132513

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

[PATCH] D132513: [lit] Implement DEFINE and REDEFINE directives

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

It was a small battle to get the tests past the windows bots, but they 
eventually succeeded:

https://lab.llvm.org/buildbot/#/builders/216/builds/10125

I pushed the following additional patches while trying to fix the tests for 
windows:

- f47a5df92d488bdff418e3b1d249f407885896f2 

- 387924b307ae992cd4bf19908bdd5e4833063ce1 

- b9735db6464cf0bb4f60077b9f5495c0fa372ecf 

- 88f183c0db71579f06cfca606c333bce7084074e 


b9735db6464cf0bb4f60077b9f5495c0fa372ecf 
 adds a 
fixme and workaround for the issue that `echo` is doubling backslashes under 
windows.  I don't think that's specific to the DEFINE/REDEFINE implementation, 
and my hunch is that it's not a critical issue.  It would take me a while to 
produce a windows build, so I won't investigate that further anytime soon.  If 
someone else wants to, please feel free.

If anything looks objectionable, I can amend further or revert the whole series.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132513

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

[PATCH] D132513: [lit] Implement DEFINE and REDEFINE directives

[Joel E. Denny via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG28412d1800e3: [lit] Implement DEFINE and REDEFINE directives 
(authored by jdenny).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132513

Files:
  clang/test/CodeGen/attr-noundef.cpp
  clang/test/CodeGen/indirect-noundef.cpp
  clang/test/Preprocessor/init.c
  llvm/docs/CommandGuide/lit.rst
  llvm/docs/TestingGuide.rst
  llvm/test/tools/llvm-cvtres/help.test
  llvm/test/tools/yaml2obj/ELF/custom-null-section.yaml
  llvm/utils/lit/lit/TestRunner.py
  llvm/utils/lit/tests/Inputs/shtest-define/errors/assignment/before-name.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/assignment/between-name-equals.txt
  llvm/utils/lit/tests/Inputs/shtest-define/errors/assignment/braces-empty.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/assignment/braces-with-dot.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/assignment/braces-with-equals.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/assignment/braces-with-newline.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/assignment/braces-with-number.txt
  llvm/utils/lit/tests/Inputs/shtest-define/errors/assignment/braces-with-ws.txt
  llvm/utils/lit/tests/Inputs/shtest-define/errors/assignment/empty.txt
  llvm/utils/lit/tests/Inputs/shtest-define/errors/assignment/no-equals.txt
  llvm/utils/lit/tests/Inputs/shtest-define/errors/assignment/no-name.txt
  llvm/utils/lit/tests/Inputs/shtest-define/errors/assignment/ws-only.txt
  llvm/utils/lit/tests/Inputs/shtest-define/errors/continuation/empty.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/continuation/end-in-double-backslash.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/continuation/unterminated-define-bad-redefine.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/continuation/unterminated-define-continuation.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/continuation/unterminated-define-redefine.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/continuation/unterminated-define-run.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/continuation/unterminated-define.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/continuation/unterminated-redefine-bad-define.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/continuation/unterminated-redefine-continuation.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/continuation/unterminated-redefine-define.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/continuation/unterminated-redefine-run.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/continuation/unterminated-redefine.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/continuation/unterminated-run-define.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/continuation/unterminated-run-redefine.txt
  llvm/utils/lit/tests/Inputs/shtest-define/errors/continuation/ws-only.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/defined-check/define-already-by-config.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/defined-check/define-already-by-test.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/defined-check/define-inside-pattern.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/defined-check/define-multiple-exact.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/defined-check/define-multiple-once-exact.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/defined-check/define-prefixes-pattern.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/defined-check/define-suffixes-pattern.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/defined-check/redefine-inside-pattern.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/defined-check/redefine-multiple-exact.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/defined-check/redefine-multiple-once-exact.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/defined-check/redefine-none.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/defined-check/redefine-prefixes-pattern.txt
  
llvm/utils/lit/tests/Inputs/shtest-define/errors/defined-check/redefine-suffixes-pattern.txt
  llvm/utils/lit/tests/Inputs/shtest-define/errors/location-range.txt
  llvm/utils/lit/tests/Inputs/shtest-define/errors/no-run.txt
  llvm/utils/lit/tests/Inputs/shtest-define/examples/param-subst.txt
  llvm/utils/lit/tests/Inputs/shtest-define/expansion-order.txt
  llvm/utils/lit/tests/Inputs/shtest-define/line-number-substitutions.txt
  llvm/utils/lit/tests/Inputs/shtest-define/lit.cfg
  llvm/utils/lit/tests/Inputs/shtest-define/name-chars.txt
  llvm/utils/lit/tests/Inputs/shtest-define/recursiveExpansionLimit.txt
  llvm/utils/lit/tests/Inputs/shtest-define/shared-substs-0.txt
  llvm/utils/lit/tests/Inputs/shtest-define/shared-substs-1.txt
  llvm/utils/lit/tests/Inputs/shtest-define/value-equals.tx

[PATCH] D126549: [Clang][OpenMP] Don't overload "extension" in status doc

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126549

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

[PATCH] D126549: [Clang][OpenMP] Don't overload "extension" in status doc

[Joel E. Denny via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0103d4da740c: [Clang][OpenMP] Don't overload 
"extension" in status doc (authored by jdenny).

Changed prior to commit:
  https://reviews.llvm.org/D126549?vs=432567&id=440419#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126549

Files:
  clang/docs/OpenMPSupport.rst

Index: clang/docs/OpenMPSupport.rst
===
--- clang/docs/OpenMPSupport.rst
+++ clang/docs/OpenMPSupport.rst
@@ -117,19 +117,19 @@
 +--+--+--+---+
 |Category  | Feature  | Status   | Reviews   |
 +==+==+==+===+
-| loop extension   | support != in the canonical loop form| :good:`done` | D54441|
+| loop | support != in the canonical loop form| :good:`done` | D54441|
 +--+--+--+---+
-| loop extension   | #pragma omp loop (directive) | :part:`worked on`|   |
+| loop | #pragma omp loop (directive) | :part:`worked on`|   |
 +--+--+--+---+
-| loop extension   | collapse imperfectly nested loop | :good:`done` |   |
+| loop | collapse imperfectly nested loop | :good:`done` |   |
 +--+--+--+---+
-| loop extension   | collapse non-rectangular nested loop | :good:`done` |   |
+| loop | collapse non-rectangular nested loop | :good:`done` |   |
 +--+--+--+---+
-| loop extension   | C++ range-base for loop  | :good:`done` |   |
+| loop | C++ range-base for loop  | :good:`done` |   |
 +--+--+--+---+
-| loop extension   | clause: if for SIMD directives   | :good:`done` |   |
+| loop | clause: if for SIMD directives   | :good:`done` |   |
 +--+--+--+---+
-| loop extension   | inclusive scan extension (matching C++17 PSTL)   | :good:`done` |   |
+| loop | inclusive

[PATCH] D126549: [Clang][OpenMP] Don't overload "extension" in status doc

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126549

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

[PATCH] D126619: [OpenMP][Clang] Fix atomic compare for signed vs. unsigned

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

Thanks for the quick review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126619

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

[PATCH] D126619: [OpenMP][Clang] Fix atomic compare for signed vs. unsigned

[Joel E. Denny via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGd2e3cb737417: [OpenMP][Clang] Fix atomic compare for signed 
vs. unsigned (authored by jdenny).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126619

Files:
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/test/OpenMP/atomic_compare_codegen.cpp
  openmp/libomptarget/test/offloading/atomic-compare-signedness.c
  openmp/runtime/test/atomic/omp-atomic-compare-signedness.c

Index: openmp/runtime/test/atomic/omp-atomic-compare-signedness.c
===
--- /dev/null
+++ openmp/runtime/test/atomic/omp-atomic-compare-signedness.c
@@ -0,0 +1,40 @@
+// Check that omp atomic compare handles signedness of integer comparisons
+// correctly.
+//
+// At one time, a bug sometimes reversed the signedness.
+
+// RUN: %libomp-compile -fopenmp-version=51
+// RUN: %libomp-run | FileCheck %s
+
+// High parallelism increases our chances of detecting a lack of atomicity.
+#define NUM_THREADS_TRY 256
+
+#include 
+#include 
+#include 
+
+int main() {
+  //  CHECK: signed: num_threads=[[#NUM_THREADS:]]{{$}}
+  // CHECK-NEXT: signed: xs=[[#NUM_THREADS-1]]{{$}}
+  int xs = -1;
+  int numThreads;
+  #pragma omp parallel for num_threads(NUM_THREADS_TRY)
+  for (int i = 0; i < omp_get_num_threads(); ++i) {
+#pragma omp atomic compare
+if (xs < i) { xs = i; }
+if (i == 0)
+  numThreads = omp_get_num_threads();
+  }
+  printf("signed: num_threads=%d\n", numThreads);
+  printf("signed: xs=%d\n", xs);
+
+  // CHECK-NEXT: unsigned: xu=0x0{{$}}
+  unsigned xu = UINT_MAX;
+  #pragma omp parallel for num_threads(NUM_THREADS_TRY)
+  for (int i = 0; i < omp_get_num_threads(); ++i) {
+#pragma omp atomic compare
+if (xu > i) { xu = i; }
+  }
+  printf("unsigned: xu=0x%x\n", xu);
+  return 0;
+}
Index: openmp/libomptarget/test/offloading/atomic-compare-signedness.c
===
--- /dev/null
+++ openmp/libomptarget/test/offloading/atomic-compare-signedness.c
@@ -0,0 +1,42 @@
+// Check that omp atomic compare handles signedness of integer comparisons
+// correctly.
+//
+// At one time, a bug sometimes reversed the signedness.
+
+// RUN: %libomptarget-compile-generic -fopenmp-version=51
+// RUN: %libomptarget-run-generic | %fcheck-generic
+
+// High parallelism increases our chances of detecting a lack of atomicity.
+#define NUM_THREADS_TRY 256
+
+#include 
+#include 
+#include 
+
+int main() {
+  //  CHECK: signed: num_threads=[[#NUM_THREADS:]]{{$}}
+  // CHECK-NEXT: signed: xs=[[#NUM_THREADS-1]]{{$}}
+  int xs = -1;
+  int numThreads;
+  #pragma omp target parallel for num_threads(NUM_THREADS_TRY) \
+  map(tofrom:xs, numThreads)
+  for (int i = 0; i < omp_get_num_threads(); ++i) {
+#pragma omp atomic compare
+if (xs < i) { xs = i; }
+if (i == 0)
+  numThreads = omp_get_num_threads();
+  }
+  printf("signed: num_threads=%d\n", numThreads);
+  printf("signed: xs=%d\n", xs);
+
+  // CHECK-NEXT: unsigned: xu=0x0{{$}}
+  unsigned xu = UINT_MAX;
+  #pragma omp target parallel for num_threads(NUM_THREADS_TRY) \
+  map(tofrom:xu)
+  for (int i = 0; i < omp_get_num_threads(); ++i) {
+#pragma omp atomic compare
+if (xu > i) { xu = i; }
+  }
+  printf("unsigned: xu=0x%x\n", xu);
+  return 0;
+}
Index: clang/test/OpenMP/atomic_compare_codegen.cpp
===
--- clang/test/OpenMP/atomic_compare_codegen.cpp
+++ clang/test/OpenMP/atomic_compare_codegen.cpp
@@ -1979,21 +1979,21 @@
 // CHECK-NEXT:[[ULLE:%.*]] = alloca i64, align 8
 // CHECK-NEXT:[[ULLD:%.*]] = alloca i64, align 8
 // CHECK-NEXT:[[TMP0:%.*]] = load i8, i8* [[CE]], align 1
-// CHECK-NEXT:[[TMP1:%.*]] = atomicrmw umin i8* [[CX]], i8 [[TMP0]] monotonic, align 1
+// CHECK-NEXT:[[TMP1:%.*]] = atomicrmw min i8* [[CX]], i8 [[TMP0]] monotonic, align 1
 // CHECK-NEXT:[[TMP2:%.*]] = load i8, i8* [[CE]], align 1
-// CHECK-NEXT:[[TMP3:%.*]] = atomicrmw umax i8* [[CX]], i8 [[TMP2]] monotonic, align 1
+// CHECK-NEXT:[[TMP3:%.*]] = atomicrmw max i8* [[CX]], i8 [[TMP2]] monotonic, align 1
 // CHECK-NEXT:[[TMP4:%.*]] = load i8, i8* [[CE]], align 1
-// CHECK-NEXT:[[TMP5:%.*]] = atomicrmw umax i8* [[CX]], i8 [[TMP4]] monotonic, align 1
+// CHECK-NEXT:[[TMP5:%.*]] = atomicrmw max i8* [[CX]], i8 [[TMP4]] monotonic, align 1
 // CHECK-NEXT:[[TMP6:%.*]] = load i8, i8* [[CE]], align 1
-// CHECK-NEXT:[[TMP7:%.*]] = atomicrmw umin i8* [[CX]], i8 [[TMP6]] monotonic, align 1
+// CHECK-NEXT:[[TMP7:%.*]] = atomicrmw min i8* [[CX]], i8 [[TMP6]] monotonic, align 1
 // CHECK-NEXT:[[TMP8:%.*]] = load i8, i8* [[CE]], align 1
-// CHECK-NEXT:[[TMP9:%.*]] = atomicrmw umin i8* [[CX]], i8 [[TMP8]] monotonic, align 1
+// CH

[PATCH] D126619: [OpenMP][Clang] Fix atomic compare for signed vs. unsigned

[Joel E. Denny via Phabricator via cfe-commits]

jdenny created this revision.
jdenny added reviewers: tianshilei1992, ABataev, jdoerfert.
jdenny added projects: OpenMP, clang.
Herald added subscribers: guansong, yaxunl.
Herald added a project: All.
jdenny requested review of this revision.
Herald added subscribers: cfe-commits, sstefan1.

Without this patch, arguments to the
`llvm::OpenMPIRBuilder::AtomicOpValue` initializer are reversed.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126619

Files:
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/test/OpenMP/atomic_compare_codegen.cpp
  openmp/libomptarget/test/offloading/atomic-compare-signedness.c
  openmp/runtime/test/atomic/omp-atomic-compare-signedness.c

Index: openmp/runtime/test/atomic/omp-atomic-compare-signedness.c
===
--- /dev/null
+++ openmp/runtime/test/atomic/omp-atomic-compare-signedness.c
@@ -0,0 +1,40 @@
+// Check that omp atomic compare handles signedness of integer comparisons
+// correctly.
+//
+// At one time, a bug sometimes reversed the signedness.
+
+// RUN: %libomp-compile -fopenmp-version=51
+// RUN: %libomp-run | FileCheck %s
+
+// High parallelism increases our chances of detecting a lack of atomicity.
+#define NUM_THREADS_TRY 256
+
+#include 
+#include 
+#include 
+
+int main() {
+  //  CHECK: signed: num_threads=[[#NUM_THREADS:]]{{$}}
+  // CHECK-NEXT: signed: xs=[[#NUM_THREADS-1]]{{$}}
+  int xs = -1;
+  int numThreads;
+  #pragma omp parallel for num_threads(NUM_THREADS_TRY)
+  for (int i = 0; i < omp_get_num_threads(); ++i) {
+#pragma omp atomic compare
+if (xs < i) { xs = i; }
+if (i == 0)
+  numThreads = omp_get_num_threads();
+  }
+  printf("signed: num_threads=%d\n", numThreads);
+  printf("signed: xs=%d\n", xs);
+
+  // CHECK-NEXT: unsigned: xu=0x0{{$}}
+  unsigned xu = UINT_MAX;
+  #pragma omp parallel for num_threads(NUM_THREADS_TRY)
+  for (int i = 0; i < omp_get_num_threads(); ++i) {
+#pragma omp atomic compare
+if (xu > i) { xu = i; }
+  }
+  printf("unsigned: xu=0x%x\n", xu);
+  return 0;
+}
Index: openmp/libomptarget/test/offloading/atomic-compare-signedness.c
===
--- /dev/null
+++ openmp/libomptarget/test/offloading/atomic-compare-signedness.c
@@ -0,0 +1,42 @@
+// Check that omp atomic compare handles signedness of integer comparisons
+// correctly.
+//
+// At one time, a bug sometimes reversed the signedness.
+
+// RUN: %libomptarget-compile-generic -fopenmp-version=51
+// RUN: %libomptarget-run-generic | %fcheck-generic
+
+// High parallelism increases our chances of detecting a lack of atomicity.
+#define NUM_THREADS_TRY 256
+
+#include 
+#include 
+#include 
+
+int main() {
+  //  CHECK: signed: num_threads=[[#NUM_THREADS:]]{{$}}
+  // CHECK-NEXT: signed: xs=[[#NUM_THREADS-1]]{{$}}
+  int xs = -1;
+  int numThreads;
+  #pragma omp target parallel for num_threads(NUM_THREADS_TRY) \
+  map(tofrom:xs, numThreads)
+  for (int i = 0; i < omp_get_num_threads(); ++i) {
+#pragma omp atomic compare
+if (xs < i) { xs = i; }
+if (i == 0)
+  numThreads = omp_get_num_threads();
+  }
+  printf("signed: num_threads=%d\n", numThreads);
+  printf("signed: xs=%d\n", xs);
+
+  // CHECK-NEXT: unsigned: xu=0x0{{$}}
+  unsigned xu = UINT_MAX;
+  #pragma omp target parallel for num_threads(NUM_THREADS_TRY) \
+  map(tofrom:xu)
+  for (int i = 0; i < omp_get_num_threads(); ++i) {
+#pragma omp atomic compare
+if (xu > i) { xu = i; }
+  }
+  printf("unsigned: xu=0x%x\n", xu);
+  return 0;
+}
Index: clang/test/OpenMP/atomic_compare_codegen.cpp
===
--- clang/test/OpenMP/atomic_compare_codegen.cpp
+++ clang/test/OpenMP/atomic_compare_codegen.cpp
@@ -1979,21 +1979,21 @@
 // CHECK-NEXT:[[ULLE:%.*]] = alloca i64, align 8
 // CHECK-NEXT:[[ULLD:%.*]] = alloca i64, align 8
 // CHECK-NEXT:[[TMP0:%.*]] = load i8, i8* [[CE]], align 1
-// CHECK-NEXT:[[TMP1:%.*]] = atomicrmw umin i8* [[CX]], i8 [[TMP0]] monotonic, align 1
+// CHECK-NEXT:[[TMP1:%.*]] = atomicrmw min i8* [[CX]], i8 [[TMP0]] monotonic, align 1
 // CHECK-NEXT:[[TMP2:%.*]] = load i8, i8* [[CE]], align 1
-// CHECK-NEXT:[[TMP3:%.*]] = atomicrmw umax i8* [[CX]], i8 [[TMP2]] monotonic, align 1
+// CHECK-NEXT:[[TMP3:%.*]] = atomicrmw max i8* [[CX]], i8 [[TMP2]] monotonic, align 1
 // CHECK-NEXT:[[TMP4:%.*]] = load i8, i8* [[CE]], align 1
-// CHECK-NEXT:[[TMP5:%.*]] = atomicrmw umax i8* [[CX]], i8 [[TMP4]] monotonic, align 1
+// CHECK-NEXT:[[TMP5:%.*]] = atomicrmw max i8* [[CX]], i8 [[TMP4]] monotonic, align 1
 // CHECK-NEXT:[[TMP6:%.*]] = load i8, i8* [[CE]], align 1
-// CHECK-NEXT:[[TMP7:%.*]] = atomicrmw umin i8* [[CX]], i8 [[TMP6]] monotonic, align 1
+// CHECK-NEXT:[[TMP7:%.*]] = atomicrmw min i8* [[CX]], i8 [[TMP6]] monotonic, align 1
 // CHECK-NEXT:[[TMP8:%.*]] = load i8, i8* [[CE]], align 1
-// CHECK-NEXT

[PATCH] D126547: [OpenACC][OpenMP] Document atomic-in-teams extension

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

Thanks for the quick review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126547

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

[PATCH] D126547: [OpenACC][OpenMP] Document atomic-in-teams extension

[Joel E. Denny via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4a368136693b: [OpenACC][OpenMP] Document atomic-in-teams 
extension (authored by jdenny).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126547

Files:
  clang/docs/OpenMPSupport.rst
  openmp/docs/openacc/OpenMPExtensions.rst


Index: openmp/docs/openacc/OpenMPExtensions.rst
===
--- openmp/docs/openacc/OpenMPExtensions.rst
+++ openmp/docs/openacc/OpenMPExtensions.rst
@@ -137,3 +137,36 @@
 the runtime level.  That is, OpenACC's dynamic reference count is
 OpenMP's dynamic reference count, and OpenACC's structured reference
 count is our OpenMP hold reference count extension.
+
+.. _atomicWithinTeams:
+
+``atomic`` Strictly Nested Within ``teams``
+---
+
+Example
+^^^
+
+OpenMP 5.2, sec. 10.2 "teams Construct", p. 232, L9-12 restricts what
+regions can be strictly nested within a ``teams`` region.  As an
+extension, Clang relaxes that restriction in the case of the
+``atomic`` construct so that, for example, the following case is
+permitted:
+
+.. code-block:: c++
+
+  #pragma omp target teams map(tofrom:x)
+  #pragma omp atomic update
+  x++;
+
+Relationship with OpenACC
+^
+
+This extension is important when translating OpenACC to OpenMP because
+OpenACC does not have the same restriction for its corresponding
+constructs.  For example, the following is conforming OpenACC:
+
+.. code-block:: c++
+
+  #pragma acc parallel copy(x)
+  #pragma acc atomic update
+  x++;
Index: clang/docs/OpenMPSupport.rst
===
--- clang/docs/OpenMPSupport.rst
+++ clang/docs/OpenMPSupport.rst
@@ -369,9 +369,12 @@
 considered for standardization.  Please contact *openmp-dev* at
 *lists.llvm.org* to provide feedback.
 
-+--+---+--++
-|Category  | Feature   
| Status   | Reviews
|
-+==+===+==++
-| device extension | `'ompx_hold' map type modifier
| :good:`prototyped`   | D106509, D106510   
|
-|  | 
`_  | 
 ||
-+--+---+--++
++--+---+--++
+|Category  | Feature   
| Status   | Reviews
|
++==+===+==++
+| atomic extension | `'atomic' strictly nested within 'teams'  
| :good:`prototyped`   | D126323
|
+|  | 
`_
  |  |  
  |
++--+---+--++
+| device extension | `'ompx_hold' map type modifier
| :good:`prototyped`   | D106509, 
D106510   |
+|  | 
`_
  |  |  
  |
++--+---+--++


I

[PATCH] D126549: [Clang][OpenMP] Don't overload "extension" in status doc

[Joel E. Denny via Phabricator via cfe-commits]

jdenny created this revision.
jdenny added reviewers: ABataev, jdoerfert, RaviNarayanaswamy.
jdenny added a project: OpenMP.
Herald added subscribers: guansong, yaxunl.
Herald added a project: All.
jdenny requested review of this revision.
Herald added a subscriber: sstefan1.
Herald added a project: clang.

In Clang's OpenMPSupport.rst, "extension" is currently overloaded to
describe both:

1. Standard OpenMP features that appear only in recent versions of the OpenMP 
spec.
2. Non-standard features supported by Clang.  This usage appears in the final 
table on the page.

Last fall, we discussed this issue in the OpenMP in LLVM call and 
agreed it should be corrected.  This patch takes the simple approach
of dropping the word "extension" for all occurrences of the first
usage.  The result seems to read well.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126549

Files:
  clang/docs/OpenMPSupport.rst

Index: clang/docs/OpenMPSupport.rst
===
--- clang/docs/OpenMPSupport.rst
+++ clang/docs/OpenMPSupport.rst
@@ -117,19 +117,19 @@
 +--+--+--+---+
 |Category  | Feature  | Status   | Reviews   |
 +==+==+==+===+
-| loop extension   | support != in the canonical loop form| :good:`done` | D54441|
+| loop | support != in the canonical loop form| :good:`done` | D54441|
 +--+--+--+---+
-| loop extension   | #pragma omp loop (directive) | :part:`worked on`|   |
+| loop | #pragma omp loop (directive) | :part:`worked on`|   |
 +--+--+--+---+
-| loop extension   | collapse imperfectly nested loop | :good:`done` |   |
+| loop | collapse imperfectly nested loop | :good:`done` |   |
 +--+--+--+---+
-| loop extension   | collapse non-rectangular nested loop | :good:`done` |   |
+| loop | collapse non-rectangular nested loop | :good:`done` |   |
 +--+--+--+---+
-| loop extension   | C++ range-base for loop  | :good:`done` |   |
+| loop | C++ range-base for loop  | :good:`done` |   |
 +--+--+--+---+
-| loop extension   | clause: if for SIMD directives   | :good:`done` |   |
+| loop | clause: if for SIMD directives   | :good:`done` |   

[PATCH] D126547: [OpenACC][OpenMP] Document atomic-in-teams extension

[Joel E. Denny via Phabricator via cfe-commits]

jdenny created this revision.
jdenny added reviewers: ABataev, jdoerfert, clementval.
jdenny added projects: OpenMP, OpenACC.
Herald added subscribers: guansong, yaxunl.
Herald added a project: All.
jdenny requested review of this revision.
Herald added subscribers: cfe-commits, sstefan1.
Herald added a project: clang.

That is, put D126323  in the status doc and 
explain its relationship to
OpenACC support.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126547

Files:
  clang/docs/OpenMPSupport.rst
  openmp/docs/openacc/OpenMPExtensions.rst


Index: openmp/docs/openacc/OpenMPExtensions.rst
===
--- openmp/docs/openacc/OpenMPExtensions.rst
+++ openmp/docs/openacc/OpenMPExtensions.rst
@@ -137,3 +137,36 @@
 the runtime level.  That is, OpenACC's dynamic reference count is
 OpenMP's dynamic reference count, and OpenACC's structured reference
 count is our OpenMP hold reference count extension.
+
+.. _atomicWithinTeams:
+
+``atomic`` Strictly Nested Within ``teams``
+---
+
+Example
+^^^
+
+OpenMP 5.2, sec. 10.2 "teams Construct", p. 232, L9-12 restricts what
+regions can be strictly nested within a ``teams`` region.  As an
+extension, Clang relaxes that restriction in the case of the
+``atomic`` construct so that, for example, the following case is
+permitted:
+
+.. code-block:: c++
+
+  #pragma omp target teams map(tofrom:x)
+  #pragma omp atomic update
+  x++;
+
+Relationship with OpenACC
+^
+
+This extension is important when translating OpenACC to OpenMP because
+OpenACC does not have the same restriction for its corresponding
+constructs.  For example, the following is conforming OpenACC:
+
+.. code-block:: c++
+
+  #pragma acc parallel copy(x)
+  #pragma acc atomic update
+  x++;
Index: clang/docs/OpenMPSupport.rst
===
--- clang/docs/OpenMPSupport.rst
+++ clang/docs/OpenMPSupport.rst
@@ -369,9 +369,12 @@
 considered for standardization.  Please contact *openmp-dev* at
 *lists.llvm.org* to provide feedback.
 
-+--+---+--++
-|Category  | Feature   
| Status   | Reviews
|
-+==+===+==++
-| device extension | `'ompx_hold' map type modifier
| :good:`prototyped`   | D106509, D106510   
|
-|  | 
`_  | 
 ||
-+--+---+--++
++--+---+--++
+|Category  | Feature   
| Status   | Reviews
|
++==+===+==++
+| atomic extension | `'atomic' strictly nested within 'teams'  
| :good:`prototyped`   | D126323
|
+|  | 
`_
  |  |  
  |
++--+---+--++
+| device extension | `'ompx_hold' map type modifier
| :good:`prototyped`   | D106509, 
D106510   |
+|  | 
`_
  |  |  
  |
++--

[PATCH] D126323: [OpenMP] Extend omp teams to permit nested omp atomic

[Joel E. Denny via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG48ca3a5ebb15: [OpenMP] Extend omp teams to permit nested omp 
atomic (authored by jdenny).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126323

Files:
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/nesting_of_regions.cpp
  openmp/libomptarget/test/offloading/target-teams-atomic.c
  openmp/runtime/test/teams/teams-atomic.c

Index: openmp/runtime/test/teams/teams-atomic.c
===
--- /dev/null
+++ openmp/runtime/test/teams/teams-atomic.c
@@ -0,0 +1,49 @@
+// Check that omp atomic is permitted and behaves when strictly nested within
+// omp teams.  This is an extension to OpenMP 5.2 and is enabled by default.
+
+// RUN: %libomp-compile-and-run | FileCheck %s
+
+#include 
+#include 
+#include 
+#include 
+
+// High parallelism increases our chances of detecting a lack of atomicity.
+#define NUM_TEAMS_TRY 256
+
+int main() {
+  //  CHECK: update: num_teams=[[#NUM_TEAMS:]]{{$}}
+  // CHECK-NEXT: update: x=[[#NUM_TEAMS]]{{$}}
+  int x = 0;
+  int numTeams;
+  #pragma omp teams num_teams(NUM_TEAMS_TRY)
+  {
+#pragma omp atomic update
+++x;
+if (omp_get_team_num() == 0)
+  numTeams = omp_get_num_teams();
+  }
+  printf("update: num_teams=%d\n", numTeams);
+  printf("update: x=%d\n", x);
+
+  // CHECK-NEXT: capture: x=[[#NUM_TEAMS]]{{$}}
+  // CHECK-NEXT: capture: xCapturedCount=[[#NUM_TEAMS]]{{$}}
+  bool xCaptured[numTeams];
+  memset(xCaptured, 0, sizeof xCaptured);
+  x = 0;
+  #pragma omp teams num_teams(NUM_TEAMS_TRY)
+  {
+int v;
+#pragma omp atomic capture
+v = x++;
+xCaptured[v] = true;
+  }
+  printf("capture: x=%d\n", x);
+  int xCapturedCount = 0;
+  for (int i = 0; i < numTeams; ++i) {
+if (xCaptured[i])
+  ++xCapturedCount;
+  }
+  printf("capture: xCapturedCount=%d\n", xCapturedCount);
+  return 0;
+}
Index: openmp/libomptarget/test/offloading/target-teams-atomic.c
===
--- /dev/null
+++ openmp/libomptarget/test/offloading/target-teams-atomic.c
@@ -0,0 +1,50 @@
+// Check that omp atomic is permitted and behaves when strictly nested within
+// omp target teams.  This is an extension to OpenMP 5.2 and is enabled by
+// default.
+
+// RUN: %libomptarget-compile-run-and-check-generic
+
+#include 
+#include 
+#include 
+#include 
+
+// High parallelism increases our chances of detecting a lack of atomicity.
+#define NUM_TEAMS_TRY 256
+
+int main() {
+  //  CHECK: update: num_teams=[[#NUM_TEAMS:]]{{$}}
+  // CHECK-NEXT: update: x=[[#NUM_TEAMS]]{{$}}
+  int x = 0;
+  int numTeams;
+  #pragma omp target teams num_teams(NUM_TEAMS_TRY) map(tofrom:x, numTeams)
+  {
+#pragma omp atomic update
+++x;
+if (omp_get_team_num() == 0)
+  numTeams = omp_get_num_teams();
+  }
+  printf("update: num_teams=%d\n", numTeams);
+  printf("update: x=%d\n", x);
+
+  // CHECK-NEXT: capture: x=[[#NUM_TEAMS]]{{$}}
+  // CHECK-NEXT: capture: xCapturedCount=[[#NUM_TEAMS]]{{$}}
+  bool xCaptured[numTeams];
+  memset(xCaptured, 0, sizeof xCaptured);
+  x = 0;
+  #pragma omp target teams num_teams(NUM_TEAMS_TRY) map(tofrom:x, numTeams)
+  {
+int v;
+#pragma omp atomic capture
+v = x++;
+xCaptured[v] = true;
+  }
+  printf("capture: x=%d\n", x);
+  int xCapturedCount = 0;
+  for (int i = 0; i < numTeams; ++i) {
+if (xCaptured[i])
+  ++xCapturedCount;
+  }
+  printf("capture: xCapturedCount=%d\n", xCapturedCount);
+  return 0;
+}
Index: clang/test/OpenMP/nesting_of_regions.cpp
===
--- clang/test/OpenMP/nesting_of_regions.cpp
+++ clang/test/OpenMP/nesting_of_regions.cpp
@@ -1,10 +1,11 @@
-// RUN: %clang_cc1 -fsyntax-only -fopenmp -fopenmp-version=45 -verify=expected,omp45,omp45warn %s
-// RUN: %clang_cc1 -fsyntax-only -fopenmp -verify=expected,omp50 %s
-// RUN: %clang_cc1 -fsyntax-only -fopenmp -fopenmp-version=45 -verify=expected,omp45 -Wno-openmp %s
-// RUN: %clang_cc1 -fsyntax-only -fopenmp -fopenmp-version=45 -verify=expected,omp45 -Wno-source-uses-openmp %s
+// RUN: %clang_cc1 -fsyntax-only -fopenmp -fopenmp-version=45 -fno-openmp-extensions -verify=expected,omp45,omp45warn,omp %s
+// RUN: %clang_cc1 -fsyntax-only -fopenmp -fno-openmp-extensions -verify=expected,omp50,omp %s
+// RUN: %clang_cc1 -fsyntax-only -fopenmp -fopenmp-extensions -verify=expected,omp50 %s
+// RUN: %clang_cc1 -fsyntax-only -fopenmp -fopenmp-version=45 -verify=expected,omp45,omp -fno-openmp-extensions -Wno-openmp %s
+// RUN: %clang_cc1 -fsyntax-only -fopenmp -fopenmp-version=45 -verify=expected,omp45,omp -fno-openmp-extensions -Wno-source-uses-openmp %s
 
-// RUN: %clang_cc1 -fsyntax-only -fopenmp-simd -fopenmp-version=45 -verify=expected,omp45,omp45warn %s

[PATCH] D126323: [OpenMP] Extend omp teams to permit nested omp atomic

[Joel E. Denny via Phabricator via cfe-commits]

jdenny marked 2 inline comments as done.
jdenny added a comment.

Thanks.  Will try to push tomorrow.


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

https://reviews.llvm.org/D126323

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

[PATCH] D126323: [OpenMP] Extend omp teams to permit nested omp atomic

[Joel E. Denny via Phabricator via cfe-commits]

jdenny updated this revision to Diff 431804.
jdenny added a comment.

Fixed a bug in the new tests.


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

https://reviews.llvm.org/D126323

Files:
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/nesting_of_regions.cpp
  openmp/libomptarget/test/offloading/target-teams-atomic.c
  openmp/runtime/test/teams/teams-atomic.c

Index: openmp/runtime/test/teams/teams-atomic.c
===
--- /dev/null
+++ openmp/runtime/test/teams/teams-atomic.c
@@ -0,0 +1,49 @@
+// Check that omp atomic is permitted and behaves when strictly nested within
+// omp teams.  This is an extension to OpenMP 5.2 and is enabled by default.
+
+// RUN: %libomp-compile-and-run | FileCheck %s
+
+#include 
+#include 
+#include 
+#include 
+
+// High parallelism increases our chances of detecting a lack of atomicity.
+#define NUM_TEAMS_TRY 256
+
+int main() {
+  //  CHECK: update: num_teams=[[#NUM_TEAMS:]]{{$}}
+  // CHECK-NEXT: update: x=[[#NUM_TEAMS]]{{$}}
+  int x = 0;
+  int numTeams;
+  #pragma omp teams num_teams(NUM_TEAMS_TRY)
+  {
+#pragma omp atomic update
+++x;
+if (omp_get_team_num() == 0)
+  numTeams = omp_get_num_teams();
+  }
+  printf("update: num_teams=%d\n", numTeams);
+  printf("update: x=%d\n", x);
+
+  // CHECK-NEXT: capture: x=[[#NUM_TEAMS]]{{$}}
+  // CHECK-NEXT: capture: xCapturedCount=[[#NUM_TEAMS]]{{$}}
+  bool xCaptured[numTeams];
+  memset(xCaptured, 0, sizeof xCaptured);
+  x = 0;
+  #pragma omp teams num_teams(NUM_TEAMS_TRY)
+  {
+int v;
+#pragma omp atomic capture
+v = x++;
+xCaptured[v] = true;
+  }
+  printf("capture: x=%d\n", x);
+  int xCapturedCount = 0;
+  for (int i = 0; i < numTeams; ++i) {
+if (xCaptured[i])
+  ++xCapturedCount;
+  }
+  printf("capture: xCapturedCount=%d\n", xCapturedCount);
+  return 0;
+}
Index: openmp/libomptarget/test/offloading/target-teams-atomic.c
===
--- /dev/null
+++ openmp/libomptarget/test/offloading/target-teams-atomic.c
@@ -0,0 +1,50 @@
+// Check that omp atomic is permitted and behaves when strictly nested within
+// omp target teams.  This is an extension to OpenMP 5.2 and is enabled by
+// default.
+
+// RUN: %libomptarget-compile-run-and-check-generic
+
+#include 
+#include 
+#include 
+#include 
+
+// High parallelism increases our chances of detecting a lack of atomicity.
+#define NUM_TEAMS_TRY 256
+
+int main() {
+  //  CHECK: update: num_teams=[[#NUM_TEAMS:]]{{$}}
+  // CHECK-NEXT: update: x=[[#NUM_TEAMS]]{{$}}
+  int x = 0;
+  int numTeams;
+  #pragma omp target teams num_teams(NUM_TEAMS_TRY) map(tofrom:x, numTeams)
+  {
+#pragma omp atomic update
+++x;
+if (omp_get_team_num() == 0)
+  numTeams = omp_get_num_teams();
+  }
+  printf("update: num_teams=%d\n", numTeams);
+  printf("update: x=%d\n", x);
+
+  // CHECK-NEXT: capture: x=[[#NUM_TEAMS]]{{$}}
+  // CHECK-NEXT: capture: xCapturedCount=[[#NUM_TEAMS]]{{$}}
+  bool xCaptured[numTeams];
+  memset(xCaptured, 0, sizeof xCaptured);
+  x = 0;
+  #pragma omp target teams num_teams(NUM_TEAMS_TRY) map(tofrom:x, numTeams)
+  {
+int v;
+#pragma omp atomic capture
+v = x++;
+xCaptured[v] = true;
+  }
+  printf("capture: x=%d\n", x);
+  int xCapturedCount = 0;
+  for (int i = 0; i < numTeams; ++i) {
+if (xCaptured[i])
+  ++xCapturedCount;
+  }
+  printf("capture: xCapturedCount=%d\n", xCapturedCount);
+  return 0;
+}
Index: clang/test/OpenMP/nesting_of_regions.cpp
===
--- clang/test/OpenMP/nesting_of_regions.cpp
+++ clang/test/OpenMP/nesting_of_regions.cpp
@@ -1,10 +1,11 @@
-// RUN: %clang_cc1 -fsyntax-only -fopenmp -fopenmp-version=45 -verify=expected,omp45,omp45warn %s
-// RUN: %clang_cc1 -fsyntax-only -fopenmp -verify=expected,omp50 %s
-// RUN: %clang_cc1 -fsyntax-only -fopenmp -fopenmp-version=45 -verify=expected,omp45 -Wno-openmp %s
-// RUN: %clang_cc1 -fsyntax-only -fopenmp -fopenmp-version=45 -verify=expected,omp45 -Wno-source-uses-openmp %s
+// RUN: %clang_cc1 -fsyntax-only -fopenmp -fopenmp-version=45 -fno-openmp-extensions -verify=expected,omp45,omp45warn,omp %s
+// RUN: %clang_cc1 -fsyntax-only -fopenmp -fno-openmp-extensions -verify=expected,omp50,omp %s
+// RUN: %clang_cc1 -fsyntax-only -fopenmp -fopenmp-extensions -verify=expected,omp50 %s
+// RUN: %clang_cc1 -fsyntax-only -fopenmp -fopenmp-version=45 -verify=expected,omp45,omp -fno-openmp-extensions -Wno-openmp %s
+// RUN: %clang_cc1 -fsyntax-only -fopenmp -fopenmp-version=45 -verify=expected,omp45,omp -fno-openmp-extensions -Wno-source-uses-openmp %s
 
-// RUN: %clang_cc1 -fsyntax-only -fopenmp-simd -fopenmp-version=45 -verify=expected,omp45,omp45warn %s
-// RUN: %clang_cc1 -fsyntax-only -fopenmp-simd -verify=expected,omp50 %s
+// RUN: %clang_cc1 -fsyntax-only -fopenmp-simd -fopenmp-version=45 -fno-openmp-extensions -verify=expe

[PATCH] D126323: [OpenMP] Extend omp teams to permit nested omp atomic

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4983
+  CurrentRegion != OMPD_loop &&
+  !(SemaRef.getLangOpts().OpenMPExtensions &&
+CurrentRegion == OMPD_atomic);

ABataev wrote:
> jdenny wrote:
> > ABataev wrote:
> > > Can you add a check for 5.2?
> > This is an extension relative not only to 5.2 and but also to previous 
> > versions.  I quoted 5.2 just because it's the most recent.
> > 
> > Given that, I'm not sure why this should check for 5.2.  Am I 
> > misunderstanding your request?
> Just looks like it is allowed in 5.2. Am I missing something?
I'm not aware of any change in the 5.2 spec that would permit this.  The text I 
cited in the review summary does not include atomic in the list of regions 
"that may be strictly nested inside the teams region":

> distribute regions, including any distribute regions arising from composite 
> constructs,
> parallel regions, including any parallel regions arising from combined 
> constructs, loop
> regions, omp_get_num_teams() regions, and omp_get_team_num() regions are the
> only OpenMP regions that may be strictly nested inside the teams region.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126323

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

[PATCH] D126323: [OpenMP] Extend omp teams to permit nested omp atomic

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4983
+  CurrentRegion != OMPD_loop &&
+  !(SemaRef.getLangOpts().OpenMPExtensions &&
+CurrentRegion == OMPD_atomic);

ABataev wrote:
> Can you add a check for 5.2?
This is an extension relative not only to 5.2 and but also to previous 
versions.  I quoted 5.2 just because it's the most recent.

Given that, I'm not sure why this should check for 5.2.  Am I misunderstanding 
your request?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126323

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

[PATCH] D126323: [OpenMP] Extend omp teams to permit nested omp atomic

[Joel E. Denny via Phabricator via cfe-commits]

jdenny created this revision.
jdenny added reviewers: jdoerfert, ABataev.
jdenny added a project: OpenMP.
Herald added subscribers: guansong, yaxunl.
Herald added a project: All.
jdenny requested review of this revision.
Herald added subscribers: cfe-commits, sstefan1.
Herald added a project: clang.

OpenMP 5.2, sec. 10.2 "teams Construct", p. 232, L9-12 restricts what
regions can be strictly nested within a `teams` construct.  This patch
relaxes Clang's enforcement of this restriction in the case of nested
`atomic` constructs unless `-fno-openmp-extensions` is specified.
Cases like the following then seem to work fine with no additional
implementation changes:

  #pragma omp target teams map(tofrom:x)
  #pragma omp atomic update
  x++;


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126323

Files:
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/nesting_of_regions.cpp
  openmp/libomptarget/test/offloading/target-teams-atomic.c
  openmp/runtime/test/teams/teams-atomic.c

Index: openmp/runtime/test/teams/teams-atomic.c
===
--- /dev/null
+++ openmp/runtime/test/teams/teams-atomic.c
@@ -0,0 +1,47 @@
+// Check that omp atomic is permitted and behaves when strictly nested within
+// omp teams.  This is an extension to OpenMP 5.2 and is enabled by default.
+
+// RUN: %libomp-compile-and-run | FileCheck %s
+
+#include 
+#include 
+#include 
+#include 
+
+// High parallelism increases our chances of detecting a lack of atomicity.
+#define NUM_TEAMS_TRY 256
+
+int main() {
+  //  CHECK: update: num_teams=[[#NUM_TEAMS:]]{{$}}
+  // CHECK-NEXT: update: x=[[#NUM_TEAMS]]{{$}}
+  int x = 0;
+  int numTeams;
+  #pragma omp teams num_teams(NUM_TEAMS_TRY)
+  {
+#pragma omp atomic update
+++x;
+if (omp_get_team_num() == 0)
+  numTeams = omp_get_num_teams();
+  }
+  printf("update: num_teams=%d\n", numTeams);
+  printf("update: x=%d\n", x);
+
+  // CHECK-NEXT: capture: x=[[#NUM_TEAMS]]{{$}}
+  // CHECK-NEXT: capture: xCapturedCount=[[#NUM_TEAMS]]{{$}}
+  bool xCaptured[numTeams];
+  memset(xCaptured, 0, sizeof xCaptured);
+  x = 0;
+  #pragma omp teams num_teams(NUM_TEAMS_TRY)
+  {
+int v;
+#pragma omp atomic capture
+v = x++;
+xCaptured[v] = true;
+  }
+  printf("capture: x=%d\n", x);
+  int xCapturedCount = 0;
+  for (int i = 0; i < numTeams; ++i)
+++xCapturedCount;
+  printf("capture: xCapturedCount=%d\n", xCapturedCount);
+  return 0;
+}
Index: openmp/libomptarget/test/offloading/target-teams-atomic.c
===
--- /dev/null
+++ openmp/libomptarget/test/offloading/target-teams-atomic.c
@@ -0,0 +1,48 @@
+// Check that omp atomic is permitted and behaves when strictly nested within
+// omp target teams.  This is an extension to OpenMP 5.2 and is enabled by
+// default.
+
+// RUN: %libomptarget-compile-run-and-check-generic
+
+#include 
+#include 
+#include 
+#include 
+
+// High parallelism increases our chances of detecting a lack of atomicity.
+#define NUM_TEAMS_TRY 256
+
+int main() {
+  //  CHECK: update: num_teams=[[#NUM_TEAMS:]]{{$}}
+  // CHECK-NEXT: update: x=[[#NUM_TEAMS]]{{$}}
+  int x = 0;
+  int numTeams;
+  #pragma omp target teams num_teams(NUM_TEAMS_TRY) map(tofrom:x, numTeams)
+  {
+#pragma omp atomic update
+++x;
+if (omp_get_team_num() == 0)
+  numTeams = omp_get_num_teams();
+  }
+  printf("update: num_teams=%d\n", numTeams);
+  printf("update: x=%d\n", x);
+
+  // CHECK-NEXT: capture: x=[[#NUM_TEAMS]]{{$}}
+  // CHECK-NEXT: capture: xCapturedCount=[[#NUM_TEAMS]]{{$}}
+  bool xCaptured[numTeams];
+  memset(xCaptured, 0, sizeof xCaptured);
+  x = 0;
+  #pragma omp target teams num_teams(NUM_TEAMS_TRY) map(tofrom:x, numTeams)
+  {
+int v;
+#pragma omp atomic capture
+v = x++;
+xCaptured[v] = true;
+  }
+  printf("capture: x=%d\n", x);
+  int xCapturedCount = 0;
+  for (int i = 0; i < numTeams; ++i)
+++xCapturedCount;
+  printf("capture: xCapturedCount=%d\n", xCapturedCount);
+  return 0;
+}
Index: clang/test/OpenMP/nesting_of_regions.cpp
===
--- clang/test/OpenMP/nesting_of_regions.cpp
+++ clang/test/OpenMP/nesting_of_regions.cpp
@@ -1,10 +1,11 @@
-// RUN: %clang_cc1 -fsyntax-only -fopenmp -fopenmp-version=45 -verify=expected,omp45,omp45warn %s
-// RUN: %clang_cc1 -fsyntax-only -fopenmp -verify=expected,omp50 %s
-// RUN: %clang_cc1 -fsyntax-only -fopenmp -fopenmp-version=45 -verify=expected,omp45 -Wno-openmp %s
-// RUN: %clang_cc1 -fsyntax-only -fopenmp -fopenmp-version=45 -verify=expected,omp45 -Wno-source-uses-openmp %s
+// RUN: %clang_cc1 -fsyntax-only -fopenmp -fopenmp-version=45 -fno-openmp-extensions -verify=expected,omp45,omp45warn,omp %s
+// RUN: %clang_cc1 -fsyntax-only -fopenmp -fno-openmp-extensions -verify=expected,omp50,omp %s
+// RUN: %clang_cc1 -fsyntax-only -fopenmp -fopenmp-extensions -verify=expected,omp50

[PATCH] D109255: [NFC][OpenMP] Add fsyntax-only to driver tests

[Joel E. Denny via Phabricator via cfe-commits]

jdenny accepted this revision.
jdenny added a comment.

LGTM.  Thanks for the fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109255

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

[PATCH] D109255: [NFC][OpenMP] Add fsyntax-only to driver tests

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D109255#2983063 , @jsji wrote:

> In D109255#2983052 , @ABataev wrote:
>
>> In D109255#2983017 , @jdenny wrote:
>>
>>> This test is meant to check that the driver processes these options 
>>> correctly.  -cc1 isn't the driver and often ignores either the positive or 
>>> negative version of an option.
>>
>> Better not to do such checks, just need to check that driver correctly 
>> translates driver options to the frontend options (using `-###`), better to 
>> use `-verify` only with the frontend tests. Suggest to rework this test.
>
> Make sense to me. How about we use `_cc1` as Alexey suggested here (and 
> rename the file to remove `driver-` prefix).

OK.

> Maybe Joel can add another test to test the driver correctly translates 
> driver options to the frontend options (using `-###`) in `test/Driver` dir?

I probably won't get to it today, but I can work on it later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109255

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

[PATCH] D109255: [NFC][OpenMP] Add fsyntax-only to driver tests

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D109255#2983018 , @jsji wrote:

> In D109255#2983017 , @jdenny wrote:
>
>> This test is meant to check that the driver processes these options 
>> correctly.  -cc1 isn't the driver and often ignores either the positive or 
>> negative version of an option.
>
> OK, I can revert to driver + fsyntax-only if that is preferred?

That would be my preference.  Otherwise, we never test the interface exposed to 
the user, and the test is misnamed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109255

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

[PATCH] D109255: [NFC][OpenMP] Add fsyntax-only to driver tests

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

This test is meant to check that the driver processes these options correctly.  
-cc1 isn't the driver and often ignores either the positive or negative version 
of an option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109255

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

[PATCH] D107503: [clang][Rewriter] patch to fix bug with ReplaceText erasing too many characters when text has been inserted before the replace location.

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/lib/Rewrite/Rewriter.cpp:136
   if (OrigLength != NewStr.size())
 AddReplaceDelta(OrigOffset, NewStr.size() - OrigLength);
 }

I think this needs to be fixed too.  That is, the removal of the inserts 
requires updating the insert delta separately from the replace delta.  
Otherwise, a future `getRewrittenText` ending at `OrigOffset` will include 
extra characters.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107503

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

[PATCH] D107503: [clang][Rewriter] patch to fix bug with ReplaceText erasing too many characters when text has been inserted before the replace location.

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

Thanks for working on this.  I agree there's a bug.  However, this patch 
doesn't fix the bug.  It provides a workaround.

That is, the current default `ReplaceText` behavior, as revealed in the first 
test added by this patch, doesn't make sense.  
`IncludeInsertsAtBeginOfRange=true` (the default) should mean to also remove 
the text that was previously inserted before the specified range.  Instead, the 
previously inserted text confuses the implementation, so it removes extra text 
//after// the specified range.  It's hard to imagine anyone is relying on that 
broken behavior.

I see a couple of potential paths forward.  Either:

1. Change the patch summary and the new comments to make it clear that the 
default behavior is broken, so specifying `IncludeInsertsAtBeginOfRange=true` 
is the only way to ensure reasonable behavior.  `FIXME:` should prefix some of 
those comments.

2. Fix the bug.  I've added an inline comment to suggest a possible fix, but I 
haven't tested it carefully.

Either way, `RemoveText` should be addressed in the same manner as it has the 
same bug.




Comment at: clang/lib/Rewrite/Rewriter.cpp:132
 StringRef NewStr) {
   unsigned RealOffset = getMappedOffset(OrigOffset, true);
   Buffer.erase(RealOffset, OrigLength);

I think this might fix the bug, but I haven't tested thoroughly.

One problem is ensuring backward compatibility here and in callers while 
keeping a consistent interface.  That is, the previous default here was 
`IncludeInsertsAtBeginOfRange=false`, but it seems the intended default 
elsewhere is meant to be the opposite.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107503

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

[PATCH] D106509: [OpenMP][OpenACC] Implement `ompx_hold` map type modifier extension in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

Thanks!  I'll wait for the D106510  review 
before trying to land.


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

https://reviews.llvm.org/D106509

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

[PATCH] D106509: [OpenMP][OpenACC] Implement `ompx_hold` map type modifier extension in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/include/clang/Driver/Options.td:2383
+defm openmp_extensions: BoolFOption<"openmp-extensions",
+  LangOpts<"OpenMPExtensions">, DefaultFalse,
+  PosFlag jdenny wrote:
> > ABataev wrote:
> > > Why do you want to disable it by default?
> > I thought that's what we agreed upon.
> > 
> > I'm personally happy if, in general, all features are enabled by default 
> > and users opt out by requesting strict conformance, but Clang doesn't do 
> > that today even for features from newer OpenMP standards.  Should it do so 
> > for extensions, which appear in no standard?
> > 
> > What would you prefer?
> I would enable it by default, as it may help users in some cases unless there 
> are no other strong opinions.
Well, that promotes my extensions, so I've made that change.


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

https://reviews.llvm.org/D106509

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

[PATCH] D106509: [OpenMP][OpenACC] Implement `ompx_hold` map type modifier extension in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/include/clang/Driver/Options.td:2383
+defm openmp_extensions: BoolFOption<"openmp-extensions",
+  LangOpts<"OpenMPExtensions">, DefaultFalse,
+  PosFlag Why do you want to disable it by default?
I thought that's what we agreed upon.

I'm personally happy if, in general, all features are enabled by default and 
users opt out by requesting strict conformance, but Clang doesn't do that today 
even for features from newer OpenMP standards.  Should it do so for extensions, 
which appear in no standard?

What would you prefer?



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5774-5776
+  if (Args.hasFlag(options::OPT_fopenmp_extensions,
+   options::OPT_fno_openmp_extensions, /*Default=*/false))
+CmdArgs.push_back("-fopenmp-extensions");

ABataev wrote:
> Do we still need this?
I tried removing these, but then the driver doesn't pass `-fopenmp-extensions` 
to the front end.  Am I doing something wrong?


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

https://reviews.llvm.org/D106509

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

[PATCH] D106509: [OpenMP][OpenACC] Implement `ompx_hold` map type modifier extension in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

jdenny marked 4 inline comments as done.
jdenny added inline comments.



Comment at: clang/lib/Basic/OpenMPKinds.cpp:64
+  return OMPC_MAP_MODIFIER_unknown;
+if (!LangOpts.OpenMPExtensions && Type == OMPC_MAP_MODIFIER_ompx_hold)
   return OMPC_MAP_MODIFIER_unknown;

ABataev wrote:
> jdenny wrote:
> > ABataev wrote:
> > > jdenny wrote:
> > > > ABataev wrote:
> > > > > I would enable this since OpenMP 5.2, since in 5.2 `ompx_` is 
> > > > > officially allowed extension format.
> > > > Do you mean both `-fopenmp-version=52` and `-fopenmp-extensions` should 
> > > > be required to enable `ompx_hold`?
> > > > 
> > > > Or do you mean only `-fopenmp-version=52` should be required to enable 
> > > > `ompx_hold`, and we can drop the `-fopenmp-extensions` implementation?
> > > Second option. Actually, we can enable it if either `-fopenmp-version=52` 
> > > or `-fopenmp-extensions` is used, depends if we want to add a switch for 
> > > non-standard OpenMP extensions. If OpenMP 5.2 is on, we can just ignore 
> > > `-f[no]-openmp-extensions`. Thoughts?
> > Consider that, if `-fopenmp-version=52` is sufficient to enable all 
> > extensions using the `ompx_` prefix, then all such extensions will be 
> > enabled by default once OpenMP 5.2 is the default.  At that point, won't it 
> > be strange that standard OpenMP 5.3 or 6.0 features will be disabled by 
> > default while some features appearing in no standard will be enabled by 
> > default?
> > 
> > By that logic, it seems `-fopenmp-version=52` shouldn't be sufficient to 
> > enable extensions.  However, it seems overkill to have to specify both 
> > `-fopenmp-version=52` and `-fopenmp-extensions`.  I think 
> > `-fopenmp-extensions` is a clear enough request to enable the `ompx_` 
> > prefix regardless of the OpenMP version.
> Ok, let's move with `-fopenmp-extensions`
OK, thanks.


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

https://reviews.llvm.org/D106509

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

[PATCH] D106509: [OpenMP][OpenACC] Implement `ompx_hold` map type modifier extension in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/lib/Basic/OpenMPKinds.cpp:64
+  return OMPC_MAP_MODIFIER_unknown;
+if (!LangOpts.OpenMPExtensions && Type == OMPC_MAP_MODIFIER_ompx_hold)
   return OMPC_MAP_MODIFIER_unknown;

ABataev wrote:
> jdenny wrote:
> > ABataev wrote:
> > > I would enable this since OpenMP 5.2, since in 5.2 `ompx_` is officially 
> > > allowed extension format.
> > Do you mean both `-fopenmp-version=52` and `-fopenmp-extensions` should be 
> > required to enable `ompx_hold`?
> > 
> > Or do you mean only `-fopenmp-version=52` should be required to enable 
> > `ompx_hold`, and we can drop the `-fopenmp-extensions` implementation?
> Second option. Actually, we can enable it if either `-fopenmp-version=52` or 
> `-fopenmp-extensions` is used, depends if we want to add a switch for 
> non-standard OpenMP extensions. If OpenMP 5.2 is on, we can just ignore 
> `-f[no]-openmp-extensions`. Thoughts?
Consider that, if `-fopenmp-version=52` is sufficient to enable all extensions 
using the `ompx_` prefix, then all such extensions will be enabled by default 
once OpenMP 5.2 is the default.  At that point, won't it be strange that 
standard OpenMP 5.3 or 6.0 features will be disabled by default while some 
features appearing in no standard will be enabled by default?

By that logic, it seems `-fopenmp-version=52` shouldn't be sufficient to enable 
extensions.  However, it seems overkill to have to specify both 
`-fopenmp-version=52` and `-fopenmp-extensions`.  I think `-fopenmp-extensions` 
is a clear enough request to enable the `ompx_` prefix regardless of the OpenMP 
version.


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

https://reviews.llvm.org/D106509

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

[PATCH] D106509: [OpenMP][OpenACC] Implement `ompx_hold` map type modifier extension in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/lib/Basic/OpenMPKinds.cpp:64
+  return OMPC_MAP_MODIFIER_unknown;
+if (!LangOpts.OpenMPExtensions && Type == OMPC_MAP_MODIFIER_ompx_hold)
   return OMPC_MAP_MODIFIER_unknown;

ABataev wrote:
> I would enable this since OpenMP 5.2, since in 5.2 `ompx_` is officially 
> allowed extension format.
Do you mean both `-fopenmp-version=52` and `-fopenmp-extensions` should be 
required to enable `ompx_hold`?

Or do you mean only `-fopenmp-version=52` should be required to enable 
`ompx_hold`, and we can drop the `-fopenmp-extensions` implementation?


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

https://reviews.llvm.org/D106509

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

[PATCH] D106509: [OpenMP][OpenACC] Implement `hold` map type modifier extension in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D106509#2896956 , @jdoerfert wrote:

>> That's fine for me, but don't the routines use llvm_omp_?
>
> That was before we "standardized" `ompx_` for OpenMP 5.2.

Ah.  Thanks.

>> Should we also have that prefix in various enumerators in the 
>> implementation? For example, what does OMP_MAP_HOLD become?
>
> I'd suggest `OMPX_MAP_HOLD`

Makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106509

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

[PATCH] D106509: [OpenMP][OpenACC] Implement `hold` map type modifier extension in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

Thanks for the reviews.

In D106509#2896351 , @ABataev wrote:

> That's strange that we need to ignore `delete` modifier,

It's not ignored in general: the standard (dynamic) OpenMP reference count is 
set to 0 (if it isn't already) so that, once `hold` is not in effect, the data 
can be unmapped.

> I would say that most probably there is a bug in the user's code.

Yes, my understanding of the motivation for this OpenACC feature is to protect 
users from unbalanced increments and decrements.  In structured cases, the 
assumption is that it's unlikely the user intends to unmap early.

>> Clang currently doesn't support OpenMP 5.1 features unless 
>> -fopenmp-version=51. Does it make sense to have an option to enable 
>> extensions? Instead of a separate option, we could accept something like 
>> -fopenmp-version=51,hold,foo.
>
> I would just add a new options `-fopenmp-extension` or something like this 
> just toenable all non-standard extensions, better to keep `-fopenmp-version` 
> as is.

Works for me.  I suppose if we later want a syntax for enabling specific 
extensions, `-fopenmp-extension` could become an alias for 
`-fopenmp-extension=all`.

>> In Clang diagnostics, this patch does not add hold to the list of acceptable 
>> map type modifiers because it's an extension. Should it? If there were a 
>> command-line option to enable extensions, that would make the path clearer.
>
> Yes, with the extensions enabled it should generate the list of all supported 
> modifiers.

Agreed.

In D106509#2896399 , @ABataev wrote:

> In D106509#2896398 , @jdoerfert 
> wrote:
>
>> I would propose we prefix these new clauses and such with `ompx_`.
>
> +1 here

That's fine for me, but don't the routines use `llvm_omp_`?

Should we also have that prefix in various enumerators in the implementation?  
For example, what does `OMP_MAP_HOLD` become?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106509

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

[PATCH] D104743: [UpdateCCTestChecks] Implement --global-hex-value-regex

[Joel E. Denny via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG5b0a948a81e6: [UpdateCCTestChecks] Implement 
--global-hex-value-regex (authored by jdenny).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104743

Files:
  clang/test/utils/update_cc_test_checks/Inputs/global-hex-value-regex.c
  
clang/test/utils/update_cc_test_checks/Inputs/global-hex-value-regex.c.expected
  clang/test/utils/update_cc_test_checks/global-hex-value-regex.test
  llvm/utils/UpdateTestChecks/common.py

Index: llvm/utils/UpdateTestChecks/common.py
===
--- llvm/utils/UpdateTestChecks/common.py
+++ llvm/utils/UpdateTestChecks/common.py
@@ -38,10 +38,13 @@
   help='Add a prefix to FileCheck IR value names to avoid conflicts with scripted names')
   parser.add_argument('--global-value-regex', nargs='+', default=[],
   help='List of regular expressions that a global value declaration must match to generate a check (has no effect if checking globals is not enabled)')
+  parser.add_argument('--global-hex-value-regex', nargs='+', default=[],
+  help='List of regular expressions such that, for matching global value declarations, literal integer values should be encoded in hex in the associated FileCheck directives')
   args = parser.parse_args()
-  global _verbose, _global_value_regex
+  global _verbose, _global_value_regex, _global_hex_value_regex
   _verbose = args.verbose
   _global_value_regex = args.global_value_regex
+  _global_hex_value_regex = args.global_hex_value_regex
   return args
 
 
@@ -607,6 +610,12 @@
   for i, line in enumerate(lines):
 # An IR variable named '%.' matches the FileCheck regex string.
 line = line.replace('%.', '%dot')
+for regex in _global_hex_value_regex:
+  if re.match('^@' + regex + ' = ', line):
+line = re.sub(r'\bi([0-9]+) ([0-9]+)',
+lambda m : 'i' + m.group(1) + ' [[#' + hex(int(m.group(2))) + ']]',
+line)
+break
 # Ignore any comments, since the check lines will too.
 scrubbed_line = SCRUB_IR_COMMENT_RE.sub(r'', line)
 lines[i] = scrubbed_line
Index: clang/test/utils/update_cc_test_checks/global-hex-value-regex.test
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/global-hex-value-regex.test
@@ -0,0 +1,19 @@
+RUN: rm -rf %t && mkdir %t
+
+# Check --global-hex-value-regex.
+RUN: cp %S/Inputs/global-hex-value-regex.c %t/test.c
+RUN: %update_cc_test_checks %t/test.c --check-globals \
+RUN: --global-value-regex "foo\..*" "bar\..*" \
+RUN: --global-hex-value-regex ".*\.hex"
+RUN: diff -u %S/Inputs/global-hex-value-regex.c.expected %t/test.c
+
+# Check that the generated directives actually work correctly.
+RUN: cp %S/Inputs/lit.cfg.example %t/lit.cfg
+# Show lit failures while avoiding confusing FileCheck input dump nesting.
+RUN: %lit %t
+# Lit was successful.  Sanity-check the results with deterministic test order.
+RUN: rm %t/.lit_test_times.txt
+RUN: %lit %t 2>&1 | FileCheck %s
+
+CHECK: Testing: 1 tests
+CHECK: PASS: {{.*}} test.c
Index: clang/test/utils/update_cc_test_checks/Inputs/global-hex-value-regex.c.expected
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/Inputs/global-hex-value-regex.c.expected
@@ -0,0 +1,25 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --check-globals --global-value-regex "foo\..*" "bar\..*" --global-hex-value-regex ".*\.hex"
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm -o - %s | FileCheck %s
+
+//.
+// CHECK: @foo.hex = internal global i32 [[#0x10]], align 4
+// CHECK: @foo.dec = internal global i32 10, align 4
+// CHECK: @bar.hex = internal global i32 [[#0x20]], align 4
+// CHECK: @bar.dec = internal global i32 20, align 4
+//.
+// CHECK-LABEL: @foo(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret void
+//
+void foo() {
+  static int hex = 0x10;
+  static int dec = 10;
+}
+// CHECK-LABEL: @bar(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret void
+//
+void bar() {
+  static int hex = 0x20;
+  static int dec = 20;
+}
Index: clang/test/utils/update_cc_test_checks/Inputs/global-hex-value-regex.c
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/Inputs/global-hex-value-regex.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm -o - %s | FileCheck %s
+
+void foo() {
+  static int hex = 0x10;
+  static int dec = 10;
+}
+void bar() {
+  static int hex = 0x20;
+  static int dec = 20;
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/

[PATCH] D104742: [UpdateCCTestChecks] Implement --global-value-regex

[Joel E. Denny via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2f5b2ea6cd85: [UpdateCCTestChecks] Implement 
--global-value-regex (authored by jdenny).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104742

Files:
  clang/test/utils/update_cc_test_checks/Inputs/global-value-regex.c
  clang/test/utils/update_cc_test_checks/Inputs/global-value-regex.c.expected
  clang/test/utils/update_cc_test_checks/global-value-regex.test
  llvm/utils/UpdateTestChecks/common.py

Index: llvm/utils/UpdateTestChecks/common.py
===
--- llvm/utils/UpdateTestChecks/common.py
+++ llvm/utils/UpdateTestChecks/common.py
@@ -36,9 +36,12 @@
   help='List of regular expressions to replace matching value names')
   parser.add_argument('--prefix-filecheck-ir-name', default='',
   help='Add a prefix to FileCheck IR value names to avoid conflicts with scripted names')
+  parser.add_argument('--global-value-regex', nargs='+', default=[],
+  help='List of regular expressions that a global value declaration must match to generate a check (has no effect if checking globals is not enabled)')
   args = parser.parse_args()
-  global _verbose
+  global _verbose, _global_value_regex
   _verbose = args.verbose
+  _global_value_regex = args.global_value_regex
   return args
 
 
@@ -796,13 +799,27 @@
 if not glob_val_dict[checkprefix][nameless_value.check_prefix]:
   continue
 
-output_lines.append(comment_marker + SEPARATOR)
-
+check_lines = []
 global_vars_seen_before = [key for key in global_vars_seen.keys()]
 for line in glob_val_dict[checkprefix][nameless_value.check_prefix]:
+  if _global_value_regex:
+matched = False
+for regex in _global_value_regex:
+  if re.match('^@' + regex + ' = ', line):
+matched = True
+break
+if not matched:
+  continue
   tmp = generalize_check_lines([line], is_analyze, set(), global_vars_seen)
   check_line = '%s %s: %s' % (comment_marker, checkprefix, tmp[0])
+  check_lines.append(check_line)
+if not check_lines:
+  continue
+
+output_lines.append(comment_marker + SEPARATOR)
+for check_line in check_lines:
   output_lines.append(check_line)
+
 printed_prefixes.add((checkprefix, nameless_value.check_prefix))
 
 # Remembe new global variables we have not seen before
Index: clang/test/utils/update_cc_test_checks/global-value-regex.test
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/global-value-regex.test
@@ -0,0 +1,18 @@
+RUN: rm -rf %t && mkdir %t
+
+# Check --global-value-regex.
+RUN: cp %S/Inputs/global-value-regex.c %t/test.c
+RUN: %update_cc_test_checks %t/test.c --check-globals \
+RUN:   --global-value-regex "foo\.."
+RUN: diff -u %S/Inputs/global-value-regex.c.expected %t/test.c
+
+# Check that the generated directives actually work correctly.
+RUN: cp %S/Inputs/lit.cfg.example %t/lit.cfg
+# Show lit failures while avoiding confusing FileCheck input dump nesting.
+RUN: %lit %t
+# Lit was successful.  Sanity-check the results with deterministic test order.
+RUN: rm %t/.lit_test_times.txt
+RUN: %lit %t 2>&1 | FileCheck %s
+
+CHECK: Testing: 1 tests
+CHECK: PASS: {{.*}} test.c
Index: clang/test/utils/update_cc_test_checks/Inputs/global-value-regex.c.expected
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/Inputs/global-value-regex.c.expected
@@ -0,0 +1,21 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --check-globals --global-value-regex "foo\.."
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm -o - %s | FileCheck %s
+
+//.
+// CHECK: @foo.i = internal global i32 0, align 4
+// CHECK: @foo.j = internal global i32 0, align 4
+//.
+// CHECK-LABEL: @foo(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret void
+//
+void foo() {
+  static int i, j;
+}
+// CHECK-LABEL: @bar(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret void
+//
+void bar() {
+  static int i, j;
+}
Index: clang/test/utils/update_cc_test_checks/Inputs/global-value-regex.c
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/Inputs/global-value-regex.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm -o - %s | FileCheck %s
+
+void foo() {
+  static int i, j;
+}
+void bar() {
+  static int i, j;
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104743: [UpdateCCTestChecks] Implement --global-hex-value-regex

[Joel E. Denny via Phabricator via cfe-commits]

jdenny updated this revision to Diff 360131.
jdenny added a comment.

Applied @jdoerfert's suggestion: generalize to all integer values.


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

https://reviews.llvm.org/D104743

Files:
  clang/test/utils/update_cc_test_checks/Inputs/global-hex-value-regex.c
  
clang/test/utils/update_cc_test_checks/Inputs/global-hex-value-regex.c.expected
  clang/test/utils/update_cc_test_checks/global-hex-value-regex.test
  llvm/utils/UpdateTestChecks/common.py

Index: llvm/utils/UpdateTestChecks/common.py
===
--- llvm/utils/UpdateTestChecks/common.py
+++ llvm/utils/UpdateTestChecks/common.py
@@ -38,10 +38,13 @@
   help='Add a prefix to FileCheck IR value names to avoid conflicts with scripted names')
   parser.add_argument('--global-value-regex', nargs='+', default=[],
   help='List of regular expressions that a global value declaration must match to generate a check (has no effect if checking globals is not enabled)')
+  parser.add_argument('--global-hex-value-regex', nargs='+', default=[],
+  help='List of regular expressions such that, for matching global value declarations, literal integer values should be encoded in hex in the associated FileCheck directives')
   args = parser.parse_args()
-  global _verbose, _global_value_regex
+  global _verbose, _global_value_regex, _global_hex_value_regex
   _verbose = args.verbose
   _global_value_regex = args.global_value_regex
+  _global_hex_value_regex = args.global_hex_value_regex
   return args
 
 
@@ -607,6 +610,12 @@
   for i, line in enumerate(lines):
 # An IR variable named '%.' matches the FileCheck regex string.
 line = line.replace('%.', '%dot')
+for regex in _global_hex_value_regex:
+  if re.match('^@' + regex + ' = ', line):
+line = re.sub(r'\bi([0-9]+) ([0-9]+)',
+lambda m : 'i' + m.group(1) + ' [[#' + hex(int(m.group(2))) + ']]',
+line)
+break
 # Ignore any comments, since the check lines will too.
 scrubbed_line = SCRUB_IR_COMMENT_RE.sub(r'', line)
 lines[i] = scrubbed_line
Index: clang/test/utils/update_cc_test_checks/global-hex-value-regex.test
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/global-hex-value-regex.test
@@ -0,0 +1,19 @@
+RUN: rm -rf %t && mkdir %t
+
+# Check --global-hex-value-regex.
+RUN: cp %S/Inputs/global-hex-value-regex.c %t/test.c
+RUN: %update_cc_test_checks %t/test.c --check-globals \
+RUN: --global-value-regex "foo\..*" "bar\..*" \
+RUN: --global-hex-value-regex ".*\.hex"
+RUN: diff -u %S/Inputs/global-hex-value-regex.c.expected %t/test.c
+
+# Check that the generated directives actually work correctly.
+RUN: cp %S/Inputs/lit.cfg.example %t/lit.cfg
+# Show lit failures while avoiding confusing FileCheck input dump nesting.
+RUN: %lit %t
+# Lit was successful.  Sanity-check the results with deterministic test order.
+RUN: rm %t/.lit_test_times.txt
+RUN: %lit %t 2>&1 | FileCheck %s
+
+CHECK: Testing: 1 tests
+CHECK: PASS: {{.*}} test.c
Index: clang/test/utils/update_cc_test_checks/Inputs/global-hex-value-regex.c.expected
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/Inputs/global-hex-value-regex.c.expected
@@ -0,0 +1,25 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --check-globals --global-value-regex "foo\..*" "bar\..*" --global-hex-value-regex ".*\.hex"
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm -o - %s | FileCheck %s
+
+//.
+// CHECK: @foo.hex = internal global i32 [[#0x10]], align 4
+// CHECK: @foo.dec = internal global i32 10, align 4
+// CHECK: @bar.hex = internal global i32 [[#0x20]], align 4
+// CHECK: @bar.dec = internal global i32 20, align 4
+//.
+// CHECK-LABEL: @foo(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret void
+//
+void foo() {
+  static int hex = 0x10;
+  static int dec = 10;
+}
+// CHECK-LABEL: @bar(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret void
+//
+void bar() {
+  static int hex = 0x20;
+  static int dec = 20;
+}
Index: clang/test/utils/update_cc_test_checks/Inputs/global-hex-value-regex.c
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/Inputs/global-hex-value-regex.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm -o - %s | FileCheck %s
+
+void foo() {
+  static int hex = 0x10;
+  static int dec = 10;
+}
+void bar() {
+  static int hex = 0x20;
+  static int dec = 20;
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104743: [UpdateCCTestChecks] Implement --global-hex-value-regex

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D104743#2889228 , @jdoerfert wrote:

> I don't understand what we do before, and how this work,

Maybe part of the confusion is that `--global-hex-value-regex` does not change 
how the value is expected to appear in LLVM IR: decimal is still expected.  It 
only changes how the value is represented in the FileCheck directive: hex is 
the representation.  The only point is to make the FileCheck directives more 
readable because, at least in my OpenMP use case, we're dealing with flags.

> is `[[#` special in lit?

No, it starts a FileCheck numeric 
.

> Also, why i32/64 only, that seems arbitrary, no?

Good point.  I suppose it should be any integer.  I'll work on that.

Thanks for the reviews.


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

https://reviews.llvm.org/D104743

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

[PATCH] D104742: [UpdateCCTestChecks] Implement --global-value-regex

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

Ping.


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

https://reviews.llvm.org/D104742

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

[PATCH] D105873: Fix utils/update_cc_test_checks/check-globals.test on stand-alone builds

[Joel E. Denny via Phabricator via cfe-commits]

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

Thanks for the fix.  LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105873

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

[PATCH] D104742: [UpdateCCTestChecks] Implement --global-value-regex

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

Ping.


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

https://reviews.llvm.org/D104742

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

[PATCH] D104743: [UpdateCCTestChecks] Implement --global-hex-value-regex

[Joel E. Denny via Phabricator via cfe-commits]

jdenny updated this revision to Diff 355582.
jdenny added a comment.

Fix the new test for the race discussed at D104714#2841465 
.


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

https://reviews.llvm.org/D104743

Files:
  clang/test/utils/update_cc_test_checks/Inputs/global-hex-value-regex.c
  
clang/test/utils/update_cc_test_checks/Inputs/global-hex-value-regex.c.expected
  clang/test/utils/update_cc_test_checks/global-hex-value-regex.test
  llvm/utils/UpdateTestChecks/common.py

Index: llvm/utils/UpdateTestChecks/common.py
===
--- llvm/utils/UpdateTestChecks/common.py
+++ llvm/utils/UpdateTestChecks/common.py
@@ -38,10 +38,13 @@
   help='Add a prefix to FileCheck IR value names to avoid conflicts with scripted names')
   parser.add_argument('--global-value-regex', nargs='+', default=[],
   help='List of regular expressions that a global value declaration must match to generate a check (has no effect if checking globals is not enabled)')
+  parser.add_argument('--global-hex-value-regex', nargs='+', default=[],
+  help='List of regular expressions such that, for matching global value declarations, literal i32/i64 values should be encoded in hex in the associated FileCheck directives')
   args = parser.parse_args()
-  global _verbose, _global_value_regex
+  global _verbose, _global_value_regex, _global_hex_value_regex
   _verbose = args.verbose
   _global_value_regex = args.global_value_regex
+  _global_hex_value_regex = args.global_hex_value_regex
   return args
 
 
@@ -607,6 +610,12 @@
   for i, line in enumerate(lines):
 # An IR variable named '%.' matches the FileCheck regex string.
 line = line.replace('%.', '%dot')
+for regex in _global_hex_value_regex:
+  if re.match('^@' + regex + ' = ', line):
+line = re.sub(r'\bi(32|64) ([0-9]+)',
+lambda m : 'i' + m.group(1) + ' [[#' + hex(int(m.group(2))) + ']]',
+line)
+break
 # Ignore any comments, since the check lines will too.
 scrubbed_line = SCRUB_IR_COMMENT_RE.sub(r'', line)
 lines[i] = scrubbed_line
Index: clang/test/utils/update_cc_test_checks/global-hex-value-regex.test
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/global-hex-value-regex.test
@@ -0,0 +1,19 @@
+RUN: rm -rf %t && mkdir %t
+
+# Check --global-hex-value-regex.
+RUN: cp %S/Inputs/global-hex-value-regex.c %t/test.c
+RUN: %update_cc_test_checks %t/test.c --check-globals \
+RUN: --global-value-regex "foo\..*" "bar\..*" \
+RUN: --global-hex-value-regex ".*\.hex"
+RUN: diff -u %S/Inputs/global-hex-value-regex.c.expected %t/test.c
+
+# Check that the generated directives actually work correctly.
+RUN: cp %S/Inputs/lit.cfg.example %t/lit.cfg
+# Show lit failures while avoiding confusing FileCheck input dump nesting.
+RUN: %lit %t
+# Lit was successful.  Sanity-check the results with deterministic test order.
+RUN: rm %t/.lit_test_times.txt
+RUN: %lit %t 2>&1 | FileCheck %s
+
+CHECK: Testing: 1 tests
+CHECK: PASS: {{.*}} test.c
Index: clang/test/utils/update_cc_test_checks/Inputs/global-hex-value-regex.c.expected
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/Inputs/global-hex-value-regex.c.expected
@@ -0,0 +1,25 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --check-globals --global-value-regex "foo\..*" "bar\..*" --global-hex-value-regex ".*\.hex"
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm -o - %s | FileCheck %s
+
+//.
+// CHECK: @foo.hex = internal global i32 [[#0x10]], align 4
+// CHECK: @foo.dec = internal global i32 10, align 4
+// CHECK: @bar.hex = internal global i32 [[#0x20]], align 4
+// CHECK: @bar.dec = internal global i32 20, align 4
+//.
+// CHECK-LABEL: @foo(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret void
+//
+void foo() {
+  static int hex = 0x10;
+  static int dec = 10;
+}
+// CHECK-LABEL: @bar(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret void
+//
+void bar() {
+  static int hex = 0x20;
+  static int dec = 20;
+}
Index: clang/test/utils/update_cc_test_checks/Inputs/global-hex-value-regex.c
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/Inputs/global-hex-value-regex.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm -o - %s | FileCheck %s
+
+void foo() {
+  static int hex = 0x10;
+  static int dec = 10;
+}
+void bar() {
+  static int hex = 0x20;
+  static int dec = 20;
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104742: [UpdateCCTestChecks] Implement --global-value-regex

[Joel E. Denny via Phabricator via cfe-commits]

jdenny updated this revision to Diff 355579.
jdenny added a comment.

Fix the new test for the race discussed at D104714#2841465 
.


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

https://reviews.llvm.org/D104742

Files:
  clang/test/utils/update_cc_test_checks/Inputs/global-value-regex.c
  clang/test/utils/update_cc_test_checks/Inputs/global-value-regex.c.expected
  clang/test/utils/update_cc_test_checks/global-value-regex.test
  llvm/utils/UpdateTestChecks/common.py

Index: llvm/utils/UpdateTestChecks/common.py
===
--- llvm/utils/UpdateTestChecks/common.py
+++ llvm/utils/UpdateTestChecks/common.py
@@ -36,9 +36,12 @@
   help='List of regular expressions to replace matching value names')
   parser.add_argument('--prefix-filecheck-ir-name', default='',
   help='Add a prefix to FileCheck IR value names to avoid conflicts with scripted names')
+  parser.add_argument('--global-value-regex', nargs='+', default=[],
+  help='List of regular expressions that a global value declaration must match to generate a check (has no effect if checking globals is not enabled)')
   args = parser.parse_args()
-  global _verbose
+  global _verbose, _global_value_regex
   _verbose = args.verbose
+  _global_value_regex = args.global_value_regex
   return args
 
 
@@ -796,13 +799,27 @@
 if not glob_val_dict[checkprefix][nameless_value.check_prefix]:
   continue
 
-output_lines.append(comment_marker + SEPARATOR)
-
+check_lines = []
 global_vars_seen_before = [key for key in global_vars_seen.keys()]
 for line in glob_val_dict[checkprefix][nameless_value.check_prefix]:
+  if _global_value_regex:
+matched = False
+for regex in _global_value_regex:
+  if re.match('^@' + regex + ' = ', line):
+matched = True
+break
+if not matched:
+  continue
   tmp = generalize_check_lines([line], is_analyze, set(), global_vars_seen)
   check_line = '%s %s: %s' % (comment_marker, checkprefix, tmp[0])
+  check_lines.append(check_line)
+if not check_lines:
+  continue
+
+output_lines.append(comment_marker + SEPARATOR)
+for check_line in check_lines:
   output_lines.append(check_line)
+
 printed_prefixes.add((checkprefix, nameless_value.check_prefix))
 
 # Remembe new global variables we have not seen before
Index: clang/test/utils/update_cc_test_checks/global-value-regex.test
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/global-value-regex.test
@@ -0,0 +1,18 @@
+RUN: rm -rf %t && mkdir %t
+
+# Check --global-value-regex.
+RUN: cp %S/Inputs/global-value-regex.c %t/test.c
+RUN: %update_cc_test_checks %t/test.c --check-globals \
+RUN:   --global-value-regex "foo\.."
+RUN: diff -u %S/Inputs/global-value-regex.c.expected %t/test.c
+
+# Check that the generated directives actually work correctly.
+RUN: cp %S/Inputs/lit.cfg.example %t/lit.cfg
+# Show lit failures while avoiding confusing FileCheck input dump nesting.
+RUN: %lit %t
+# Lit was successful.  Sanity-check the results with deterministic test order.
+RUN: rm %t/.lit_test_times.txt
+RUN: %lit %t 2>&1 | FileCheck %s
+
+CHECK: Testing: 1 tests
+CHECK: PASS: {{.*}} test.c
Index: clang/test/utils/update_cc_test_checks/Inputs/global-value-regex.c.expected
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/Inputs/global-value-regex.c.expected
@@ -0,0 +1,21 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --check-globals --global-value-regex "foo\.."
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm -o - %s | FileCheck %s
+
+//.
+// CHECK: @foo.i = internal global i32 0, align 4
+// CHECK: @foo.j = internal global i32 0, align 4
+//.
+// CHECK-LABEL: @foo(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret void
+//
+void foo() {
+  static int i, j;
+}
+// CHECK-LABEL: @bar(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret void
+//
+void bar() {
+  static int i, j;
+}
Index: clang/test/utils/update_cc_test_checks/Inputs/global-value-regex.c
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/Inputs/global-value-regex.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm -o - %s | FileCheck %s
+
+void foo() {
+  static int i, j;
+}
+void bar() {
+  static int i, j;
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D104714: [UpdateCCTestChecks] Support --check-globals

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D104714#2841457 , @thakis wrote:

> Another thing: https://reviews.llvm.org/harbormaster/unit/view/789318/
>
>   # command stderr:
>   
> /var/lib/buildkite-agent/builds/llvm-project/clang/test/utils/update_cc_test_checks/check-globals.test:83:10:
>  error: LIT-RUN: expected string not found in input
>   LIT-RUN: PASS: {{.*}} norm.c
>   
>   Input was:
>   <<
>   1: lit.py: 
> /var/lib/buildkite-agent/builds/llvm-project/llvm/utils/lit/lit/llvm/config.py:436:
>  note: using clang: 
> /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang 
>   2: -- Testing: 2 tests, 1 workers -- 
>   3: PASS: update_cc_test_checks.py example :: norm.c (1 of 2) 
>   4: PASS: update_cc_test_checks.py example :: igf.c (2 of 2) 
>
> Looks like the test expects order igf.c, norm.c while the output is the other 
> way round. Should this just use `LIT-RUN-DAG` so that the order doesn't 
> matter, or should we add a `sorted()` somewhere that the output is 
> deterministic?

I pushed cc60fa2685bd 
 to fix 
that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104714

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

[PATCH] D104714: [UpdateCCTestChecks] Support --check-globals

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D104714#2841304 , @thakis wrote:

> I landed a fix attempt for my thing in 
> fda790fbfa0cba426d5e3787429a51e09ec64c6d 
>  .

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104714

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

[PATCH] D104714: [UpdateCCTestChecks] Support --check-globals

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D104714#2841229 , @thakis wrote:

> Oh I see this assumes that lit.site.cfg.py is below clang_obj_root. I guess 
> that's my problem, looking…

Let me know if the patch isn't general enough, or if I can help in some other 
way.

Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104714

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

[PATCH] D104714: [UpdateCCTestChecks] Support --check-globals

[Joel E. Denny via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG9eaf0d120d32: [UpdateCCTestChecks] Support --check-globals 
(authored by jdenny).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104714

Files:
  clang/test/utils/update_cc_test_checks/Inputs/check-globals.c
  clang/test/utils/update_cc_test_checks/Inputs/lit.cfg.example
  clang/test/utils/update_cc_test_checks/check-globals.test
  clang/test/utils/update_cc_test_checks/lit.local.cfg
  llvm/utils/UpdateTestChecks/common.py
  llvm/utils/update_cc_test_checks.py

Index: llvm/utils/update_cc_test_checks.py
===
--- llvm/utils/update_cc_test_checks.py
+++ llvm/utils/update_cc_test_checks.py
@@ -147,6 +147,8 @@
   help='Keep function signature information around for the check line')
   parser.add_argument('--check-attributes', action='store_true',
   help='Check "Function Attributes" for functions')
+  parser.add_argument('--check-globals', action='store_true',
+  help='Check global entries (global variables, metadata, attribute sets, ...) for functions')
   parser.add_argument('tests', nargs='+')
   args = common.parse_commandline_args(parser)
   infer_dependent_args(args)
@@ -301,6 +303,7 @@
 global_vars_seen_dict = {}
 prefix_set = set([prefix for p in filecheck_run_list for prefix in p[0]])
 output_lines = []
+has_checked_pre_function_globals = False
 
 include_generated_funcs = common.find_arg_in_test(ti,
   lambda args: ti.args.include_generated_funcs,
@@ -333,6 +336,10 @@
  prefixes,
  func_dict, func)
 
+  if ti.args.check_globals:
+common.add_global_checks(builder.global_var_dict(), '//', run_list,
+ output_lines, global_vars_seen_dict, True,
+ True)
   common.add_checks_at_end(output_lines, filecheck_run_list, builder.func_order(),
'//', lambda my_output_lines, prefixes, func:
check_generator(my_output_lines,
@@ -347,6 +354,9 @@
 m = common.CHECK_RE.match(line)
 if m and m.group(1) in prefix_set:
   continue  # Don't append the existing CHECK lines
+# Skip special separator comments added by commmon.add_global_checks.
+if line.strip() == '//' + common.SEPARATOR:
+  continue
 if idx in line2spell_and_mangled_list:
   added = set()
   for spell, mangled in line2spell_and_mangled_list[idx]:
@@ -364,6 +374,11 @@
 # line as part of common.add_ir_checks()
 output_lines.pop()
 last_line = output_lines[-1].strip()
+  if ti.args.check_globals and not has_checked_pre_function_globals:
+common.add_global_checks(builder.global_var_dict(), '//',
+ run_list, output_lines,
+ global_vars_seen_dict, True, True)
+has_checked_pre_function_globals = True
   if added:
 output_lines.append('//')
   added.add(mangled)
@@ -375,6 +390,9 @@
 if include_line:
   output_lines.append(line.rstrip('\n'))
 
+if ti.args.check_globals:
+  common.add_global_checks(builder.global_var_dict(), '//', run_list,
+   output_lines, global_vars_seen_dict, True, False)
 common.debug('Writing %d lines to %s...' % (len(output_lines), ti.path))
 with open(ti.path, 'wb') as f:
   f.writelines(['{}\n'.format(l).encode('utf-8') for l in output_lines])
Index: llvm/utils/UpdateTestChecks/common.py
===
--- llvm/utils/UpdateTestChecks/common.py
+++ llvm/utils/UpdateTestChecks/common.py
@@ -780,6 +780,8 @@
 for p in prefix_list:
   global_vars_seen = {}
   checkprefixes = p[0]
+  if checkprefixes is None:
+continue
   for checkprefix in checkprefixes:
 if checkprefix in global_vars_seen_dict:
 global_vars_seen.update(global_vars_seen_dict[checkprefix])
Index: clang/test/utils/update_cc_test_checks/lit.local.cfg
===
--- clang/test/utils/update_cc_test_checks/lit.local.cfg
+++ clang/test/utils/update_cc_test_checks/lit.local.cfg
@@ -19,9 +19,13 @@
 script_path = os.path.join(config.llvm_src_root, 'utils',
'update_cc_test_checks.py')
 assert os.path.isfile(script_path)
+lit = shell_quote(os.path.join(config.llvm_src_root, 'utils', 'lit', 'lit.py'))
+python = shell_quote(config.python_executable)
 con

[PATCH] D104714: [UpdateCCTestChecks] Support --check-globals

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D104714#2835874 , @ggeorgakoudis 
wrote:

> LGTM too!

Thanks.


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

https://reviews.llvm.org/D104714

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

[PATCH] D104714: [UpdateCCTestChecks] Support --check-globals

[Joel E. Denny via Phabricator via cfe-commits]

jdenny updated this revision to Diff 353998.
jdenny marked an inline comment as done.
jdenny added a comment.

Applied arichardson's suggestion: comment on exclusion of separator comments.

Rebased.


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

https://reviews.llvm.org/D104714

Files:
  clang/test/utils/update_cc_test_checks/Inputs/check-globals.c
  clang/test/utils/update_cc_test_checks/Inputs/lit.cfg.example
  clang/test/utils/update_cc_test_checks/check-globals.test
  clang/test/utils/update_cc_test_checks/lit.local.cfg
  llvm/utils/UpdateTestChecks/common.py
  llvm/utils/update_cc_test_checks.py

Index: llvm/utils/update_cc_test_checks.py
===
--- llvm/utils/update_cc_test_checks.py
+++ llvm/utils/update_cc_test_checks.py
@@ -147,6 +147,8 @@
   help='Keep function signature information around for the check line')
   parser.add_argument('--check-attributes', action='store_true',
   help='Check "Function Attributes" for functions')
+  parser.add_argument('--check-globals', action='store_true',
+  help='Check global entries (global variables, metadata, attribute sets, ...) for functions')
   parser.add_argument('tests', nargs='+')
   args = common.parse_commandline_args(parser)
   infer_dependent_args(args)
@@ -301,6 +303,7 @@
 global_vars_seen_dict = {}
 prefix_set = set([prefix for p in filecheck_run_list for prefix in p[0]])
 output_lines = []
+has_checked_pre_function_globals = False
 
 include_generated_funcs = common.find_arg_in_test(ti,
   lambda args: ti.args.include_generated_funcs,
@@ -333,6 +336,10 @@
  prefixes,
  func_dict, func)
 
+  if ti.args.check_globals:
+common.add_global_checks(builder.global_var_dict(), '//', run_list,
+ output_lines, global_vars_seen_dict, True,
+ True)
   common.add_checks_at_end(output_lines, filecheck_run_list, builder.func_order(),
'//', lambda my_output_lines, prefixes, func:
check_generator(my_output_lines,
@@ -347,6 +354,9 @@
 m = common.CHECK_RE.match(line)
 if m and m.group(1) in prefix_set:
   continue  # Don't append the existing CHECK lines
+# Skip special separator comments added by commmon.add_global_checks.
+if line.strip() == '//' + common.SEPARATOR:
+  continue
 if idx in line2spell_and_mangled_list:
   added = set()
   for spell, mangled in line2spell_and_mangled_list[idx]:
@@ -364,6 +374,11 @@
 # line as part of common.add_ir_checks()
 output_lines.pop()
 last_line = output_lines[-1].strip()
+  if ti.args.check_globals and not has_checked_pre_function_globals:
+common.add_global_checks(builder.global_var_dict(), '//',
+ run_list, output_lines,
+ global_vars_seen_dict, True, True)
+has_checked_pre_function_globals = True
   if added:
 output_lines.append('//')
   added.add(mangled)
@@ -375,6 +390,9 @@
 if include_line:
   output_lines.append(line.rstrip('\n'))
 
+if ti.args.check_globals:
+  common.add_global_checks(builder.global_var_dict(), '//', run_list,
+   output_lines, global_vars_seen_dict, True, False)
 common.debug('Writing %d lines to %s...' % (len(output_lines), ti.path))
 with open(ti.path, 'wb') as f:
   f.writelines(['{}\n'.format(l).encode('utf-8') for l in output_lines])
Index: llvm/utils/UpdateTestChecks/common.py
===
--- llvm/utils/UpdateTestChecks/common.py
+++ llvm/utils/UpdateTestChecks/common.py
@@ -780,6 +780,8 @@
 for p in prefix_list:
   global_vars_seen = {}
   checkprefixes = p[0]
+  if checkprefixes is None:
+continue
   for checkprefix in checkprefixes:
 if checkprefix in global_vars_seen_dict:
 global_vars_seen.update(global_vars_seen_dict[checkprefix])
Index: clang/test/utils/update_cc_test_checks/lit.local.cfg
===
--- clang/test/utils/update_cc_test_checks/lit.local.cfg
+++ clang/test/utils/update_cc_test_checks/lit.local.cfg
@@ -19,9 +19,13 @@
 script_path = os.path.join(config.llvm_src_root, 'utils',
'update_cc_test_checks.py')
 assert os.path.isfile(script_path)
+lit = shell_quote(os.path.join(config.llvm_src_root, 'utils', 'lit', 'lit.py'))
+python = shell_quote(config.python_executable)
 config.substitutions.append(
 ('%update_cc_test_checks', "%s %s %s" %

[PATCH] D104714: [UpdateCCTestChecks] Support --check-globals

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D104714#2834696 , @arichardson 
wrote:

> Thanks for working on this!

Thanks for the review.

> We have some tests downstream that check globals and currently have to use 
> `// UTC_ARGS: --disable` to manually retain them.

Does that mean you manually maintain FileCheck directives there?

> The other update script tests compare to an expected output file instead of 
> using Filecheck directives. For larger tests the separate files are a lot 
> more readable, whereas it doesn't really matter for this test.

I believe the only reason I used FileCheck directives in this case is because I 
didn't want to hard-code expected metadata and attributes, so I wanted regexes. 
 However, I don't have a lot of experience writing checks for such constructs, 
so maybe hard-coding those is not brittle at all.  If so, I'd be happy to 
switch to expected output files, which I agree are more readable.




Comment at: clang/test/utils/update_cc_test_checks/check-globals.test:34
+RUN: %lit %t
+# Lit was successful.  Sanity-check the results.
+RUN: %lit %t 2>&1 | FileCheck -check-prefix=LIT-RUN %s

arichardson wrote:
> Running lit to verify that the output is valid could be something we might 
> want to do for the other update script tests. But I guess using the scripts 
> to generate tests that are checked it is usually sufficient.
I don't know a general rule for when it's worthwhile and when it's not.

In this patch, I felt it was worthwhile because it was easy to think the 
directives looked fine without realizing declarations were in the wrong order 
(that happened to me at one point during development).  I'm not sure someone 
modifying update_cc_test_checks.py and running this test suite is necessarily 
going to know which tests in other test suites should be re-generated using 
update_cc_test_checks.py and re-run to be sure all is well.

On the other hand, I think I could be convinced it's not worthwhile in my later 
patches in this series.



Comment at: llvm/utils/update_cc_test_checks.py:357
   continue  # Don't append the existing CHECK lines
+if line.strip() == '//' + common.SEPARATOR:
+  continue

arichardson wrote:
> I feel like this line could do with a comment since it's not immediately 
> obvious why it's needed as part of this commit.
Sure, I'll work on that.


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

https://reviews.llvm.org/D104714

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

[PATCH] D104743: [UpdateCCTestChecks] Implement --global-hex-value-regex

[Joel E. Denny via Phabricator via cfe-commits]

jdenny created this revision.
jdenny added reviewers: arichardson, ggeorgakoudis, jdoerfert, MaskRay, 
mtrofin, greened.
jdenny requested review of this revision.
Herald added a subscriber: sstefan1.
Herald added projects: clang, LLVM.

For example, in OpenMP offload codegen tests, global variables like
`.offload_maptypes*` are much easier to read in hex.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104743

Files:
  clang/test/utils/update_cc_test_checks/Inputs/global-hex-value-regex.c
  
clang/test/utils/update_cc_test_checks/Inputs/global-hex-value-regex.c.expected
  clang/test/utils/update_cc_test_checks/global-hex-value-regex.test
  llvm/utils/UpdateTestChecks/common.py


Index: llvm/utils/UpdateTestChecks/common.py
===
--- llvm/utils/UpdateTestChecks/common.py
+++ llvm/utils/UpdateTestChecks/common.py
@@ -38,10 +38,13 @@
   help='Add a prefix to FileCheck IR value names to avoid 
conflicts with scripted names')
   parser.add_argument('--global-value-regex', nargs='+', default=[],
   help='List of regular expressions that a global value 
declaration must match to generate a check (has no effect if checking globals 
is not enabled)')
+  parser.add_argument('--global-hex-value-regex', nargs='+', default=[],
+  help='List of regular expressions such that, for 
matching global value declarations, literal i32/i64 values should be encoded in 
hex in the associated FileCheck directives')
   args = parser.parse_args()
-  global _verbose, _global_value_regex
+  global _verbose, _global_value_regex, _global_hex_value_regex
   _verbose = args.verbose
   _global_value_regex = args.global_value_regex
+  _global_hex_value_regex = args.global_hex_value_regex
   return args
 
 
@@ -607,6 +610,12 @@
   for i, line in enumerate(lines):
 # An IR variable named '%.' matches the FileCheck regex string.
 line = line.replace('%.', '%dot')
+for regex in _global_hex_value_regex:
+  if re.match('^@' + regex + ' = ', line):
+line = re.sub(r'\bi(32|64) ([0-9]+)',
+lambda m : 'i' + m.group(1) + ' [[#' + hex(int(m.group(2))) + ']]',
+line)
+break
 # Ignore any comments, since the check lines will too.
 scrubbed_line = SCRUB_IR_COMMENT_RE.sub(r'', line)
 lines[i] = scrubbed_line
Index: clang/test/utils/update_cc_test_checks/global-hex-value-regex.test
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/global-hex-value-regex.test
@@ -0,0 +1,18 @@
+RUN: rm -rf %t && mkdir %t
+
+# Check --global-hex-value-regex.
+RUN: cp %S/Inputs/global-hex-value-regex.c %t/test.c
+RUN: %update_cc_test_checks %t/test.c --check-globals \
+RUN: --global-value-regex "foo\..*" "bar\..*" \
+RUN: --global-hex-value-regex ".*\.hex"
+RUN: diff -u %S/Inputs/global-hex-value-regex.c.expected %t/test.c
+
+# Check that the generated directives actually work correctly.
+RUN: cp %S/Inputs/lit.cfg.example %t/lit.cfg
+# Show lit failures while avoiding confusing FileCheck input dump nesting.
+RUN: %lit %t
+# Lit was successful.  Sanity-check the results.
+RUN: %lit %t 2>&1 | FileCheck %s
+
+CHECK: Testing: 1 tests
+CHECK: PASS: {{.*}} test.c
Index: 
clang/test/utils/update_cc_test_checks/Inputs/global-hex-value-regex.c.expected
===
--- /dev/null
+++ 
clang/test/utils/update_cc_test_checks/Inputs/global-hex-value-regex.c.expected
@@ -0,0 +1,25 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py 
UTC_ARGS: --check-globals --global-value-regex "foo\..*" "bar\..*" 
--global-hex-value-regex ".*\.hex"
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm -o - %s | FileCheck 
%s
+
+//.
+// CHECK: @foo.hex = internal global i32 [[#0x10]], align 4
+// CHECK: @foo.dec = internal global i32 10, align 4
+// CHECK: @bar.hex = internal global i32 [[#0x20]], align 4
+// CHECK: @bar.dec = internal global i32 20, align 4
+//.
+// CHECK-LABEL: @foo(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret void
+//
+void foo() {
+  static int hex = 0x10;
+  static int dec = 10;
+}
+// CHECK-LABEL: @bar(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret void
+//
+void bar() {
+  static int hex = 0x20;
+  static int dec = 20;
+}
Index: clang/test/utils/update_cc_test_checks/Inputs/global-hex-value-regex.c
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/Inputs/global-hex-value-regex.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm -o - %s | FileCheck 
%s
+
+void foo() {
+  static int hex = 0x10;
+  static int dec = 10;
+}
+void bar() {
+  static int hex = 0x20;
+  static int dec = 20;
+}


Index: llvm/utils/UpdateTestChecks/common.py
===
--- llvm/

[PATCH] D104742: [UpdateCCTestChecks] Implement --global-value-regex

[Joel E. Denny via Phabricator via cfe-commits]

jdenny created this revision.
jdenny added reviewers: arichardson, ggeorgakoudis, jdoerfert, MaskRay, 
mtrofin, greened.
jdenny requested review of this revision.
Herald added a subscriber: sstefan1.
Herald added projects: clang, LLVM.

`--check-globals` activates checks for all global values, and
`--global-value-regex` filters them.  For example, I'd like to use it
in OpenMP offload codegen tests to check only global variables like
`.offload_maptypes*`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104742

Files:
  clang/test/utils/update_cc_test_checks/Inputs/global-value-regex.c
  clang/test/utils/update_cc_test_checks/Inputs/global-value-regex.c.expected
  clang/test/utils/update_cc_test_checks/global-value-regex.test
  llvm/utils/UpdateTestChecks/common.py

Index: llvm/utils/UpdateTestChecks/common.py
===
--- llvm/utils/UpdateTestChecks/common.py
+++ llvm/utils/UpdateTestChecks/common.py
@@ -36,9 +36,12 @@
   help='List of regular expressions to replace matching value names')
   parser.add_argument('--prefix-filecheck-ir-name', default='',
   help='Add a prefix to FileCheck IR value names to avoid conflicts with scripted names')
+  parser.add_argument('--global-value-regex', nargs='+', default=[],
+  help='List of regular expressions that a global value declaration must match to generate a check (has no effect if checking globals is not enabled)')
   args = parser.parse_args()
-  global _verbose
+  global _verbose, _global_value_regex
   _verbose = args.verbose
+  _global_value_regex = args.global_value_regex
   return args
 
 
@@ -796,13 +799,27 @@
 if not glob_val_dict[checkprefix][nameless_value.check_prefix]:
   continue
 
-output_lines.append(comment_marker + SEPARATOR)
-
+check_lines = []
 global_vars_seen_before = [key for key in global_vars_seen.keys()]
 for line in glob_val_dict[checkprefix][nameless_value.check_prefix]:
+  if _global_value_regex:
+matched = False
+for regex in _global_value_regex:
+  if re.match('^@' + regex + ' = ', line):
+matched = True
+break
+if not matched:
+  continue
   tmp = generalize_check_lines([line], is_analyze, set(), global_vars_seen)
   check_line = '%s %s: %s' % (comment_marker, checkprefix, tmp[0])
+  check_lines.append(check_line)
+if not check_lines:
+  continue
+
+output_lines.append(comment_marker + SEPARATOR)
+for check_line in check_lines:
   output_lines.append(check_line)
+
 printed_prefixes.add((checkprefix, nameless_value.check_prefix))
 
 # Remembe new global variables we have not seen before
Index: clang/test/utils/update_cc_test_checks/global-value-regex.test
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/global-value-regex.test
@@ -0,0 +1,17 @@
+RUN: rm -rf %t && mkdir %t
+
+# Check --global-value-regex.
+RUN: cp %S/Inputs/global-value-regex.c %t/test.c
+RUN: %update_cc_test_checks %t/test.c --check-globals \
+RUN:   --global-value-regex "foo\.."
+RUN: diff -u %S/Inputs/global-value-regex.c.expected %t/test.c
+
+# Check that the generated directives actually work correctly.
+RUN: cp %S/Inputs/lit.cfg.example %t/lit.cfg
+# Show lit failures while avoiding confusing FileCheck input dump nesting.
+RUN: %lit %t
+# Lit was successful.  Sanity-check the results.
+RUN: %lit %t 2>&1 | FileCheck %s
+
+CHECK: Testing: 1 tests
+CHECK: PASS: {{.*}} test.c
Index: clang/test/utils/update_cc_test_checks/Inputs/global-value-regex.c.expected
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/Inputs/global-value-regex.c.expected
@@ -0,0 +1,21 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --check-globals --global-value-regex "foo\.."
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm -o - %s | FileCheck %s
+
+//.
+// CHECK: @foo.i = internal global i32 0, align 4
+// CHECK: @foo.j = internal global i32 0, align 4
+//.
+// CHECK-LABEL: @foo(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret void
+//
+void foo() {
+  static int i, j;
+}
+// CHECK-LABEL: @bar(
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:ret void
+//
+void bar() {
+  static int i, j;
+}
Index: clang/test/utils/update_cc_test_checks/Inputs/global-value-regex.c
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/Inputs/global-value-regex.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm -o - %s | FileCheck %s
+
+void foo() {
+  static int i, j;
+}
+void bar() {
+  static int i, j;
+}
___
cfe-com

[PATCH] D104714: [UpdateCCTestChecks] Support --check-globals

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D104714#2833238 , @jdoerfert wrote:

> LG, cool :)

Thanks for the reviews.


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

https://reviews.llvm.org/D104714

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

[PATCH] D104714: [UpdateCCTestChecks] Support --check-globals

[Joel E. Denny via Phabricator via cfe-commits]

jdenny updated this revision to Diff 353765.
jdenny added a comment.

Fixed the reported test failure.  (Sorry, I had accidentally put that fix in a 
later patch even though the associated test is in this patch.)


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

https://reviews.llvm.org/D104714

Files:
  clang/test/utils/update_cc_test_checks/Inputs/check-globals.c
  clang/test/utils/update_cc_test_checks/Inputs/lit.cfg.example
  clang/test/utils/update_cc_test_checks/check-globals.test
  clang/test/utils/update_cc_test_checks/lit.local.cfg
  llvm/utils/UpdateTestChecks/common.py
  llvm/utils/update_cc_test_checks.py

Index: llvm/utils/update_cc_test_checks.py
===
--- llvm/utils/update_cc_test_checks.py
+++ llvm/utils/update_cc_test_checks.py
@@ -147,6 +147,8 @@
   help='Keep function signature information around for the check line')
   parser.add_argument('--check-attributes', action='store_true',
   help='Check "Function Attributes" for functions')
+  parser.add_argument('--check-globals', action='store_true',
+  help='Check global entries (global variables, metadata, attribute sets, ...) for functions')
   parser.add_argument('tests', nargs='+')
   args = common.parse_commandline_args(parser)
   infer_dependent_args(args)
@@ -301,6 +303,7 @@
 global_vars_seen_dict = {}
 prefix_set = set([prefix for p in filecheck_run_list for prefix in p[0]])
 output_lines = []
+has_checked_pre_function_globals = False
 
 include_generated_funcs = common.find_arg_in_test(ti,
   lambda args: ti.args.include_generated_funcs,
@@ -333,6 +336,10 @@
  prefixes,
  func_dict, func)
 
+  if ti.args.check_globals:
+common.add_global_checks(builder.global_var_dict(), '//', run_list,
+ output_lines, global_vars_seen_dict, True,
+ True)
   common.add_checks_at_end(output_lines, filecheck_run_list, builder.func_order(),
'//', lambda my_output_lines, prefixes, func:
check_generator(my_output_lines,
@@ -347,6 +354,8 @@
 m = common.CHECK_RE.match(line)
 if m and m.group(1) in prefix_set:
   continue  # Don't append the existing CHECK lines
+if line.strip() == '//' + common.SEPARATOR:
+  continue
 if idx in line2spell_and_mangled_list:
   added = set()
   for spell, mangled in line2spell_and_mangled_list[idx]:
@@ -364,6 +373,11 @@
 # line as part of common.add_ir_checks()
 output_lines.pop()
 last_line = output_lines[-1].strip()
+  if ti.args.check_globals and not has_checked_pre_function_globals:
+common.add_global_checks(builder.global_var_dict(), '//',
+ run_list, output_lines,
+ global_vars_seen_dict, True, True)
+has_checked_pre_function_globals = True
   if added:
 output_lines.append('//')
   added.add(mangled)
@@ -375,6 +389,9 @@
 if include_line:
   output_lines.append(line.rstrip('\n'))
 
+if ti.args.check_globals:
+  common.add_global_checks(builder.global_var_dict(), '//', run_list,
+   output_lines, global_vars_seen_dict, True, False)
 common.debug('Writing %d lines to %s...' % (len(output_lines), ti.path))
 with open(ti.path, 'wb') as f:
   f.writelines(['{}\n'.format(l).encode('utf-8') for l in output_lines])
Index: llvm/utils/UpdateTestChecks/common.py
===
--- llvm/utils/UpdateTestChecks/common.py
+++ llvm/utils/UpdateTestChecks/common.py
@@ -780,6 +780,8 @@
 for p in prefix_list:
   global_vars_seen = {}
   checkprefixes = p[0]
+  if checkprefixes is None:
+continue
   for checkprefix in checkprefixes:
 if checkprefix in global_vars_seen_dict:
 global_vars_seen.update(global_vars_seen_dict[checkprefix])
Index: clang/test/utils/update_cc_test_checks/lit.local.cfg
===
--- clang/test/utils/update_cc_test_checks/lit.local.cfg
+++ clang/test/utils/update_cc_test_checks/lit.local.cfg
@@ -19,9 +19,13 @@
 script_path = os.path.join(config.llvm_src_root, 'utils',
'update_cc_test_checks.py')
 assert os.path.isfile(script_path)
+lit = shell_quote(os.path.join(config.llvm_src_root, 'utils', 'lit', 'lit.py'))
+python = shell_quote(config.python_executable)
 config.substitutions.append(
 ('%update_cc_test_checks', "%s %s %s" % (
-shell_quote(config.python_executable), shell_quote(sc

[PATCH] D104714: [UpdateCCTestChecks] Support --check-globals

[Joel E. Denny via Phabricator via cfe-commits]

jdenny created this revision.
jdenny added reviewers: arichardson, ggeorgakoudis, jdoerfert, MaskRay, 
mtrofin, greened.
jdenny requested review of this revision.
Herald added a subscriber: sstefan1.
Herald added projects: clang, LLVM.

This option is already supported by update_test_checks.py, but it can
also be useful in update_cc_test_checks.py.  For example, I'd like to
use it in OpenMP offload codegen tests to check global variables like
`.offload_maptypes*`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104714

Files:
  clang/test/utils/update_cc_test_checks/Inputs/check-globals.c
  clang/test/utils/update_cc_test_checks/Inputs/lit.cfg.example
  clang/test/utils/update_cc_test_checks/check-globals.test
  clang/test/utils/update_cc_test_checks/lit.local.cfg
  llvm/utils/update_cc_test_checks.py

Index: llvm/utils/update_cc_test_checks.py
===
--- llvm/utils/update_cc_test_checks.py
+++ llvm/utils/update_cc_test_checks.py
@@ -147,6 +147,8 @@
   help='Keep function signature information around for the check line')
   parser.add_argument('--check-attributes', action='store_true',
   help='Check "Function Attributes" for functions')
+  parser.add_argument('--check-globals', action='store_true',
+  help='Check global entries (global variables, metadata, attribute sets, ...) for functions')
   parser.add_argument('tests', nargs='+')
   args = common.parse_commandline_args(parser)
   infer_dependent_args(args)
@@ -300,6 +302,7 @@
 global_vars_seen_dict = {}
 prefix_set = set([prefix for p in filecheck_run_list for prefix in p[0]])
 output_lines = []
+has_checked_pre_function_globals = False
 
 include_generated_funcs = common.find_arg_in_test(ti,
   lambda args: ti.args.include_generated_funcs,
@@ -332,6 +335,10 @@
  prefixes,
  func_dict, func)
 
+  if ti.args.check_globals:
+common.add_global_checks(builder.global_var_dict(), '//', run_list,
+ output_lines, global_vars_seen_dict, True,
+ True)
   common.add_checks_at_end(output_lines, filecheck_run_list, builder.func_order(),
'//', lambda my_output_lines, prefixes, func:
check_generator(my_output_lines,
@@ -346,6 +353,8 @@
 m = common.CHECK_RE.match(line)
 if m and m.group(1) in prefix_set:
   continue  # Don't append the existing CHECK lines
+if line.strip() == '//' + common.SEPARATOR:
+  continue
 if idx in line2spell_and_mangled_list:
   added = set()
   for spell, mangled in line2spell_and_mangled_list[idx]:
@@ -363,6 +372,11 @@
 # line as part of common.add_ir_checks()
 output_lines.pop()
 last_line = output_lines[-1].strip()
+  if ti.args.check_globals and not has_checked_pre_function_globals:
+common.add_global_checks(builder.global_var_dict(), '//',
+ run_list, output_lines,
+ global_vars_seen_dict, True, True)
+has_checked_pre_function_globals = True
   if added:
 output_lines.append('//')
   added.add(mangled)
@@ -374,6 +388,9 @@
 if include_line:
   output_lines.append(line.rstrip('\n'))
 
+if ti.args.check_globals:
+  common.add_global_checks(builder.global_var_dict(), '//', run_list,
+   output_lines, global_vars_seen_dict, True, False)
 common.debug('Writing %d lines to %s...' % (len(output_lines), ti.path))
 with open(ti.path, 'wb') as f:
   f.writelines(['{}\n'.format(l).encode('utf-8') for l in output_lines])
Index: clang/test/utils/update_cc_test_checks/lit.local.cfg
===
--- clang/test/utils/update_cc_test_checks/lit.local.cfg
+++ clang/test/utils/update_cc_test_checks/lit.local.cfg
@@ -19,9 +19,13 @@
 script_path = os.path.join(config.llvm_src_root, 'utils',
'update_cc_test_checks.py')
 assert os.path.isfile(script_path)
+lit = shell_quote(os.path.join(config.llvm_src_root, 'utils', 'lit', 'lit.py'))
+python = shell_quote(config.python_executable)
 config.substitutions.append(
 ('%update_cc_test_checks', "%s %s %s" % (
-shell_quote(config.python_executable), shell_quote(script_path),
-extra_args)))
+python, shell_quote(script_path), extra_args)))
 config.substitutions.append(
 ('%clang_tools_dir', shell_quote(config.clang_tools_dir)))
+config.substitutions.append(
+('%lit', "%s %s -Dclang_obj_root=%s -j1 -vv" % (
+python, lit, shell_quote(config.clang_obj_root)))

[PATCH] D104566: [UpdateCCTestChecks] Fix --replace-value-regex across RUN lines

[Joel E. Denny via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG2bfe0536e514: [UpdateCCTestChecks] Fix --replace-value-regex 
across RUN lines (authored by jdenny).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104566

Files:
  
clang/test/utils/update_cc_test_checks/Inputs/replace-value-regex-across-runs.c
  
clang/test/utils/update_cc_test_checks/Inputs/replace-value-regex-across-runs.c.expected
  clang/test/utils/update_cc_test_checks/replace-value-regex-across-runs.test
  llvm/utils/UpdateTestChecks/common.py

Index: llvm/utils/UpdateTestChecks/common.py
===
--- llvm/utils/UpdateTestChecks/common.py
+++ llvm/utils/UpdateTestChecks/common.py
@@ -351,27 +351,6 @@
 for l in scrubbed_body.splitlines():
   print('  ' + l, file=sys.stderr)
   for prefix in prefixes:
-if func in self._func_dict[prefix]:
-  if (self._func_dict[prefix][func] is None or
-  str(self._func_dict[prefix][func]) != scrubbed_body or
-  self._func_dict[prefix][func].args_and_sig != args_and_sig or
-  self._func_dict[prefix][func].attrs != attrs):
-if (self._func_dict[prefix][func] is not None and
-self._func_dict[prefix][func].is_same_except_arg_names(
-scrubbed_extra,
-args_and_sig,
-attrs)):
-  self._func_dict[prefix][func].scrub = scrubbed_extra
-  self._func_dict[prefix][func].args_and_sig = args_and_sig
-  continue
-else:
-  # This means a previous RUN line produced a body for this function
-  # that is different from the one produced by this current RUN line,
-  # so the body can't be common accross RUN lines. We use None to
-  # indicate that.
-  self._func_dict[prefix][func] = None
-  continue
-
 # Replace function names matching the regex.
 for regex in self._replace_value_regex:
   # Pattern that matches capture groups in the regex in leftmost order.
@@ -394,7 +373,29 @@
 func_repl = group_regex.sub(re.escape(g), func_repl, count=1)
 # Substitute function call names that match the regex with the same
 # capture groups set.
-scrubbed_body = re.sub(func_repl, '{{' + func_repl + '}}', scrubbed_body)
+scrubbed_body = re.sub(func_repl, '{{' + func_repl + '}}',
+   scrubbed_body)
+
+if func in self._func_dict[prefix]:
+  if (self._func_dict[prefix][func] is None or
+  str(self._func_dict[prefix][func]) != scrubbed_body or
+  self._func_dict[prefix][func].args_and_sig != args_and_sig or
+  self._func_dict[prefix][func].attrs != attrs):
+if (self._func_dict[prefix][func] is not None and
+self._func_dict[prefix][func].is_same_except_arg_names(
+scrubbed_extra,
+args_and_sig,
+attrs)):
+  self._func_dict[prefix][func].scrub = scrubbed_extra
+  self._func_dict[prefix][func].args_and_sig = args_and_sig
+  continue
+else:
+  # This means a previous RUN line produced a body for this function
+  # that is different from the one produced by this current RUN line,
+  # so the body can't be common accross RUN lines. We use None to
+  # indicate that.
+  self._func_dict[prefix][func] = None
+  continue
 
 self._func_dict[prefix][func] = function_body(
 scrubbed_body, scrubbed_extra, args_and_sig, attrs)
Index: clang/test/utils/update_cc_test_checks/replace-value-regex-across-runs.test
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/replace-value-regex-across-runs.test
@@ -0,0 +1,7 @@
+# Test that --replace-value-regex is applied correctly when multiple RUN lines
+# use the same FileCheck prefix and have the same output.
+
+RUN: cp %S/Inputs/replace-value-regex-across-runs.c %t.c
+RUN: %update_cc_test_checks %t.c \
+RUN: --replace-value-regex '__omp_offloading_[0-9a-z]+_[0-9a-z]+'
+RUN: diff -u %S/Inputs/replace-value-regex-across-runs.c.expected %t.c
Index: clang/test/utils/update_cc_test_checks/Inputs/replace-value-regex-across-runs.c.expected
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/Inputs/replace-value-regex-across-runs.c.expected
@@ -0,0 +1,15 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --replace-value-regex "__omp_offloading_[0-9a-z]+_[0-9a-z]+"
+/

[PATCH] D104566: [UpdateCCTestChecks] Fix --replace-value-regex across RUN lines

[Joel E. Denny via Phabricator via cfe-commits]

jdenny created this revision.
jdenny added reviewers: arichardson, ggeorgakoudis, jdoerfert, MaskRay, 
mtrofin, greened.
jdenny requested review of this revision.
Herald added a subscriber: sstefan1.
Herald added projects: clang, LLVM.

Without this patch, llvm/utils/update_cc_test_checks.py fails to
perform `--replace-value-regex` replacements when two RUN lines
produce the same output and use the same single FileCheck prefix.  The 
problem is that replacements in a RUN line's output are not performed
until after comparing against previous RUN lines' output, where
replacements have already been performed.  This patch fixes that.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104566

Files:
  
clang/test/utils/update_cc_test_checks/Inputs/replace-value-regex-across-runs.c
  
clang/test/utils/update_cc_test_checks/Inputs/replace-value-regex-across-runs.c.expected
  clang/test/utils/update_cc_test_checks/replace-value-regex-across-runs.test
  llvm/utils/UpdateTestChecks/common.py

Index: llvm/utils/UpdateTestChecks/common.py
===
--- llvm/utils/UpdateTestChecks/common.py
+++ llvm/utils/UpdateTestChecks/common.py
@@ -350,27 +350,6 @@
 for l in scrubbed_body.splitlines():
   print('  ' + l, file=sys.stderr)
   for prefix in prefixes:
-if func in self._func_dict[prefix]:
-  if (self._func_dict[prefix][func] is None or
-  str(self._func_dict[prefix][func]) != scrubbed_body or
-  self._func_dict[prefix][func].args_and_sig != args_and_sig or
-  self._func_dict[prefix][func].attrs != attrs):
-if (self._func_dict[prefix][func] is not None and
-self._func_dict[prefix][func].is_same_except_arg_names(
-scrubbed_extra,
-args_and_sig,
-attrs)):
-  self._func_dict[prefix][func].scrub = scrubbed_extra
-  self._func_dict[prefix][func].args_and_sig = args_and_sig
-  continue
-else:
-  # This means a previous RUN line produced a body for this function
-  # that is different from the one produced by this current RUN line,
-  # so the body can't be common accross RUN lines. We use None to
-  # indicate that.
-  self._func_dict[prefix][func] = None
-  continue
-
 # Replace function names matching the regex.
 for regex in self._replace_value_regex:
   # Pattern that matches capture groups in the regex in leftmost order.
@@ -393,7 +372,29 @@
 func_repl = group_regex.sub(re.escape(g), func_repl, count=1)
 # Substitute function call names that match the regex with the same
 # capture groups set.
-scrubbed_body = re.sub(func_repl, '{{' + func_repl + '}}', scrubbed_body)
+scrubbed_body = re.sub(func_repl, '{{' + func_repl + '}}',
+   scrubbed_body)
+
+if func in self._func_dict[prefix]:
+  if (self._func_dict[prefix][func] is None or
+  str(self._func_dict[prefix][func]) != scrubbed_body or
+  self._func_dict[prefix][func].args_and_sig != args_and_sig or
+  self._func_dict[prefix][func].attrs != attrs):
+if (self._func_dict[prefix][func] is not None and
+self._func_dict[prefix][func].is_same_except_arg_names(
+scrubbed_extra,
+args_and_sig,
+attrs)):
+  self._func_dict[prefix][func].scrub = scrubbed_extra
+  self._func_dict[prefix][func].args_and_sig = args_and_sig
+  continue
+else:
+  # This means a previous RUN line produced a body for this function
+  # that is different from the one produced by this current RUN line,
+  # so the body can't be common accross RUN lines. We use None to
+  # indicate that.
+  self._func_dict[prefix][func] = None
+  continue
 
 self._func_dict[prefix][func] = function_body(
 scrubbed_body, scrubbed_extra, args_and_sig, attrs)
Index: clang/test/utils/update_cc_test_checks/replace-value-regex-across-runs.test
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/replace-value-regex-across-runs.test
@@ -0,0 +1,7 @@
+# Test that --replace-value-regex is applied correctly when multiple RUN lines
+# use the same FileCheck prefix and have the same output.
+
+RUN: cp %S/Inputs/replace-value-regex-across-runs.c %t.c
+RUN: %update_cc_test_checks %t.c \
+RUN: --replace-value-regex '__omp_offloading_[0-9a-z]+_[0-9a-z]+'
+RUN: diff -u %S/Inputs/replace-value-regex-across-runs.c.expected %t.c
Index: clang/test/utils/update_cc_test_checks/Inputs/replace-value-regex-across-runs.c.expected
==

[PATCH] D101564: [OpenMP] Fix second debug name from map clause

[Joel E. Denny via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG82e99f50351d: [OpenMP] Fix second debug name from map clause 
(authored by jdenny).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101564

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/test/OpenMP/target_map_names.cpp


Index: clang/test/OpenMP/target_map_names.cpp
===
--- clang/test/OpenMP/target_map_names.cpp
+++ clang/test/OpenMP/target_map_names.cpp
@@ -16,7 +16,6 @@
 // DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";s.ps->ps;{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
 // DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";s.ps->ps->ps;{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
 // DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";s.ps->ps->s.f[:22];{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
-// DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";s.ps->ps->s.f[:22];{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
 // DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";ps;{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
 // DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";ps->i;{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
 // DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";ps->s.f;{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
@@ -25,7 +24,6 @@
 // DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";ps->ps->ps;{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
 // DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";ps->ps->ps->ps;{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
 // DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";ps->ps->ps->s.f[:22];{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
-// DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";ps->ps->ps->s.f[:22];{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
 // DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";s.f[:22];{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
 // DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";s.p[:33];{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
 // DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";ps->p[:33];{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
@@ -181,6 +179,18 @@
 
 // DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";s.Z[0:64];{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
 
+// Clang used to mistakenly generate the map name "x" for both x and y on this
+// directive.  Conditions to reproduce the bug: a single map clause has two
+// variables, and at least the second is used in the associated statement.
+//
+// DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";x;{{.*}}.cpp;[[@LINE+3]];7;;\00"
+// DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";y;{{.*}}.cpp;[[@LINE+2]];10;;\00"
+void secondMapNameInClause() {
+  int x, y;
+  #pragma omp target map(to: x, y)
+  x = y = 1;
+}
+
 // DEBUG: %{{.+}} = call i32 @__tgt_target_mapper(%struct.ident_t* @{{.+}}, 
i64 -1, i8* @{{.+}}, i32 1, i8** %{{.+}}, i8** %{{.+}}, i64* {{.+}}, i64* 
{{.+}}, i8** getelementptr inbounds ([{{[0-9]+}} x i8*], [{{[0-9]+}} x i8*]* 
@.offload_mapnames{{.*}}, i32 0, i32 0), i8** {{.+}})
 // DEBUG: %{{.+}} = call i32 @__tgt_target_teams_mapper(%struct.ident_t* 
@{{.+}}, i64 -1, i8* @{{.+}}, i32 1, i8** %{{.+}}, i8** %{{.+}}, i64* {{.+}}, 
i64* {{.+}}, i8** getelementptr inbounds ([{{[0-9]+}} x i8*], [{{[0-9]+}} x 
i8*]* @.offload_mapnames{{.*}}, i32 0, i32 0), i8** {{.+}}, i32 {{.+}}, i32 
{{.+}})
 // DEBUG: call void @__tgt_target_data_begin_mapper(%struct.ident_t* @{{.+}}, 
i64 -1, i32 1, i8** %{{.+}}, i8** %{{.+}}, i64* {{.+}}, i64* {{.+}}, i8** 
getelementptr inbounds ([{{[0-9]+}} x i8*], [{{[0-9]+}} x i8*]* 
@.offload_mapnames{{.*}}, i32 0, i32 0), i8** {{.+}})
Index: clang/lib/CodeGen/CGOpenMPRuntime.cpp
===
--- clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -7763,6 +7763,8 @@
   const ValueDecl *MapDecl = (I->getAssociatedDeclaration())
  ? I->getAssociatedDeclaration()
  : BaseDecl;
+  MapExpr = (I->getAssociatedExpression()) ? I->getAssociatedExpression()
+   : MapExpr;
 
   // Get information on whether the element is a pointer. Have to do a
   // special treatment for array sections given that they are built-in


Index: clang/test/OpenMP/target_map_names.cpp
===
--- clang/test/OpenMP/target_map_names.cpp
+++ clang/test/OpenMP/target_map_names.cpp
@@ -16,7 +16,6 @@
 // DEBUG: @{{[0-9]+}} = private unnamed_addr constant [

[PATCH] D101564: [OpenMP] Fix second debug name from map clause

[Joel E. Denny via Phabricator via cfe-commits]

jdenny created this revision.
jdenny added reviewers: jhuber6, jdoerfert.
Herald added subscribers: guansong, yaxunl.
jdenny requested review of this revision.
Herald added a subscriber: sstefan1.
Herald added a project: clang.

This patch fixes a bug from D89802 .  For 
example, without it, Clang
generates x as the debug map name for both x and y in the following
example:

  #pragma omp target map(to: x, y)
  x = y = 1;


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D101564

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/test/OpenMP/target_map_names.cpp


Index: clang/test/OpenMP/target_map_names.cpp
===
--- clang/test/OpenMP/target_map_names.cpp
+++ clang/test/OpenMP/target_map_names.cpp
@@ -16,7 +16,6 @@
 // DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";s.ps->ps;{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
 // DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";s.ps->ps->ps;{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
 // DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";s.ps->ps->s.f[:22];{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
-// DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";s.ps->ps->s.f[:22];{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
 // DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";ps;{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
 // DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";ps->i;{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
 // DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";ps->s.f;{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
@@ -25,7 +24,6 @@
 // DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";ps->ps->ps;{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
 // DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";ps->ps->ps->ps;{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
 // DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";ps->ps->ps->s.f[:22];{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
-// DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";ps->ps->ps->s.f[:22];{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
 // DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";s.f[:22];{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
 // DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";s.p[:33];{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
 // DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";ps->p[:33];{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
@@ -181,6 +179,18 @@
 
 // DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";s.Z[0:64];{{.*}}.cpp;{{[0-9]+}};{{[0-9]+}};;\00"
 
+// Clang used to mistakenly generate the map name "x" for both x and y on this
+// directive.  Conditions to reproduce the bug: a single map clause has two
+// variables, and at least the second is used in the associated statement.
+//
+// DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";x;{{.*}}.cpp;[[@LINE+3]];7;;\00"
+// DEBUG: @{{[0-9]+}} = private unnamed_addr constant [{{[0-9]+}} x i8] 
c";y;{{.*}}.cpp;[[@LINE+2]];10;;\00"
+void secondMapNameInClause() {
+  int x, y;
+  #pragma omp target map(to: x, y)
+  x = y = 1;
+}
+
 // DEBUG: %{{.+}} = call i32 @__tgt_target_mapper(%struct.ident_t* @{{.+}}, 
i64 -1, i8* @{{.+}}, i32 1, i8** %{{.+}}, i8** %{{.+}}, i64* {{.+}}, i64* 
{{.+}}, i8** getelementptr inbounds ([{{[0-9]+}} x i8*], [{{[0-9]+}} x i8*]* 
@.offload_mapnames{{.*}}, i32 0, i32 0), i8** {{.+}})
 // DEBUG: %{{.+}} = call i32 @__tgt_target_teams_mapper(%struct.ident_t* 
@{{.+}}, i64 -1, i8* @{{.+}}, i32 1, i8** %{{.+}}, i8** %{{.+}}, i64* {{.+}}, 
i64* {{.+}}, i8** getelementptr inbounds ([{{[0-9]+}} x i8*], [{{[0-9]+}} x 
i8*]* @.offload_mapnames{{.*}}, i32 0, i32 0), i8** {{.+}}, i32 {{.+}}, i32 
{{.+}})
 // DEBUG: call void @__tgt_target_data_begin_mapper(%struct.ident_t* @{{.+}}, 
i64 -1, i32 1, i8** %{{.+}}, i8** %{{.+}}, i64* {{.+}}, i64* {{.+}}, i8** 
getelementptr inbounds ([{{[0-9]+}} x i8*], [{{[0-9]+}} x i8*]* 
@.offload_mapnames{{.*}}, i32 0, i32 0), i8** {{.+}})
Index: clang/lib/CodeGen/CGOpenMPRuntime.cpp
===
--- clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -7764,6 +7764,8 @@
   const ValueDecl *MapDecl = (I->getAssociatedDeclaration())
  ? I->getAssociatedDeclaration()
  : BaseDecl;
+  MapExpr = (I->getAssociatedExpression()) ? I->getAssociatedExpression()
+   : MapExpr;
 
   // Get information on whether the element is a pointer. Have to do a
   // special treatment for array sections given that they are built-in


Index: clang/test/OpenMP/target_map_names.cpp
=

[PATCH] D94973: [clang][OpenMP] Use OpenMPIRBuilder for workshare loops.

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D94973#2591210 , @Meinersbur wrote:

> In D94973#2590867 , @jdenny wrote:
>
>> One property of this patch that has bothered me is that OMPCanonicalLoop is 
>> not a loop.  Instead, it's an AST node that is sandwiched between a 
>> directive and a loop to contain extra information about the loop.  The 
>> TreeTransform issues we've been discussing highlight how that extra node can 
>> be confusing.
>
> There are many other AST nodes that are sandwiched carrying semantic 
> information. Examples: AttributedStmt, ImplicitCast, CapturedStmt of any 
> OMPExecutableStmt, ExprWithCleanups, CXXRewrittenBinaryOperator, 
> CXXBindTemporaryExpr.

Thanks for the examples.  I never thought of those cases in that way probably 
because they're easier to imagine as distinct constructs or operations.  But 
I'm probably splitting hairs.

> I even think that representing semantic information alongside of syntax is 
> one of the principles of Clang's AST.

Interesting.  Is that documented somewhere?

>> Now I remember that there's D95496 , which 
>> seems not to have that property.  I haven't reviewed that patch in any 
>> detail, and I haven't tried to locate any prior discussions.  Can you please 
>> summarize why you prefer the current patch over that one?
>
> D95496  is a lot more invasive to code that 
> does not even use OpenMP.

Fair enough.




Comment at: clang/lib/Sema/TreeTransform.h:8321
+TreeTransform::TransformOMPCanonicalLoop(OMPCanonicalLoop *L) {
+  // The OMPCanonicalLoop will be recreated when transforming the 
loop-associted
+  // directive.

Meinersbur wrote:
> jdenny wrote:
> > Meinersbur wrote:
> > > jdenny wrote:
> > > > Meinersbur wrote:
> > > > > jdenny wrote:
> > > > > > Meinersbur wrote:
> > > > > > > jdenny wrote:
> > > > > > > > Meinersbur wrote:
> > > > > > > > > jdenny wrote:
> > > > > > > > > > I'm used to seeing `TransformX` call `RebuildX` call 
> > > > > > > > > > `ActOnX`.  Why not do that for `X=OMPCanonicalLoop`?  Does 
> > > > > > > > > > `TransformOMPExecutableDirective` really need a special 
> > > > > > > > > > case for `OMPCanonicalLoop`?
> > > > > > > > > The intended behaviour is: 
> > > > > > > > > 
> > > > > > > > > 1. Transform the child loop
> > > > > > > > > 2. Return the child loop as representing the OMPCanonicalLoop 
> > > > > > > > > (i.e. OMPCanonicalLoop wrapper is removed).
> > > > > > > > > 3. Parent loop-associated directive (e.g. workshare) is 
> > > > > > > > > processed.
> > > > > > > > > 4. `ActOnOpenMPCanonicalLoop` adds new OMPCanonicalLoop 
> > > > > > > > > wrapper for loop nest.
> > > > > > > > > 
> > > > > > > > > This guarantees maximum flexibility on what of the loop can 
> > > > > > > > > be changed, such as
> > > > > > > > > * Change lower bound, upper bound, step
> > > > > > > > > * Convert between CXXForRangeStmt and ForStmt
> > > > > > > > > * Change the associated depth (e.g. different value for 
> > > > > > > > > `collapse` clause)
> > > > > > > > > * Remove the directive and no OMPCanonicalLoop remain
> > > > > > > > > 
> > > > > > > > > This also saves adding many lines of code handling 
> > > > > > > > > transforming each member of OMPCanonicalLoop separately.
> > > > > > > > > The intended behaviour is: 
> > > > > > > > > 
> > > > > > > > > 1. Transform the child loop
> > > > > > > > 
> > > > > > > > For  my suggestion, that call would remain within 
> > > > > > > > `TransformOMPCanonicalLoop` where it is now.
> > > > > > > > 
> > > > > > > > > 2. Return the child loop as representing the OMPCanonicalLoop 
> > > > > > > > > (i.e. OMPCanonicalLoop wrapper is removed).
> > > > > > > > 
> > > > > > > > For my suggestion, this would not happen.  I think it's normal 
> > > > > > > > for a `TransformX` to return the transformed `X` not the 
> > > > > > > > transformed child of `X`.  If a caller wants to transform the 
> > > > > > > > child, then it should transform the child directly instead.
> > > > > > > > 
> > > > > > > > > 3. Parent loop-associated directive (e.g. workshare) is 
> > > > > > > > > processed.
> > > > > > > > 
> > > > > > > > It looks to me like step 3 is currently within 
> > > > > > > > `TransformOMPExecutableDirective` and starts before the call to 
> > > > > > > > `TranformOMPCanonicalLoop` and thus before step 1.  It 
> > > > > > > > completes after step 4.  Or am I misunderstanding what you're 
> > > > > > > > describing as step 3?
> > > > > > > > 
> > > > > > > > > 4. `ActOnOpenMPCanonicalLoop` adds new OMPCanonicalLoop 
> > > > > > > > > wrapper for loop nest.
> > > > > > > > 
> > > > > > > > For my suggestion, this would still happen.  However, instead 
> > > > > > > > of step 2: within `TransformOMPCanonicalLoop`, you would call 
> > > > > > > > `RebuildOMPCanonicalLoop`, which would call 
> > > > > > > > `ActOnOpenMPC

[PATCH] D94973: [clang][OpenMP] Use OpenMPIRBuilder for workshare loops.

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

One property of this patch that has bothered me is that OMPCanonicalLoop is not 
a loop.  Instead, it's an AST node that is sandwiched between a directive and a 
loop to contain extra information about the loop.  The TreeTransform issues 
we've been discussing highlight how that extra node can be confusing.

Now I remember that there's D95496 , which 
seems not to have that property.  I haven't reviewed that patch in any detail, 
and I haven't tried to locate any prior discussions.  Can you please summarize 
why you prefer the current patch over that one?




Comment at: clang/lib/Sema/TreeTransform.h:8321
+TreeTransform::TransformOMPCanonicalLoop(OMPCanonicalLoop *L) {
+  // The OMPCanonicalLoop will be recreated when transforming the 
loop-associted
+  // directive.

Meinersbur wrote:
> jdenny wrote:
> > Meinersbur wrote:
> > > jdenny wrote:
> > > > Meinersbur wrote:
> > > > > jdenny wrote:
> > > > > > Meinersbur wrote:
> > > > > > > jdenny wrote:
> > > > > > > > I'm used to seeing `TransformX` call `RebuildX` call `ActOnX`.  
> > > > > > > > Why not do that for `X=OMPCanonicalLoop`?  Does 
> > > > > > > > `TransformOMPExecutableDirective` really need a special case 
> > > > > > > > for `OMPCanonicalLoop`?
> > > > > > > The intended behaviour is: 
> > > > > > > 
> > > > > > > 1. Transform the child loop
> > > > > > > 2. Return the child loop as representing the OMPCanonicalLoop 
> > > > > > > (i.e. OMPCanonicalLoop wrapper is removed).
> > > > > > > 3. Parent loop-associated directive (e.g. workshare) is processed.
> > > > > > > 4. `ActOnOpenMPCanonicalLoop` adds new OMPCanonicalLoop wrapper 
> > > > > > > for loop nest.
> > > > > > > 
> > > > > > > This guarantees maximum flexibility on what of the loop can be 
> > > > > > > changed, such as
> > > > > > > * Change lower bound, upper bound, step
> > > > > > > * Convert between CXXForRangeStmt and ForStmt
> > > > > > > * Change the associated depth (e.g. different value for 
> > > > > > > `collapse` clause)
> > > > > > > * Remove the directive and no OMPCanonicalLoop remain
> > > > > > > 
> > > > > > > This also saves adding many lines of code handling transforming 
> > > > > > > each member of OMPCanonicalLoop separately.
> > > > > > > The intended behaviour is: 
> > > > > > > 
> > > > > > > 1. Transform the child loop
> > > > > > 
> > > > > > For  my suggestion, that call would remain within 
> > > > > > `TransformOMPCanonicalLoop` where it is now.
> > > > > > 
> > > > > > > 2. Return the child loop as representing the OMPCanonicalLoop 
> > > > > > > (i.e. OMPCanonicalLoop wrapper is removed).
> > > > > > 
> > > > > > For my suggestion, this would not happen.  I think it's normal for 
> > > > > > a `TransformX` to return the transformed `X` not the transformed 
> > > > > > child of `X`.  If a caller wants to transform the child, then it 
> > > > > > should transform the child directly instead.
> > > > > > 
> > > > > > > 3. Parent loop-associated directive (e.g. workshare) is processed.
> > > > > > 
> > > > > > It looks to me like step 3 is currently within 
> > > > > > `TransformOMPExecutableDirective` and starts before the call to 
> > > > > > `TranformOMPCanonicalLoop` and thus before step 1.  It completes 
> > > > > > after step 4.  Or am I misunderstanding what you're describing as 
> > > > > > step 3?
> > > > > > 
> > > > > > > 4. `ActOnOpenMPCanonicalLoop` adds new OMPCanonicalLoop wrapper 
> > > > > > > for loop nest.
> > > > > > 
> > > > > > For my suggestion, this would still happen.  However, instead of 
> > > > > > step 2: within `TransformOMPCanonicalLoop`, you would call 
> > > > > > `RebuildOMPCanonicalLoop`, which would call 
> > > > > > `ActOnOpenMPCanonicalLoop` as step 4.  The effect is you moved 
> > > > > > `ActOnOpenMPCanonicalLoop` from the caller 
> > > > > > (`TransformOMPExecutableDirective`) to the callee's callee, but the 
> > > > > > behavior should remain the same.
> > > > > > 
> > > > > > > This guarantees maximum flexibility on what of the loop can be 
> > > > > > > changed, such as
> > > > > > > * Change lower bound, upper bound, step
> > > > > > > * Convert between CXXForRangeStmt and ForStmt
> > > > > > > * Change the associated depth (e.g. different value for 
> > > > > > > `collapse` clause)
> > > > > > > * Remove the directive and no OMPCanonicalLoop remain
> > > > > > 
> > > > > > Flexibility for whom?
> > > > > > 
> > > > > > A class extending `TreeTransform`?  With my suggestion, it can 
> > > > > > override `TransformOMPCanonicalLoop` or `RebuildOMPCanonicalLoop`, 
> > > > > > depending on how much it wants to alter the transformation.
> > > > > > 
> > > > > > Or a caller of `TransformOMPCanonicalLoop` within `TreeTransform`?  
> > > > > > I only see one right now, `TransformOMPExecutableDirective`, and I 
> > > > > > don't see how it needs the flexibility.  Are there other callers I 
> > > > > > missed?
> > > > > > 
> > > >

[PATCH] D94973: [clang][OpenMP] Use OpenMPIRBuilder for workshare loops.

[Joel E. Denny via Phabricator via cfe-commits]

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

Other than the additional comments I've requested, LGTM.  Thanks for the 
explanations and the changes.




Comment at: clang/lib/Sema/TreeTransform.h:8321
+TreeTransform::TransformOMPCanonicalLoop(OMPCanonicalLoop *L) {
+  // The OMPCanonicalLoop will be recreated when transforming the 
loop-associted
+  // directive.

Meinersbur wrote:
> jdenny wrote:
> > Meinersbur wrote:
> > > jdenny wrote:
> > > > Meinersbur wrote:
> > > > > jdenny wrote:
> > > > > > I'm used to seeing `TransformX` call `RebuildX` call `ActOnX`.  Why 
> > > > > > not do that for `X=OMPCanonicalLoop`?  Does 
> > > > > > `TransformOMPExecutableDirective` really need a special case for 
> > > > > > `OMPCanonicalLoop`?
> > > > > The intended behaviour is: 
> > > > > 
> > > > > 1. Transform the child loop
> > > > > 2. Return the child loop as representing the OMPCanonicalLoop (i.e. 
> > > > > OMPCanonicalLoop wrapper is removed).
> > > > > 3. Parent loop-associated directive (e.g. workshare) is processed.
> > > > > 4. `ActOnOpenMPCanonicalLoop` adds new OMPCanonicalLoop wrapper for 
> > > > > loop nest.
> > > > > 
> > > > > This guarantees maximum flexibility on what of the loop can be 
> > > > > changed, such as
> > > > > * Change lower bound, upper bound, step
> > > > > * Convert between CXXForRangeStmt and ForStmt
> > > > > * Change the associated depth (e.g. different value for `collapse` 
> > > > > clause)
> > > > > * Remove the directive and no OMPCanonicalLoop remain
> > > > > 
> > > > > This also saves adding many lines of code handling transforming each 
> > > > > member of OMPCanonicalLoop separately.
> > > > > The intended behaviour is: 
> > > > > 
> > > > > 1. Transform the child loop
> > > > 
> > > > For  my suggestion, that call would remain within 
> > > > `TransformOMPCanonicalLoop` where it is now.
> > > > 
> > > > > 2. Return the child loop as representing the OMPCanonicalLoop (i.e. 
> > > > > OMPCanonicalLoop wrapper is removed).
> > > > 
> > > > For my suggestion, this would not happen.  I think it's normal for a 
> > > > `TransformX` to return the transformed `X` not the transformed child of 
> > > > `X`.  If a caller wants to transform the child, then it should 
> > > > transform the child directly instead.
> > > > 
> > > > > 3. Parent loop-associated directive (e.g. workshare) is processed.
> > > > 
> > > > It looks to me like step 3 is currently within 
> > > > `TransformOMPExecutableDirective` and starts before the call to 
> > > > `TranformOMPCanonicalLoop` and thus before step 1.  It completes after 
> > > > step 4.  Or am I misunderstanding what you're describing as step 3?
> > > > 
> > > > > 4. `ActOnOpenMPCanonicalLoop` adds new OMPCanonicalLoop wrapper for 
> > > > > loop nest.
> > > > 
> > > > For my suggestion, this would still happen.  However, instead of step 
> > > > 2: within `TransformOMPCanonicalLoop`, you would call 
> > > > `RebuildOMPCanonicalLoop`, which would call `ActOnOpenMPCanonicalLoop` 
> > > > as step 4.  The effect is you moved `ActOnOpenMPCanonicalLoop` from the 
> > > > caller (`TransformOMPExecutableDirective`) to the callee's callee, but 
> > > > the behavior should remain the same.
> > > > 
> > > > > This guarantees maximum flexibility on what of the loop can be 
> > > > > changed, such as
> > > > > * Change lower bound, upper bound, step
> > > > > * Convert between CXXForRangeStmt and ForStmt
> > > > > * Change the associated depth (e.g. different value for `collapse` 
> > > > > clause)
> > > > > * Remove the directive and no OMPCanonicalLoop remain
> > > > 
> > > > Flexibility for whom?
> > > > 
> > > > A class extending `TreeTransform`?  With my suggestion, it can override 
> > > > `TransformOMPCanonicalLoop` or `RebuildOMPCanonicalLoop`, depending on 
> > > > how much it wants to alter the transformation.
> > > > 
> > > > Or a caller of `TransformOMPCanonicalLoop` within `TreeTransform`?  I 
> > > > only see one right now, `TransformOMPExecutableDirective`, and I don't 
> > > > see how it needs the flexibility.  Are there other callers I missed?
> > > > 
> > > > Are you trying to create flexibility without requiring deriving from 
> > > > `TreeTransform`?  But, as far as I can tell, you're doing so at the 
> > > > expense of normal `TreeTransform` semantics.  Doesn't seem worth it.
> > > > 
> > > > If you see a precedent for your approach elsewhere in `TreeTransform`, 
> > > > please point it out.
> > > > 
> > > > > This also saves adding many lines of code handling transforming each 
> > > > > member of OMPCanonicalLoop separately.
> > > > 
> > > > Why would you need to?  In the other `TransformX` functions I looked 
> > > > at, the arguments to the `RebuildX` function are transformed, and those 
> > > > are typically just the arguments to the `ActOnX` function.  In other 
> > > > words, you would just transform the loop within your `OMPCanonica

[PATCH] D94973: [clang][OpenMP] Use OpenMPIRBuilder for workshare loops.

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/lib/Sema/TreeTransform.h:8326-8331
+template 
+StmtResult
+TreeTransform::TransformOMPCanonicalLoop(OMPCanonicalLoop *L) {
+  llvm_unreachable("OMPCanonicalLoop must be handled by the "
+   "OMPExecutableDirective that it is associated with");
+}

Because this receives an OMPCanonicalLoop, I'm assuming the OpenMP IR builder 
is enabled.  Without my suggested edits, the caller checks that.  After my 
suggested edits, maybe it's worthwhile to have an assertion here that it's 
enabled.  I don't know.



Comment at: clang/lib/Sema/TreeTransform.h:8368-8374
+  if (isOpenMPLoopDirective(D->getDirectiveKind()) &&
+  getSema().getLangOpts().OpenMPIRBuilder)
+CS = cast(CS)->getLoopStmt();
   Body = getDerived().TransformStmt(CS);
+  if (Body.isUsable() && isOpenMPLoopDirective(D->getDirectiveKind()) &&
+  getSema().getLangOpts().OpenMPIRBuilder)
+Body = getDerived().RebuildOMPCanonicalLoop(Body.get());





Comment at: clang/lib/Sema/TreeTransform.h:8321
+TreeTransform::TransformOMPCanonicalLoop(OMPCanonicalLoop *L) {
+  // The OMPCanonicalLoop will be recreated when transforming the 
loop-associted
+  // directive.

Meinersbur wrote:
> jdenny wrote:
> > Meinersbur wrote:
> > > jdenny wrote:
> > > > I'm used to seeing `TransformX` call `RebuildX` call `ActOnX`.  Why not 
> > > > do that for `X=OMPCanonicalLoop`?  Does 
> > > > `TransformOMPExecutableDirective` really need a special case for 
> > > > `OMPCanonicalLoop`?
> > > The intended behaviour is: 
> > > 
> > > 1. Transform the child loop
> > > 2. Return the child loop as representing the OMPCanonicalLoop (i.e. 
> > > OMPCanonicalLoop wrapper is removed).
> > > 3. Parent loop-associated directive (e.g. workshare) is processed.
> > > 4. `ActOnOpenMPCanonicalLoop` adds new OMPCanonicalLoop wrapper for loop 
> > > nest.
> > > 
> > > This guarantees maximum flexibility on what of the loop can be changed, 
> > > such as
> > > * Change lower bound, upper bound, step
> > > * Convert between CXXForRangeStmt and ForStmt
> > > * Change the associated depth (e.g. different value for `collapse` clause)
> > > * Remove the directive and no OMPCanonicalLoop remain
> > > 
> > > This also saves adding many lines of code handling transforming each 
> > > member of OMPCanonicalLoop separately.
> > > The intended behaviour is: 
> > > 
> > > 1. Transform the child loop
> > 
> > For  my suggestion, that call would remain within 
> > `TransformOMPCanonicalLoop` where it is now.
> > 
> > > 2. Return the child loop as representing the OMPCanonicalLoop (i.e. 
> > > OMPCanonicalLoop wrapper is removed).
> > 
> > For my suggestion, this would not happen.  I think it's normal for a 
> > `TransformX` to return the transformed `X` not the transformed child of 
> > `X`.  If a caller wants to transform the child, then it should transform 
> > the child directly instead.
> > 
> > > 3. Parent loop-associated directive (e.g. workshare) is processed.
> > 
> > It looks to me like step 3 is currently within 
> > `TransformOMPExecutableDirective` and starts before the call to 
> > `TranformOMPCanonicalLoop` and thus before step 1.  It completes after step 
> > 4.  Or am I misunderstanding what you're describing as step 3?
> > 
> > > 4. `ActOnOpenMPCanonicalLoop` adds new OMPCanonicalLoop wrapper for loop 
> > > nest.
> > 
> > For my suggestion, this would still happen.  However, instead of step 2: 
> > within `TransformOMPCanonicalLoop`, you would call 
> > `RebuildOMPCanonicalLoop`, which would call `ActOnOpenMPCanonicalLoop` as 
> > step 4.  The effect is you moved `ActOnOpenMPCanonicalLoop` from the caller 
> > (`TransformOMPExecutableDirective`) to the callee's callee, but the 
> > behavior should remain the same.
> > 
> > > This guarantees maximum flexibility on what of the loop can be changed, 
> > > such as
> > > * Change lower bound, upper bound, step
> > > * Convert between CXXForRangeStmt and ForStmt
> > > * Change the associated depth (e.g. different value for `collapse` clause)
> > > * Remove the directive and no OMPCanonicalLoop remain
> > 
> > Flexibility for whom?
> > 
> > A class extending `TreeTransform`?  With my suggestion, it can override 
> > `TransformOMPCanonicalLoop` or `RebuildOMPCanonicalLoop`, depending on how 
> > much it wants to alter the transformation.
> > 
> > Or a caller of `TransformOMPCanonicalLoop` within `TreeTransform`?  I only 
> > see one right now, `TransformOMPExecutableDirective`, and I don't see how 
> > it needs the flexibility.  Are there other callers I missed?
> > 
> > Are you trying to create flexibility without requiring deriving from 
> > `TreeTransform`?  But, as far as I can tell, you're doing so at the expense 
> > of normal `TreeTransform` semantics.  Doesn't seem worth it.
> > 
> > If you see a precedent for your approach elsewhere in `TreeT

[PATCH] D94973: [clang][OpenMP] Use OpenMPIRBuilder for workshare loops.

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

This patch has no effect if the OpenMP IR builder is not enabled, and it's 
disabled by default.  Is that right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94973

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

[PATCH] D94973: [clang][OpenMP] Use OpenMPIRBuilder for workshare loops.

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:5355
+  Expr *Cond, *Inc;
+  VarDecl *CounterDecl, *LVDecl;
+  if (auto *For = dyn_cast(AStmt)) {

jdenny wrote:
> `CounterDecl` is the declaration of the "loop iteration variable" based on 
> the comments now on `OMPCanonicalLoop`, right?  If so, can we update the 
> variable names here?  One possibility is `LIVDecl` and `LUVDecl`.
This is marked done, but I don't see a change or reply for it.



Comment at: clang/lib/Sema/TreeTransform.h:8321
+TreeTransform::TransformOMPCanonicalLoop(OMPCanonicalLoop *L) {
+  // The OMPCanonicalLoop will be recreated when transforming the 
loop-associted
+  // directive.

Meinersbur wrote:
> jdenny wrote:
> > I'm used to seeing `TransformX` call `RebuildX` call `ActOnX`.  Why not do 
> > that for `X=OMPCanonicalLoop`?  Does `TransformOMPExecutableDirective` 
> > really need a special case for `OMPCanonicalLoop`?
> The intended behaviour is: 
> 
> 1. Transform the child loop
> 2. Return the child loop as representing the OMPCanonicalLoop (i.e. 
> OMPCanonicalLoop wrapper is removed).
> 3. Parent loop-associated directive (e.g. workshare) is processed.
> 4. `ActOnOpenMPCanonicalLoop` adds new OMPCanonicalLoop wrapper for loop nest.
> 
> This guarantees maximum flexibility on what of the loop can be changed, such 
> as
> * Change lower bound, upper bound, step
> * Convert between CXXForRangeStmt and ForStmt
> * Change the associated depth (e.g. different value for `collapse` clause)
> * Remove the directive and no OMPCanonicalLoop remain
> 
> This also saves adding many lines of code handling transforming each member 
> of OMPCanonicalLoop separately.
> The intended behaviour is: 
> 
> 1. Transform the child loop

For  my suggestion, that call would remain within `TransformOMPCanonicalLoop` 
where it is now.

> 2. Return the child loop as representing the OMPCanonicalLoop (i.e. 
> OMPCanonicalLoop wrapper is removed).

For my suggestion, this would not happen.  I think it's normal for a 
`TransformX` to return the transformed `X` not the transformed child of `X`.  
If a caller wants to transform the child, then it should transform the child 
directly instead.

> 3. Parent loop-associated directive (e.g. workshare) is processed.

It looks to me like step 3 is currently within 
`TransformOMPExecutableDirective` and starts before the call to 
`TranformOMPCanonicalLoop` and thus before step 1.  It completes after step 4.  
Or am I misunderstanding what you're describing as step 3?

> 4. `ActOnOpenMPCanonicalLoop` adds new OMPCanonicalLoop wrapper for loop nest.

For my suggestion, this would still happen.  However, instead of step 2: within 
`TransformOMPCanonicalLoop`, you would call `RebuildOMPCanonicalLoop`, which 
would call `ActOnOpenMPCanonicalLoop` as step 4.  The effect is you moved 
`ActOnOpenMPCanonicalLoop` from the caller (`TransformOMPExecutableDirective`) 
to the callee's callee, but the behavior should remain the same.

> This guarantees maximum flexibility on what of the loop can be changed, such 
> as
> * Change lower bound, upper bound, step
> * Convert between CXXForRangeStmt and ForStmt
> * Change the associated depth (e.g. different value for `collapse` clause)
> * Remove the directive and no OMPCanonicalLoop remain

Flexibility for whom?

A class extending `TreeTransform`?  With my suggestion, it can override 
`TransformOMPCanonicalLoop` or `RebuildOMPCanonicalLoop`, depending on how much 
it wants to alter the transformation.

Or a caller of `TransformOMPCanonicalLoop` within `TreeTransform`?  I only see 
one right now, `TransformOMPExecutableDirective`, and I don't see how it needs 
the flexibility.  Are there other callers I missed?

Are you trying to create flexibility without requiring deriving from 
`TreeTransform`?  But, as far as I can tell, you're doing so at the expense of 
normal `TreeTransform` semantics.  Doesn't seem worth it.

If you see a precedent for your approach elsewhere in `TreeTransform`, please 
point it out.

> This also saves adding many lines of code handling transforming each member 
> of OMPCanonicalLoop separately.

Why would you need to?  In the other `TransformX` functions I looked at, the 
arguments to the `RebuildX` function are transformed, and those are typically 
just the arguments to the `ActOnX` function.  In other words, you would just 
transform the loop within your `OMPCanonicalLoop` as you're doing now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94973

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

[PATCH] D94973: [clang][OpenMP] Use OpenMPIRBuilder for workshare loops.

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/lib/CodeGen/CodeGenFunction.h:292
+  /// nest would extend.
+  SmallVector OMPLoopNestStack;
+

Unless I missed something, the only accesses to `OMPLoopNestStack` are 
`push_back`, `clear`, `back`, and `size`.  It's never popped.  That is, every 
push appears to mean that all existing elements will never be accessed again.  
So why is it a stack?  Is that purely for the sake of the assertion that calls 
`size`?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:5355
+  Expr *Cond, *Inc;
+  VarDecl *CounterDecl, *LVDecl;
+  if (auto *For = dyn_cast(AStmt)) {

`CounterDecl` is the declaration of the "loop iteration variable" based on the 
comments now on `OMPCanonicalLoop`, right?  If so, can we update the variable 
names here?  One possibility is `LIVDecl` and `LUVDecl`.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:5139
+/// Used to capture variable references if already parsed 
statements/expressions
+/// into a CapturedStatement.
+class CaptureVars : public TreeTransform {

Meinersbur wrote:
> jdenny wrote:
> > I think this means:
> > 
> > > If already parsed statements/expressions, used to capture variable 
> > > references into a CapturedStatement.
> > 
> > But it reads like it means:
> > 
> > > If already parsed statements/expressions into a CapturedStatement, used 
> > > to capture variable references.
> if -> of
> 
> I reformulated the sentence which I hope is now more clear.
That's better.  Thanks.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:5215
+  Expr *NegIncAmount =
+  AssertSuccess(Actions.BuildUnaryOp(nullptr, {}, UO_Minus, NewStep));
+  Expr *BackwardDist = AssertSuccess(

Meinersbur wrote:
> Meinersbur wrote:
> > jdenny wrote:
> > > It looks like the AST nodes `NewStart`, `NewStop`, and `NewStep` are 
> > > shared by multiple parent nodes here.  Does that ever happen anywhere 
> > > else in a Clang AST?
> > Yes. For instance, when instantiating a template, nodes form the 
> > TemplatedDecl are reused unless they need to change. Therefore multiple 
> > template instantiation can also share entire AST subtrees. Why rebuild them 
> > if they are identical?
> > 
> > However, I also found the following description of 
> > TreeTransform::AlwaysRebuild():
> > ```
> >   /// We must always rebuild all AST nodes when performing variadic template
> >   /// pack expansion, in order to avoid violating the AST invariant that 
> > each
> >   /// statement node appears at most once in its containing declaration.
> > ```
> > It was added by @rsmith in rG2aa81a718e64fb4ca651ea12ab7afaeffc11e463 to 
> > fix llvm.org/PR17800 . The assertion doesn't seem to exist in the current 
> > code base.
> > 
> > Does this apply here? 
> I think the invariant exists because codegen has a map from ast expression 
> nodes to its temporary memory location. This means that reusing the same AST 
> expression node indeed should not be done.
> 
> I think the assertion that failed in llvm.org/PR17800 is now here:
> https://github.com/llvm/llvm-project/blob/7ba2e1c6011eeb1b91ce3f5d8fa7187b7518e77a/clang/lib/AST/ExprConstant.cpp#L1872
> 
> Assertion message was changed in this commit:
> https://github.com/llvm/llvm-project/commit/f7f2e4261a98b2da519d58e7f6794b013cda7a4b
Thanks for the links.

I guess reuse is ok across different instantiations of a template because this 
map is cleared across different functions.

It might not be worth much at this point, but I also saw this comment as more 
evidence for this invariant: [[ 
https://clang.llvm.org/doxygen/classclang_1_1ASTContext.html#ad6ff7b541a77cea65d8f72ed3f903fb3
 | "expression's are never shared"]].



Comment at: clang/lib/Sema/TreeTransform.h:8321
+TreeTransform::TransformOMPCanonicalLoop(OMPCanonicalLoop *L) {
+  // The OMPCanonicalLoop will be recreated when transforming the 
loop-associted
+  // directive.

I'm used to seeing `TransformX` call `RebuildX` call `ActOnX`.  Why not do that 
for `X=OMPCanonicalLoop`?  Does `TransformOMPExecutableDirective` really need a 
special case for `OMPCanonicalLoop`?



Comment at: clang/tools/libclang/CIndex.cpp:2839
+  VisitStmt(L);
+  EnqueueChildren(L);
+}

Doesn't `VisitStmt(L)` already call `EnqueueChildren(L)`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94973

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

[PATCH] D94973: [clang][OpenMP] Use OpenMPIRBuilder for workshare loops.

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/include/clang/AST/StmtOpenMP.h:166
+assert((isa(S) || isa(S)) &&
+   "Canonical loop must be a for loop (range-based or otherwise)");
+SubStmts[LOOPY_STMT] = S;

Meinersbur wrote:
> jdenny wrote:
> > To convert run-time errors into compile-time errors, what if `setLoopStmt` 
> > is overloaded to take either a `ForStmt` or `CXXForRangeStmt` instead of 
> > any `Stmt`?
> This would also require callers to call two different versions and just 
> removes the problem up the call stack. For instance, StmtReader just receives 
> a Stmt from ReadStmt. If we had setLoopStmt, we'd need a switch to call one 
> of overloads.
OK, I checked the call stack for your setters this time.  It looks like it's 
just `create`, and I see that trying to achieve my goal would make a mess of 
that call stack.  Sorry for the noise.



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:1872
+  LValue CapStruct = ParentCGF.InitCapturedStruct(*S);
+  CodeGenFunction CGF(ParentCGF.CGM, true);
+  std::unique_ptr CSI =





Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3497
+
+  bool NeedsBarrer = !S.getSingleClause();
+  llvm::OpenMPIRBuilder &OMPBuilder =





Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:94
   // been "emitted" to the outside, thus, modifications are still sensible.
-  if (CGM.getLangOpts().OpenMPIRBuilder)
-CGM.getOpenMPRuntime().getOMPBuilder().finalize();
+  if (CGM.getLangOpts().OpenMPIRBuilder && CurFn)
+CGM.getOpenMPRuntime().getOMPBuilder().finalize(CurFn);

Without this patch, `finalize` is called even if `!CurFn`?



Comment at: clang/lib/CodeGen/CodeGenFunction.h:288
+  /// this stack when done. Entering a new loop requires clearing this list; it
+  /// either means we start parsing an new loop nest (in which case the 
previous
+  /// loop nest goes out of scope) or a second loop in the same level in which





Comment at: clang/lib/Sema/SemaOpenMP.cpp:3771
+
+  void VisitOMPCanonicalLoop(OMPCanonicalLoop *S) { VisitStmt(S); }
+

Isn't `VisitStmt` called for any `Stmt` without this?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:5139
+/// Used to capture variable references if already parsed 
statements/expressions
+/// into a CapturedStatement.
+class CaptureVars : public TreeTransform {

I think this means:

> If already parsed statements/expressions, used to capture variable references 
> into a CapturedStatement.

But it reads like it means:

> If already parsed statements/expressions into a CapturedStatement, used to 
> capture variable references.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:5215
+  Expr *NegIncAmount =
+  AssertSuccess(Actions.BuildUnaryOp(nullptr, {}, UO_Minus, NewStep));
+  Expr *BackwardDist = AssertSuccess(

It looks like the AST nodes `NewStart`, `NewStop`, and `NewStep` are shared by 
multiple parent nodes here.  Does that ever happen anywhere else in a Clang AST?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94973

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

[PATCH] D94973: [clang][OpenMP] Use OpenMPIRBuilder for workshare loops.

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D94973#2547383 , @jdoerfert wrote:

> I have a single last comment/request. @jdenny I'll take it you finish the 
> review and accept as you see fit.

@jdoerfert, if you've already reviewed everything and want to accept so this 
can move forward, I'm fine with that.  Otherwise, I'll review more as I find 
time.  There's a lot of code.




Comment at: clang/include/clang/AST/StmtOpenMP.h:45
+/// known before entering the loop and allow skipping to an arbitrary 
iteration.
+/// The OMPCanonicalLoop AST node wraps a ForStmt or CXXRangeForStmt that is
+/// known to fulfill OpenMP's canonical loop requirements.





Comment at: clang/include/clang/AST/StmtOpenMP.h:54
+///   for range-based for-statement, this is the hidden iterator '__begin'. For
+///   other loops, it is identical to the loop variable. Must be a 
random-access
+///   iterator, pointer or integer type.





Comment at: clang/include/clang/AST/StmtOpenMP.h:58
+///   incrementing by one at each iteration. Allows abstracting over the type
+///   of the loop counter and is always an unsigned integer type appropriate to
+///   represent the range of the loop counter variable. Its value corresponds 
to





Comment at: clang/include/clang/AST/StmtOpenMP.h:59
+///   of the loop counter and is always an unsigned integer type appropriate to
+///   represent the range of the loop counter variable. Its value corresponds 
to
+///   the logical iteration number in the OpenMP specification.





Comment at: clang/include/clang/AST/StmtOpenMP.h:97
+/// `std::vector::iterator::difference_type` aka `ptrdiff_t`).
+/// Therefore, the distance function will be 
+///   [&](size_t &Result) { Result = __end - __begin; }





Comment at: clang/include/clang/AST/StmtOpenMP.h:166
+assert((isa(S) || isa(S)) &&
+   "Canonical loop must be a for loop (range-based or otherwise)");
+SubStmts[LOOPY_STMT] = S;

To convert run-time errors into compile-time errors, what if `setLoopStmt` is 
overloaded to take either a `ForStmt` or `CXXForRangeStmt` instead of any 
`Stmt`?



Comment at: clang/include/clang/AST/StmtOpenMP.h:190
+
+  /// The function that compute the loop  user variable from a logical 
iteration
+  /// counter. Can be evaluated as first statement in the loop.





Comment at: clang/include/clang/AST/StmtOpenMP.h:196
+  /// value, step size) are captured by the closure. In particular, the initial
+  /// value of loop counter is captured by value to be unaffected by previous
+  /// iterations.





Comment at: clang/include/clang/AST/StmtOpenMP.h:92
+/// 
+///   [&,__begin](std::vector::iterator &Result, size_t Logical) {
+///   Result = __begin + Logical; }

Meinersbur wrote:
> jdenny wrote:
> > Why is `__begin` an explicit capture here but not for the distance function?
> Because `__begin` is captured by-value, everything lese uses the `&`default 
> capture. This is the loop counter variable is modified inside the loop body.
> Because `__begin` is captured by-value, everything lese uses the `&`default 
> capture. This is the loop counter variable is modified inside the loop body.





Comment at: clang/include/clang/AST/StmtOpenMP.h:92
+/// 
+///   [&,__begin](std::vector::iterator &Result, size_t Logical) {
+///   Result = __begin + Logical; }

jdenny wrote:
> Meinersbur wrote:
> > jdenny wrote:
> > > Why is `__begin` an explicit capture here but not for the distance 
> > > function?
> > Because `__begin` is captured by-value, everything lese uses the `&`default 
> > capture. This is the loop counter variable is modified inside the loop body.
> > Because `__begin` is captured by-value, everything lese uses the `&`default 
> > capture. This is the loop counter variable is modified inside the loop body.
> 
> 
Ah, I now see there are comments about this in the code.  I think a brief note 
here too would help because it stands out in the example, but your call.



Comment at: clang/include/clang/AST/StmtOpenMP.h:101
+  enum {
+LOOPY_STMT,
+DISTANCE_FUNC,

Meinersbur wrote:
> jdenny wrote:
> > Why "loopy" with a "y"?
> `loopy` was meant to refer to the property of the statement (ForStmt, 
> CXXForRangeStmt, potentially others such as WhileStmt, DoStmt, functions from 
> `#include `, etc) instead of a specific loop node (such as 
> OMPLoopDirective or OMPCanonicalLoop itself), although I did not apply this 
> idea consistently. Do you prefer a plain 'LOOP_STMT'?
> 
Yes.  In my mind, "loopy" doesn't help with that distinction.  But your call.



Comment at: clang/include/clang/Sema/Sema.h:10486
+
+  /// Called for synt

[PATCH] D94973: [clang][OpenMP] Use OpenMPIRBuilder for workshare loops.

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

@Meinersbur Sorry, I haven't gotten very far yet in reviewing this, but I'll 
try to work on this more soon.




Comment at: clang/include/clang/AST/StmtOpenMP.h:31
 
+/// Representation of an OpenMP canonical loop.
+///

I think it would be helpful to cite the OpenMP spec here.  Just version and 
section number.



Comment at: clang/include/clang/AST/StmtOpenMP.h:37
+/// The OMPCanonicalLoop AST node wraps a ForStmt or CXXRangeForStmt that is
+/// known for fulfill OpenMP's loop requirements.
+///





Comment at: clang/include/clang/AST/StmtOpenMP.h:41
+/// purposes:
+/// * Loop variable: The user-accessible variable with different value for each
+///   iteration.

I suggest the terms "loop user variable" and "loop iteration variable".  In 
particular, I find "loop counter" to be misleading because "counter" suggests 
to me it's always an integer, so it's easy to get it confused with the "logical 
iteration counter".  If you're using standard terminology that I'm not aware 
of, then never mind.



Comment at: clang/include/clang/AST/StmtOpenMP.h:43
+///   iteration.
+/// * Loop counter: The variable used to identify a loop iterations; for
+///   range-based for-statement, this is the hidden iterator '__begin'. For





Comment at: clang/include/clang/AST/StmtOpenMP.h:46
+///   other loops, it is identical to the loop variable. Must be a 
random-access
+///   iterator or integer type.
+/// * Logical iteration counter: Normalized loop counter starting at 0 and

Please list pointer too as I don't think C has an iterator concept.



Comment at: clang/include/clang/AST/StmtOpenMP.h:48
+/// * Logical iteration counter: Normalized loop counter starting at 0 and
+///   incrementing by one at each iterations. Allows abstracting over the type
+///   of the loop counter and is always an unsigned integer type appropriate to





Comment at: clang/include/clang/AST/StmtOpenMP.h:61
+/// random-access iterator. The OpenMPIRBuilder will use this information to
+/// convert the loop into simd-, workshare-, distribute-, taskloop etc. For
+/// compatibility with the non-OpenMPIRBuilder codegen path, an 
OMPCanonicalLoop

Are those hyphens intentional?



Comment at: clang/include/clang/AST/StmtOpenMP.h:92
+/// 
+///   [&,__begin](std::vector::iterator &Result, size_t Logical) {
+///   Result = __begin + Logical; }

Why is `__begin` an explicit capture here but not for the distance function?



Comment at: clang/include/clang/AST/StmtOpenMP.h:94
+///   Result = __begin + Logical; }
+/// 
+class OMPCanonicalLoop : public Stmt {

Thanks for this careful documentation!



Comment at: clang/include/clang/AST/StmtOpenMP.h:101
+  enum {
+LOOPY_STMT,
+DISTANCE_FUNC,

Why "loopy" with a "y"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94973

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

[PATCH] D94261: [clang] Add powerpc64le-none-linux-gnu to gnu toolchain for PPC64

[Joel E. Denny via Phabricator via cfe-commits]

jdenny accepted this revision.
jdenny 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/D94261/new/

https://reviews.llvm.org/D94261

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

[PATCH] D86119: [OPENMP50]Allow overlapping mapping in target constrcuts.

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D86119#2330339 , @ABataev wrote:

> In D86119#2330310 , @jdenny wrote:
>
>> Thanks for working on this.  Sorry to take so long to review.  Before I try 
>> to digest the code, I have a few high-level questions.
>>
>> Based on the test suite changes, `TARGET_PARAM` is disappearing from many 
>> cases.  Can you explain a bit how that supports overlapping and reordering?
>
> `TARGET_PARAM` is used only for marking the data that should be passed to the 
> kernel function as an argument. We just generate it in many cases but the 
> runtime actually does not use them. Thу patch relies on this thing, 
> otherwise, we may pass an incorrect number of arguments to the kernel 
> functions.

Is it reasonable to extract that change into a parent patch?  That would at 
least make the test suite changes easier to follow.

>> Have you considered issue 2337 for the OpenMP spec and how your 
>> implementation handles the ambiguous cases cited there?
>
> Can you provide the details about this issue?

It discusses ambiguities in the way the spec describes map clause ordering.  
Here's one example relevant to `present`:

  #pragma omp target exit data map(from: x) map(present, release: x)

One statement in the spec says a map clause with `from` is effectively ordered 
before one with `release`.  Another statement says a map clause with `present` 
is effectively ordered before one without.  Which applies in the above example? 
 In some discussions, people agreed the statement about `present` was intended 
to have precedence, but the spec doesn't say that.  That resolution probably 
makes sense at entry to a region, but does it make sense at exit?  Would it 
suppress `from` in this example?  (Actually, should there be two reference 
counter decrements in this example or just one?)

These ambiguities are the reason I punted on this issue when implementing 
`present`.  If we can come up with a reasonable implementation for all cases, 
perhaps we can propose a new wording for the spec.


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

[PATCH] D86119: [OPENMP50]Allow overlapping mapping in target constrcuts.

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

Thanks for working on this.  Sorry to take so long to review.  Before I try to 
digest the code, I have a few high-level questions.

Based on the test suite changes, `TARGET_PARAM` is disappearing from many 
cases.  Can you explain a bit how that supports overlapping and reordering?

Have you considered issue 2337 for the OpenMP spec and how your implementation 
handles the ambiguous cases cited there?




Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7283
+llvm::find(MotionModifiers, OMPC_MOTION_MODIFIER_present) !=
+MotionModifiers.end())
   Bits |= OMP_MAP_PRESENT;

Unrelated change?


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

[PATCH] D85692: python bindings: fix DeprecationWarning

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D85692#2228860 , @nickdesaulniers 
wrote:

> In D85692#2227531 , @jdenny wrote:
>
>> Does this mean we officially no longer support python 2, which this change 
>> breaks?
>
> Send a patch to support both and I'll approve it.

:-)  I have no desire to support both.  I'm asking whether I have to worry 
about python 2 when I do submit patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85692

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

[PATCH] D85692: python bindings: fix DeprecationWarning

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

Does this mean we officially no longer support python 2, which this change 
breaks?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85692

___
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`

[Joel E. Denny via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG002d61db2b77: [OpenMP] Fix `present` for exit from `omp 
target data` (authored by jdenny).

Changed prior to commit:
  https://reviews.llvm.org/D84422?vs=281330&id=283224#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84422

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.h
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/test/OpenMP/target_data_codegen.cpp
  openmp/libomptarget/src/omptarget.cpp
  openmp/libomptarget/test/mapping/present/target_data_at_exit.c

Index: openmp/libomptarget/test/mapping/present/target_data_at_exit.c
===
--- /dev/null
+++ openmp/libomptarget/test/mapping/present/target_data_at_exit.c
@@ -0,0 +1,37 @@
+// RUN: %libomptarget-compile-aarch64-unknown-linux-gnu -fopenmp-version=51
+// RUN: %libomptarget-run-aarch64-unknown-linux-gnu 2>&1 \
+// RUN: | %fcheck-aarch64-unknown-linux-gnu
+
+// RUN: %libomptarget-compile-powerpc64-ibm-linux-gnu -fopenmp-version=51
+// RUN: %libomptarget-run-powerpc64-ibm-linux-gnu 2>&1 \
+// RUN: | %fcheck-powerpc64-ibm-linux-gnu
+
+// RUN: %libomptarget-compile-powerpc64le-ibm-linux-gnu -fopenmp-version=51
+// RUN: %libomptarget-run-powerpc64le-ibm-linux-gnu 2>&1 \
+// RUN: | %fcheck-powerpc64le-ibm-linux-gnu
+
+// RUN: %libomptarget-compile-x86_64-pc-linux-gnu -fopenmp-version=51
+// RUN: %libomptarget-run-x86_64-pc-linux-gnu 2>&1 \
+// RUN: | %fcheck-x86_64-pc-linux-gnu
+
+#include 
+
+int main() {
+  int i;
+
+#pragma omp target enter data map(alloc:i)
+
+  // i isn't present at the end of the target data region, but the "present"
+  // modifier is only checked at the beginning of a region.
+#pragma omp target data map(present, alloc: i)
+  {
+#pragma omp target exit data map(delete:i)
+  }
+
+  // CHECK-NOT: Libomptarget
+  // CHECK: success
+  // CHECK-NOT: Libomptarget
+  fprintf(stderr, "success\n");
+
+  return 0;
+}
Index: openmp/libomptarget/src/omptarget.cpp
===
--- openmp/libomptarget/src/omptarget.cpp
+++ openmp/libomptarget/src/omptarget.cpp
@@ -506,8 +506,14 @@
   DP("Mapping does not exist (%s)\n",
  (HasPresentModifier ? "'present' map type modifier" : "ignored"));
   if (HasPresentModifier) {
-// FIXME: This should not be an error on exit from "omp target data",
-// but it should be an error upon entering an "omp target exit data".
+// This should be an error upon entering an "omp target exit data".  It
+// should not be an error upon exiting an "omp target data" or "omp
+// target".  For "omp target data", Clang thus doesn't include present
+// modifiers for end calls.  For "omp target", we have not found a valid
+// OpenMP program for which the error matters: it appears that, if a
+// program can guarantee that data is present at the beginning of an
+// "omp target" region so that there's no error there, that data is also
+// guaranteed to be present at the end.
 MESSAGE("device mapping required by 'present' map type modifier does "
 "not exist for host address " DPxMOD " (%ld bytes)",
 DPxPTR(HstPtrBegin), DataSize);
Index: clang/test/OpenMP/target_data_codegen.cpp
===
--- clang/test/OpenMP/target_data_codegen.cpp
+++ clang/test/OpenMP/target_data_codegen.cpp
@@ -256,10 +256,16 @@
 double gc[100];
 
 // PRESENT=0x1000 | TARGET_PARAM=0x20 | TO=0x1 = 0x1021
-// CK1A: [[MTYPE00:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x1021]]]
+// CK1A: [[MTYPE00Begin:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x1021]]]
+
+// TARGET_PARAM=0x20 | TO=0x1 = 0x21
+// CK1A: [[MTYPE00End:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x21]]]
 
 // PRESENT=0x1000 | CLOSE=0x400 | TARGET_PARAM=0x20 | ALWAYS=0x4 | TO=0x1 = 0x1425
-// CK1A: [[MTYPE01:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x1425]]]
+// CK1A: [[MTYPE01Begin:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x1425]]]
+
+// CLOSE=0x400 | TARGET_PARAM=0x20 | ALWAYS=0x4 | TO=0x1 = 0x425
+// CK1A: [[MTYPE01End:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x425]]]
 
 // CK1A-LABEL: _Z3fooi
 void foo(int arg) {
@@ -267,7 +273,7 @@
   float lb[arg];
 
   // Region 00
-  // CK1A-DAG: call void @__tgt_target_data_begin_mapper(i64 -1, i32 1, i8** [[GEPBP:%.+]], i8** [[GEPP:%.+]], i[[sz:32|64]]* [[GEPS:%.+]], {{.+}}getelementptr {{.+}}[1 x i{{.+}}]* [[MTYPE00]]{{.+}})
+  // CK1A-DAG: call void @__tgt_target_data_begin_mapper(i64 -1, i32 1, i8** [[GEPBP:%.+]], i8** [[GEPP:%.+]], i[[sz:32|64]]* [[GEPS:%.+]], {{.+}}getelementptr {{.+}}[1 x i{{.+}}]* [[MTYPE00Begin]]{{.+}})
   // CK1A-DAG: [[GEPBP]] = getelementptr inbounds {{.+}}[[BP:%[^,]+]]
   // CK1A-DAG: [[GEPP]] = getelementptr inbounds {{.+}

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

[Joel E. Denny via Phabricator via cfe-commits]

jdenny marked an inline comment as done.
jdenny added a comment.

Thanks for the review.

As discussed during the 7/29 call, I'll wait to push until we're sure about 
what the OpenMP committee intended here.  I'm pursuing this and will report 
back when I have more information.


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] D84711: [OpenMP] Implement TR8 `present` motion modifier in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:8055
   for (const auto L : C->component_lists()) {
-InfoGen(std::get<0>(L), std::get<1>(L), OMPC_MAP_to, llvm::None,
+SmallVector MapModifiers;
+translateMotionModifiers(C->getMotionModifiers(), MapModifiers);

jdenny wrote:
> I haven't managed to locally reproduce the bot failures I saw today when 
> trying to push this patch.  Somehow they're dropping the present modifiers 
> during codegen.  I think it's due to memory corruption that my machines 
> aren't managing to experience.  The culprit seems to be that `MapModifiers` 
> is local here, but the `InfoGen` call below stores it in a `MapInfo` as an 
> `ArrayRef`, which becames a dangling ref by the time it's used.
> 
> One solution is to change `MapInfo` to store a `SmallVector` instead.  
> Another would be for `MapInfo` to store a second `ArrayRef` for 
> `C->getMotionModifiers()`, which would be translated to runtime map flags 
> later.  I'm planning to try the latter solution tomorrow.  I prefer it 
> because it seems more space efficient and because it translates motion 
> modifiers directly to runtime flags instead of translating to map type 
> modifiers first.
9f2f3b9de6314a009322b6081c792ebf9a469460 relands this patch with the latter 
solution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84711

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

[PATCH] D84711: [OpenMP] Implement TR8 `present` motion modifier in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:8055
   for (const auto L : C->component_lists()) {
-InfoGen(std::get<0>(L), std::get<1>(L), OMPC_MAP_to, llvm::None,
+SmallVector MapModifiers;
+translateMotionModifiers(C->getMotionModifiers(), MapModifiers);

I haven't managed to locally reproduce the bot failures I saw today when trying 
to push this patch.  Somehow they're dropping the present modifiers during 
codegen.  I think it's due to memory corruption that my machines aren't 
managing to experience.  The culprit seems to be that `MapModifiers` is local 
here, but the `InfoGen` call below stores it in a `MapInfo` as an `ArrayRef`, 
which becames a dangling ref by the time it's used.

One solution is to change `MapInfo` to store a `SmallVector` instead.  Another 
would be for `MapInfo` to store a second `ArrayRef` for 
`C->getMotionModifiers()`, which would be translated to runtime map flags 
later.  I'm planning to try the latter solution tomorrow.  I prefer it because 
it seems more space efficient and because it translates motion modifiers 
directly to runtime flags instead of translating to map type modifiers first.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84711

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

[PATCH] D84711: [OpenMP] Implement TR8 `present` motion modifier in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG3c3faae49704: [OpenMP] Implement TR8 `present` motion 
modifier in Clang (1/2) (authored by jdenny).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84711

Files:
  clang/include/clang/AST/OpenMPClause.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/OpenMPKinds.def
  clang/include/clang/Basic/OpenMPKinds.h
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/OpenMPClause.cpp
  clang/lib/Basic/OpenMPKinds.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/OpenMP/declare_mapper_ast_print.c
  clang/test/OpenMP/declare_mapper_codegen.cpp
  clang/test/OpenMP/target_update_ast_print.cpp
  clang/test/OpenMP/target_update_codegen.cpp
  clang/test/OpenMP/target_update_messages.cpp

Index: clang/test/OpenMP/target_update_messages.cpp
===
--- clang/test/OpenMP/target_update_messages.cpp
+++ clang/test/OpenMP/target_update_messages.cpp
@@ -1,8 +1,10 @@
-// RUN: %clang_cc1 -verify=expected,omp45 -fopenmp -fopenmp-version=45 -ferror-limit 100 -o - -std=c++11 %s -Wuninitialized
-// RUN: %clang_cc1 -verify=expected,omp50 -fopenmp -fopenmp-version=50 -ferror-limit 100 -o - -std=c++11 %s -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,lt50,lt51 -fopenmp -fopenmp-version=45 -ferror-limit 100 -o - -std=c++11 %s -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,ge50,lt51 -fopenmp -fopenmp-version=50 -ferror-limit 100 -o - -std=c++11 %s -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,ge50,ge51 -fopenmp -fopenmp-version=51 -ferror-limit 100 -o - -std=c++11 %s -Wuninitialized
 
-// RUN: %clang_cc1 -verify=expected,omp45 -fopenmp-simd -fopenmp-version=45 -ferror-limit 100 -o - -std=c++11 %s -Wuninitialized
-// RUN: %clang_cc1 -verify=expected,omp50 -fopenmp-simd -fopenmp-version=50 -ferror-limit 100 -o - -std=c++11 %s -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,lt50,lt51 -fopenmp-simd -fopenmp-version=45 -ferror-limit 100 -o - -std=c++11 %s -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,ge50,lt51 -fopenmp-simd -fopenmp-version=50 -ferror-limit 100 -o - -std=c++11 %s -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,ge50,ge51 -fopenmp-simd -fopenmp-version=51 -ferror-limit 100 -o - -std=c++11 %s -Wuninitialized
 
 void xxx(int argc) {
   int x; // expected-note {{initialize the variable 'x' to silence this warning}}
@@ -25,6 +27,10 @@
   return 0;
 }
 
+struct S {
+  int i;
+};
+
 int main(int argc, char **argv) {
   int m;
   #pragma omp target update // expected-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
@@ -34,15 +40,114 @@
   #pragma omp target update to(m) ] // expected-warning {{extra tokens at the end of '#pragma omp target update' are ignored}}
   #pragma omp target update to(m) ) // expected-warning {{extra tokens at the end of '#pragma omp target update' are ignored}}
 
+  #pragma omp declare mapper(id: S s) map(s.i)
+  S s;
+
+  // Check parsing with no modifiers.
+  // lt51-error@+2 {{expected expression}}
+  // lt51-error@+1 {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
+  #pragma omp target update to(: s)
+  // expected-error@+2 {{expected expression}}
+  // expected-error@+1 {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
+  #pragma omp target update to(:)
+  // expected-error@+2 2 {{expected expression}}
+  // expected-error@+1 {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
+  #pragma omp target update to(,:)
+
+  // Check parsing with one modifier.
+  // expected-error@+2 {{use of undeclared identifier 'foobar'}}
+  // expected-error@+1 {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
+  #pragma omp target update to(foobar: s)
+  // expected-error@+3 {{expected ',' or ')' in 'to' clause}}
+  // expected-error@+2 {{expected ')'}}
+  // expected-note@+1 {{to match this '('}}
+  #pragma omp target update to(m: s)
+  #pragma omp target update to(mapper(id): s)
+  // lt51-error@+2 {{use of undeclared identifier 'present'}}
+  // lt51-error@+1 {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
+  #pragma omp target update to(present: s)
+  // ge51-warning@+4 {{missing ':' after motion modifier - ignoring}}
+  // lt51-warning@+3 {{missing ':' after ) - ignoring}}
+  // expected-error@+2 {{expected expression}}
+  // expected-error@+1 {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp targ

[PATCH] D84710: [OpenMP][NFC] Consolidate `to` and `from` clause modifiers

[Joel E. Denny via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa3d1f88fa7da: [OpenMP][NFC] Consolidate `to` and `from` 
clause modifiers (authored by jdenny).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84710

Files:
  clang/include/clang/Basic/OpenMPKinds.def
  clang/include/clang/Basic/OpenMPKinds.h
  clang/lib/Basic/OpenMPKinds.cpp
  clang/lib/Parse/ParseOpenMP.cpp

Index: clang/lib/Parse/ParseOpenMP.cpp
===
--- clang/lib/Parse/ParseOpenMP.cpp
+++ clang/lib/Parse/ParseOpenMP.cpp
@@ -3442,21 +3442,10 @@
   Data.ColonLoc = ConsumeToken();
   } else if (Kind == OMPC_to || Kind == OMPC_from) {
 if (Tok.is(tok::identifier)) {
-  bool IsMapperModifier = false;
-  if (Kind == OMPC_to) {
-auto Modifier =
-static_cast(getOpenMPSimpleClauseType(
-Kind, PP.getSpelling(Tok), getLangOpts().OpenMP));
-if (Modifier == OMPC_TO_MODIFIER_mapper)
-  IsMapperModifier = true;
-  } else {
-auto Modifier =
-static_cast(getOpenMPSimpleClauseType(
-Kind, PP.getSpelling(Tok), getLangOpts().OpenMP));
-if (Modifier == OMPC_FROM_MODIFIER_mapper)
-  IsMapperModifier = true;
-  }
-  if (IsMapperModifier) {
+  auto Modifier =
+  static_cast(getOpenMPSimpleClauseType(
+  Kind, PP.getSpelling(Tok), getLangOpts().OpenMP));
+  if (Modifier == OMPC_MOTION_MODIFIER_mapper) {
 // Parse the mapper modifier.
 ConsumeToken();
 IsInvalidMapperModifier = parseMapperModifier(Data);
Index: clang/lib/Basic/OpenMPKinds.cpp
===
--- clang/lib/Basic/OpenMPKinds.cpp
+++ clang/lib/Basic/OpenMPKinds.cpp
@@ -64,17 +64,12 @@
 return Type;
   }
   case OMPC_to:
-return llvm::StringSwitch(Str)
-#define OPENMP_TO_MODIFIER_KIND(Name)  \
-  .Case(#Name, static_cast(OMPC_TO_MODIFIER_##Name))
-#include "clang/Basic/OpenMPKinds.def"
-.Default(OMPC_TO_MODIFIER_unknown);
   case OMPC_from:
 return llvm::StringSwitch(Str)
-#define OPENMP_FROM_MODIFIER_KIND(Name) \
-  .Case(#Name, static_cast(OMPC_FROM_MODIFIER_##Name))
+#define OPENMP_MOTION_MODIFIER_KIND(Name)  \
+  .Case(#Name, static_cast(OMPC_MOTION_MODIFIER_##Name))
 #include "clang/Basic/OpenMPKinds.def"
-.Default(OMPC_FROM_MODIFIER_unknown);
+.Default(OMPC_MOTION_MODIFIER_unknown);
   case OMPC_dist_schedule:
 return llvm::StringSwitch(Str)
 #define OPENMP_DIST_SCHEDULE_KIND(Name) .Case(#Name, OMPC_DIST_SCHEDULE_##Name)
@@ -258,29 +253,18 @@
 }
 llvm_unreachable("Invalid OpenMP 'map' clause type");
   case OMPC_to:
-switch (Type) {
-case OMPC_TO_MODIFIER_unknown:
-  return "unknown";
-#define OPENMP_TO_MODIFIER_KIND(Name)  \
-  case OMPC_TO_MODIFIER_##Name:\
-return #Name;
-#include "clang/Basic/OpenMPKinds.def"
-default:
-  break;
-}
-llvm_unreachable("Invalid OpenMP 'to' clause type");
   case OMPC_from:
 switch (Type) {
-case OMPC_FROM_MODIFIER_unknown:
+case OMPC_MOTION_MODIFIER_unknown:
   return "unknown";
-#define OPENMP_FROM_MODIFIER_KIND(Name)\
-  case OMPC_FROM_MODIFIER_##Name:  \
+#define OPENMP_MOTION_MODIFIER_KIND(Name)  \
+  case OMPC_MOTION_MODIFIER_##Name:\
 return #Name;
 #include "clang/Basic/OpenMPKinds.def"
 default:
   break;
 }
-llvm_unreachable("Invalid OpenMP 'from' clause type");
+llvm_unreachable("Invalid OpenMP 'to' or 'from' clause type");
   case OMPC_dist_schedule:
 switch (Type) {
 case OMPC_DIST_SCHEDULE_unknown:
Index: clang/include/clang/Basic/OpenMPKinds.h
===
--- clang/include/clang/Basic/OpenMPKinds.h
+++ clang/include/clang/Basic/OpenMPKinds.h
@@ -86,20 +86,12 @@
 static constexpr unsigned NumberOfOMPMapClauseModifiers =
 OMPC_MAP_MODIFIER_last - OMPC_MAP_MODIFIER_unknown - 1;
 
-/// OpenMP modifier kind for 'to' clause.
-enum OpenMPToModifierKind {
-#define OPENMP_TO_MODIFIER_KIND(Name) \
-  OMPC_TO_MODIFIER_##Name,
+/// OpenMP modifier kind for 'to' or 'from' clause.
+enum OpenMPMotionModifierKind {
+#define OPENMP_MOTION_MODIFIER_KIND(Name) \
+  OMPC_MOTION_MODIFIER_##Name,
 #include "clang/Basic/OpenMPKinds.def"
-  OMPC_TO_MODIFIER_unknown
-};
-
-/// OpenMP modifier kind for 'from' clause.
-enum OpenMPFromModifierKind {
-#define OPENMP_FROM_MODI

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

[Joel E. Denny via Phabricator via cfe-commits]

jdenny marked an inline comment as done.
jdenny added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:8843
 llvm::Value *&MapTypesArrayArg, llvm::Value *&MappersArrayArg,
-CGOpenMPRuntime::TargetDataInfo &Info) {
+CGOpenMPRuntime::TargetDataInfo &Info, bool ForEndCall = false) {
+  assert((!ForEndCall || Info.separateBeginEndCalls()) &&

ABataev wrote:
> Do not append param here, use the one from `Info`
`Info.SeparateBeginEndCalls` and `ForEndCall` do not represent the same thing.  
If `Info.SeparateBeginEndCalls=true`, as in `emitTargetDataCalls` below, then 
`emitOffloadingArraysArgument` is called twice with the same `Info`, once with 
`ForEndCall=false` and once with `ForEndCall=true`.


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] D84711: [OpenMP] Implement TR8 `present` motion modifier in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

Thanks!


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

https://reviews.llvm.org/D84711

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

[PATCH] D84711: [OpenMP] Implement TR8 `present` motion modifier in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/lib/Basic/OpenMPKinds.cpp:73-74
 .Default(OMPC_MOTION_MODIFIER_unknown);
+if (OpenMPVersion < 51 && Type != OMPC_MOTION_MODIFIER_mapper)
+  return OMPC_MOTION_MODIFIER_unknown;
+return Type;

ABataev wrote:
> Better to check for the new clauses here:
> ```
> if (OpenMPVersion >= 51 && Type == OMPC_MOTION_MODIFIER_present)
> 
> ```
I think I've done what you're after here.  This now looks a look like the 
related code for map type modifiers above.


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

https://reviews.llvm.org/D84711

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

[PATCH] D84711: [OpenMP] Implement TR8 `present` motion modifier in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

jdenny updated this revision to Diff 281339.
jdenny marked 4 inline comments as done.
jdenny added a comment.

Adjusted logic for rejecting `present` as requested.  Rebased.


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

https://reviews.llvm.org/D84711

Files:
  clang/include/clang/AST/OpenMPClause.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/OpenMPKinds.def
  clang/include/clang/Basic/OpenMPKinds.h
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/OpenMPClause.cpp
  clang/lib/Basic/OpenMPKinds.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/OpenMP/declare_mapper_ast_print.c
  clang/test/OpenMP/declare_mapper_codegen.cpp
  clang/test/OpenMP/target_update_ast_print.cpp
  clang/test/OpenMP/target_update_codegen.cpp
  clang/test/OpenMP/target_update_messages.cpp

Index: clang/test/OpenMP/target_update_messages.cpp
===
--- clang/test/OpenMP/target_update_messages.cpp
+++ clang/test/OpenMP/target_update_messages.cpp
@@ -1,8 +1,10 @@
-// RUN: %clang_cc1 -verify=expected,omp45 -fopenmp -fopenmp-version=45 -ferror-limit 100 -o - -std=c++11 %s -Wuninitialized
-// RUN: %clang_cc1 -verify=expected,omp50 -fopenmp -fopenmp-version=50 -ferror-limit 100 -o - -std=c++11 %s -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,lt50,lt51 -fopenmp -fopenmp-version=45 -ferror-limit 100 -o - -std=c++11 %s -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,ge50,lt51 -fopenmp -fopenmp-version=50 -ferror-limit 100 -o - -std=c++11 %s -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,ge50,ge51 -fopenmp -fopenmp-version=51 -ferror-limit 100 -o - -std=c++11 %s -Wuninitialized
 
-// RUN: %clang_cc1 -verify=expected,omp45 -fopenmp-simd -fopenmp-version=45 -ferror-limit 100 -o - -std=c++11 %s -Wuninitialized
-// RUN: %clang_cc1 -verify=expected,omp50 -fopenmp-simd -fopenmp-version=50 -ferror-limit 100 -o - -std=c++11 %s -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,lt50,lt51 -fopenmp-simd -fopenmp-version=45 -ferror-limit 100 -o - -std=c++11 %s -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,ge50,lt51 -fopenmp-simd -fopenmp-version=50 -ferror-limit 100 -o - -std=c++11 %s -Wuninitialized
+// RUN: %clang_cc1 -verify=expected,ge50,ge51 -fopenmp-simd -fopenmp-version=51 -ferror-limit 100 -o - -std=c++11 %s -Wuninitialized
 
 void xxx(int argc) {
   int x; // expected-note {{initialize the variable 'x' to silence this warning}}
@@ -25,6 +27,10 @@
   return 0;
 }
 
+struct S {
+  int i;
+};
+
 int main(int argc, char **argv) {
   int m;
   #pragma omp target update // expected-error {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
@@ -34,15 +40,114 @@
   #pragma omp target update to(m) ] // expected-warning {{extra tokens at the end of '#pragma omp target update' are ignored}}
   #pragma omp target update to(m) ) // expected-warning {{extra tokens at the end of '#pragma omp target update' are ignored}}
 
+  #pragma omp declare mapper(id: S s) map(s.i)
+  S s;
+
+  // Check parsing with no modifiers.
+  // lt51-error@+2 {{expected expression}}
+  // lt51-error@+1 {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
+  #pragma omp target update to(: s)
+  // expected-error@+2 {{expected expression}}
+  // expected-error@+1 {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
+  #pragma omp target update to(:)
+  // expected-error@+2 2 {{expected expression}}
+  // expected-error@+1 {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
+  #pragma omp target update to(,:)
+
+  // Check parsing with one modifier.
+  // expected-error@+2 {{use of undeclared identifier 'foobar'}}
+  // expected-error@+1 {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
+  #pragma omp target update to(foobar: s)
+  // expected-error@+3 {{expected ',' or ')' in 'to' clause}}
+  // expected-error@+2 {{expected ')'}}
+  // expected-note@+1 {{to match this '('}}
+  #pragma omp target update to(m: s)
+  #pragma omp target update to(mapper(id): s)
+  // lt51-error@+2 {{use of undeclared identifier 'present'}}
+  // lt51-error@+1 {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
+  #pragma omp target update to(present: s)
+  // ge51-warning@+4 {{missing ':' after motion modifier - ignoring}}
+  // lt51-warning@+3 {{missing ':' after ) - ignoring}}
+  // expected-error@+2 {{expected expression}}
+  // expected-error@+1 {{expected at least one 'to' clause or 'from' clause specified to '#pragma omp target update'}}
+  #pragma omp target update to(mapper(id) s)

[PATCH] D84711: [OpenMP] Implement TR8 `present` motion modifier in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/lib/Parse/ParseOpenMP.cpp:3462
+  // OpenMP 5.1 accepts an optional ',' even if the next character is ':'.
+  // TODO: Is that intentional?
+  if (Tok.is(tok::comma))

ABataev wrote:
> `FIXME`. This is a bug.
I think you mean:

* It's a bug in TR8.
* I should change TODO to FIXME.
* This patch implements and tests the TR8 bug, and I should leave it that way 
for now.

Is all that right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84711

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

[PATCH] D84710: [OpenMP][NFC] Consolidate `to` and `from` clause modifiers

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84710

___
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`

[Joel E. Denny via Phabricator via cfe-commits]

jdenny updated this revision to Diff 281330.
jdenny added a comment.

Replaced `SeparateBeginEnd` parameter with new `TargetDataInfo` field as 
requested.  Rebased.


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

https://reviews.llvm.org/D84422

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.h
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/test/OpenMP/target_data_codegen.cpp
  openmp/libomptarget/src/omptarget.cpp
  openmp/libomptarget/test/mapping/present/target_data_at_exit.c

Index: openmp/libomptarget/test/mapping/present/target_data_at_exit.c
===
--- /dev/null
+++ openmp/libomptarget/test/mapping/present/target_data_at_exit.c
@@ -0,0 +1,37 @@
+// RUN: %libomptarget-compile-aarch64-unknown-linux-gnu -fopenmp-version=51
+// RUN: %libomptarget-run-aarch64-unknown-linux-gnu 2>&1 \
+// RUN: | %fcheck-aarch64-unknown-linux-gnu
+
+// RUN: %libomptarget-compile-powerpc64-ibm-linux-gnu -fopenmp-version=51
+// RUN: %libomptarget-run-powerpc64-ibm-linux-gnu 2>&1 \
+// RUN: | %fcheck-powerpc64-ibm-linux-gnu
+
+// RUN: %libomptarget-compile-powerpc64le-ibm-linux-gnu -fopenmp-version=51
+// RUN: %libomptarget-run-powerpc64le-ibm-linux-gnu 2>&1 \
+// RUN: | %fcheck-powerpc64le-ibm-linux-gnu
+
+// RUN: %libomptarget-compile-x86_64-pc-linux-gnu -fopenmp-version=51
+// RUN: %libomptarget-run-x86_64-pc-linux-gnu 2>&1 \
+// RUN: | %fcheck-x86_64-pc-linux-gnu
+
+#include 
+
+int main() {
+  int i;
+
+#pragma omp target enter data map(alloc:i)
+
+  // i isn't present at the end of the target data region, but the "present"
+  // modifier is only checked at the beginning of a region.
+#pragma omp target data map(present, alloc: i)
+  {
+#pragma omp target exit data map(delete:i)
+  }
+
+  // CHECK-NOT: Libomptarget
+  // CHECK: success
+  // CHECK-NOT: Libomptarget
+  fprintf(stderr, "success\n");
+
+  return 0;
+}
Index: openmp/libomptarget/src/omptarget.cpp
===
--- openmp/libomptarget/src/omptarget.cpp
+++ openmp/libomptarget/src/omptarget.cpp
@@ -484,8 +484,14 @@
   DP("Mapping does not exist (%s)\n",
  (HasPresentModifier ? "'present' map type modifier" : "ignored"));
   if (HasPresentModifier) {
-// FIXME: This should not be an error on exit from "omp target data",
-// but it should be an error upon entering an "omp target exit data".
+// This should be an error upon entering an "omp target exit data".  It
+// should not be an error upon exiting an "omp target data" or "omp
+// target".  For "omp target data", Clang thus doesn't include present
+// modifiers for end calls.  For "omp target", we have not found a valid
+// OpenMP program for which the error matters: it appears that, if a
+// program can guarantee that data is present at the beginning of an
+// "omp target" region so that there's no error there, that data is also
+// guaranteed to be present at the end.
 MESSAGE("device mapping required by 'present' map type modifier does "
 "not exist for host address " DPxMOD " (%ld bytes)",
 DPxPTR(HstPtrBegin), data_size);
Index: clang/test/OpenMP/target_data_codegen.cpp
===
--- clang/test/OpenMP/target_data_codegen.cpp
+++ clang/test/OpenMP/target_data_codegen.cpp
@@ -256,10 +256,16 @@
 double gc[100];
 
 // PRESENT=0x1000 | TARGET_PARAM=0x20 | TO=0x1 = 0x1021
-// CK1A: [[MTYPE00:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x1021]]]
+// CK1A: [[MTYPE00Begin:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x1021]]]
+
+// TARGET_PARAM=0x20 | TO=0x1 = 0x21
+// CK1A: [[MTYPE00End:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x21]]]
 
 // PRESENT=0x1000 | CLOSE=0x400 | TARGET_PARAM=0x20 | ALWAYS=0x4 | TO=0x1 = 0x1425
-// CK1A: [[MTYPE01:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x1425]]]
+// CK1A: [[MTYPE01Begin:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x1425]]]
+
+// CLOSE=0x400 | TARGET_PARAM=0x20 | ALWAYS=0x4 | TO=0x1 = 0x425
+// CK1A: [[MTYPE01End:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x425]]]
 
 // CK1A-LABEL: _Z3fooi
 void foo(int arg) {
@@ -267,7 +273,7 @@
   float lb[arg];
 
   // Region 00
-  // CK1A-DAG: call void @__tgt_target_data_begin_mapper(i64 -1, i32 1, i8** [[GEPBP:%.+]], i8** [[GEPP:%.+]], i[[sz:32|64]]* [[GEPS:%.+]], {{.+}}getelementptr {{.+}}[1 x i{{.+}}]* [[MTYPE00]]{{.+}})
+  // CK1A-DAG: call void @__tgt_target_data_begin_mapper(i64 -1, i32 1, i8** [[GEPBP:%.+]], i8** [[GEPP:%.+]], i[[sz:32|64]]* [[GEPS:%.+]], {{.+}}getelementptr {{.+}}[1 x i{{.+}}]* [[MTYPE00Begin]]{{.+}})
   // CK1A-DAG: [[GEPBP]] = getelementptr inbounds {{.+}}[[BP:%[^,]+]]
   // CK1A-DAG: [[GEPP]] = getelementptr inbounds {{.+}}[[P:%[^,]+]]
   // CK1A-DAG: [[GEPS]] = getelementptr inbounds {{.+}}[[S:%[^,]+]]
@@ -285,7 +291,7 @@
   // CK1A-32-DAG: [[CSVAL032]] = mul