[PATCH] D78643: [OpenMP] Fix false error report of array subscription for templated indexes.

[Hana Joo via Phabricator via cfe-commits]

h-joo added a comment.

In D78643#2012511 , @jdoerfert wrote:

> In D78643#2012212 , @h-joo wrote:
>
> > Sorry, I feel like I'm holding back the fix for this bug since I'm stuck on 
> > how to proceed with the build failure, @ABataev would you like to proceed 
> > with the other revision  that you were 
> > working on? If you would, I will drop this one then.
>
>
> Don't worry. What build failure exactly? The lldb libc++ failures shown here 
> in phab or something else?


Exactly, I was a bit lost by the lldb and libc++ failures


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78643



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

[PATCH] D78643: [OpenMP] Fix false error report of array subscription for templated indexes.

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D78643#2012212 , @h-joo wrote:

> Sorry, I feel like I'm holding back the fix for this bug since I'm stuck on 
> how to proceed with the build failure, @ABataev would you like to proceed 
> with the other revision  that you were 
> working on? If you would, I will drop this one then.


Don't worry. What build failure exactly? The lldb libc++ failures shown here in 
phab or something else?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78643



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

[PATCH] D78643: [OpenMP] Fix false error report of array subscription for templated indexes.

[Hana Joo via Phabricator via cfe-commits]

h-joo marked an inline comment as done.
h-joo added a comment.

Sorry, I feel like I'm holding back the fix for this bug since I'm stuck on how 
to proceed with the build failure, @ABataev would you like to proceed with the 
other revision  that you were working on? If 
you would, I will drop this one then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78643



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

[PATCH] D78643: [OpenMP] Fix false error report of array subscription for templated indexes.

[Hana Joo via Phabricator via cfe-commits]

h-joo updated this revision to Diff 259343.
h-joo added a comment.

In D78643#1997405 , @ABataev wrote:

> In D78643#1997344 , @jdoerfert wrote:
>
> > This looks reasonable to me. @ABataev WDYT?
>
>
> I would add a positive test with -ast-print




1. Changed into a positive test with -ast-print
2. Just `QualType BaseType = ASE->getBase()->getType().getNonReferenceType();` 
and dropped all the call for getNonReferenceType() in later checks.

Thank you for your time for the review! I do have one more question to ask. I 
don't understand the log of the build failure, would you be able to give me a 
bit of a hint?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78643

Files:
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/depend_template_subscription.cpp


Index: clang/test/OpenMP/depend_template_subscription.cpp
===
--- /dev/null
+++ clang/test/OpenMP/depend_template_subscription.cpp
@@ -0,0 +1,49 @@
+// RUN: %clang_cc1 -verify -fopenmp -ast-print %s | FileCheck %s
+
+#ifndef HEADER
+#define HEADER
+
+template
+void test(double *A, IndexT k)
+{
+  #pragma omp task depend(out: A[k]) 
+  {
+;
+  }
+}
+// CHECK: template  void test(double *A, IndexT k) {
+// CHECK: #pragma omp task depend(out : A[k])
+// CHECK: {
+// CHECK: ;
+// CHECK: }
+// CHECK: }
+// CHECK: template<> void test(double *A, int k) {
+// CHECK: #pragma omp task depend(out : A[k])
+// CHECK: {
+// CHECK: ;
+// CHECK: }
+// CHECK: }
+
+
+struct lValueVector {
+  int operator [] (int index) {
+return index + 42;
+  }
+};
+template
+void test2(BaseTypeT A, IndexT k)
+{
+  #pragma omp task depend(out: A[k]) // expected-error {{expected addressable 
lvalue expression, array element or array section}}
+  {
+;
+  }
+}
+int driver(double *A)
+{
+  int k = 42;
+  test(A, k);
+  test2(lValueVector(), k); // expected-note {{in instantiation of function 
template specialization 'test2' requested here}} 
+  return 0;
+}
+
+#endif
\ No newline at end of file
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -15902,20 +15902,24 @@
   continue;
 }
 
-auto *ASE = dyn_cast(SimpleExpr);
-if (!RefExpr->IgnoreParenImpCasts()->isLValue() ||
-(ASE &&
- !ASE->getBase()
-  ->getType()
-  .getNonReferenceType()
-  ->isPointerType() &&
- !ASE->getBase()->getType().getNonReferenceType()->isArrayType())) 
{
+if (!RefExpr->IgnoreParenImpCasts()->isLValue()) {
   Diag(ELoc, diag::err_omp_expected_addressable_lvalue_or_array_item)
   << (LangOpts.OpenMP >= 50 ? 1 : 0)
   << (LangOpts.OpenMP >= 50 ? 1 : 0) << RefExpr->getSourceRange();
   continue;
 }
 
+if (auto *ASE = dyn_cast(SimpleExpr)) {
+  QualType BaseType = ASE->getBase()->getType().getNonReferenceType();
+  if (!BaseType->isDependentType() && !BaseType->isPointerType() &&
+  !BaseType->isArrayType()) {
+Diag(ELoc, diag::err_omp_expected_addressable_lvalue_or_array_item)
+<< (LangOpts.OpenMP >= 50 ? 1 : 0)
+<< (LangOpts.OpenMP >= 50 ? 1 : 0) << 
RefExpr->getSourceRange();
+continue;
+  }
+}
+
 ExprResult Res;
 {
   Sema::TentativeAnalysisScope Trap(*this);


Index: clang/test/OpenMP/depend_template_subscription.cpp
===
--- /dev/null
+++ clang/test/OpenMP/depend_template_subscription.cpp
@@ -0,0 +1,49 @@
+// RUN: %clang_cc1 -verify -fopenmp -ast-print %s | FileCheck %s
+
+#ifndef HEADER
+#define HEADER
+
+template
+void test(double *A, IndexT k)
+{
+  #pragma omp task depend(out: A[k]) 
+  {
+;
+  }
+}
+// CHECK: template  void test(double *A, IndexT k) {
+// CHECK: #pragma omp task depend(out : A[k])
+// CHECK: {
+// CHECK: ;
+// CHECK: }
+// CHECK: }
+// CHECK: template<> void test(double *A, int k) {
+// CHECK: #pragma omp task depend(out : A[k])
+// CHECK: {
+// CHECK: ;
+// CHECK: }
+// CHECK: }
+
+
+struct lValueVector {
+  int operator [] (int index) {
+return index + 42;
+  }
+};
+template
+void test2(BaseTypeT A, IndexT k)
+{
+  #pragma omp task depend(out: A[k]) // expected-error {{expected addressable lvalue expression, array element or array section}}
+  {
+;
+  }
+}
+int driver(double *A)
+{
+  int k = 42;
+  test(A, k);
+  test2(lValueVector(), k); // expected-note {{in instantiation of function template specialization 'test2' requested her

[PATCH] D78643: [OpenMP] Fix false error report of array subscription for templated indexes.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D78643#1997344 , @jdoerfert wrote:

> This looks reasonable to me. @ABataev WDYT?


I would add a positive test with -ast-print




Comment at: clang/lib/Sema/SemaOpenMP.cpp:15913
+if (auto *ASE = dyn_cast(SimpleExpr)) {
+  QualType BaseType = ASE->getBase()->getType();
+  if (!BaseType->isDependentType() &&

Just `QualType BaseType = ASE->getBase()->getType().getNonReferenceType();` and 
drop the call for `getNonReferenceType()` in later checks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78643



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

[PATCH] D78643: [OpenMP] Fix false error report of array subscription for templated indexes.

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

This looks reasonable to me. @ABataev WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78643



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

[PATCH] D78643: [OpenMP] Fix false error report of array subscription for templated indexes.

[Hana Joo via Phabricator via cfe-commits]

h-joo updated this revision to Diff 259326.
h-joo added a comment.

In D78643#1997103 , @jdoerfert wrote:

> Can we create a test case that shows even if it is a dependent type we will 
> eventuall issue an error if it is not an addressable lvalue or array item?
>  If this is the part that needs refactoring to work, we should add the test 
> with a TODO.


Added a test to check the test triggers in presence of lvalue expression. 
Although the test does not trigger line 15912, but rather line 15905.  I tried 
some examples and it seems like after during  the template instantiation, while 
an ArraySubscriptExpr is being constructed, it already checks whether it's a 
pointer type or an array type, thus, I am thinking this check in line 15912 
might actually be redundant?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78643

Files:
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/depend_template_subscription.cpp


Index: clang/test/OpenMP/depend_template_subscription.cpp
===
--- /dev/null
+++ clang/test/OpenMP/depend_template_subscription.cpp
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -verify -fopenmp -std=c++11 %s -Wuninitialized
+
+#ifndef HEADER
+#define HEADER
+
+template
+void test(double *A, IndexT k)
+{
+
+  #pragma omp task depend(out: A[k]) // Should not print error Bug #45383
+  {
+;
+  }
+}
+
+struct lValueVector {
+  int operator [] (int index) {
+return index + 42;
+  }
+};
+template
+void test2(BaseTypeT A, IndexT k)
+{
+  #pragma omp task depend(out: A[k]) // expected-error {{expected addressable 
lvalue expression, array element or array section}}
+  {
+;
+  }
+}
+int driver(double *A)
+{
+  int k = 42;
+  test(A, k);
+  test2(lValueVector(), k); // expected-note {{in instantiation of function 
template specialization 'test2' requested here}} 
+  return 0;
+}
+
+#endif
\ No newline at end of file
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -15902,20 +15902,25 @@
   continue;
 }
 
-auto *ASE = dyn_cast(SimpleExpr);
-if (!RefExpr->IgnoreParenImpCasts()->isLValue() ||
-(ASE &&
- !ASE->getBase()
-  ->getType()
-  .getNonReferenceType()
-  ->isPointerType() &&
- !ASE->getBase()->getType().getNonReferenceType()->isArrayType())) 
{
+if (!RefExpr->IgnoreParenImpCasts()->isLValue()) {
   Diag(ELoc, diag::err_omp_expected_addressable_lvalue_or_array_item)
   << (LangOpts.OpenMP >= 50 ? 1 : 0)
   << (LangOpts.OpenMP >= 50 ? 1 : 0) << RefExpr->getSourceRange();
   continue;
 }
 
+if (auto *ASE = dyn_cast(SimpleExpr)) {
+  QualType BaseType = ASE->getBase()->getType();
+  if (!BaseType->isDependentType() &&
+  !BaseType.getNonReferenceType()->isPointerType() &&
+  !BaseType.getNonReferenceType()->isArrayType()) {
+Diag(ELoc, diag::err_omp_expected_addressable_lvalue_or_array_item)
+<< (LangOpts.OpenMP >= 50 ? 1 : 0)
+<< (LangOpts.OpenMP >= 50 ? 1 : 0) << 
RefExpr->getSourceRange();
+continue;
+  }
+}
+
 ExprResult Res;
 {
   Sema::TentativeAnalysisScope Trap(*this);


Index: clang/test/OpenMP/depend_template_subscription.cpp
===
--- /dev/null
+++ clang/test/OpenMP/depend_template_subscription.cpp
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -verify -fopenmp -std=c++11 %s -Wuninitialized
+
+#ifndef HEADER
+#define HEADER
+
+template
+void test(double *A, IndexT k)
+{
+
+  #pragma omp task depend(out: A[k]) // Should not print error Bug #45383
+  {
+;
+  }
+}
+
+struct lValueVector {
+  int operator [] (int index) {
+return index + 42;
+  }
+};
+template
+void test2(BaseTypeT A, IndexT k)
+{
+  #pragma omp task depend(out: A[k]) // expected-error {{expected addressable lvalue expression, array element or array section}}
+  {
+;
+  }
+}
+int driver(double *A)
+{
+  int k = 42;
+  test(A, k);
+  test2(lValueVector(), k); // expected-note {{in instantiation of function template specialization 'test2' requested here}} 
+  return 0;
+}
+
+#endif
\ No newline at end of file
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -15902,20 +15902,25 @@
   continue;
 }
 
-auto *ASE = dyn_cast(SimpleExpr);
-if (!RefExpr->IgnoreParenImpCasts()->isLValue() ||
-(ASE &&
- !ASE->getBase()
-  ->getType()
-  .getNonRef

[PATCH] D78643: [OpenMP] Fix false error report of array subscription for templated indexes.

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

Can we create a test case that shows even if it is a dependent type we will 
eventuall issue an error if it is not an addressable lvalue or array item?
If this is the part that needs refactoring to work, we should add the test with 
a TODO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78643



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

[PATCH] D78643: [OpenMP] Fix false error report of array subscription for templated indexes.

[Hana Joo via Phabricator via cfe-commits]

h-joo created this revision.
h-joo added a reviewer: ABataev.
h-joo added projects: clang, OpenMP.
Herald added subscribers: cfe-commits, arphaman, guansong, yaxunl.
Herald added a reviewer: jdoerfert.

This is a fix for Bug 45383 .

This revision fixes the error reported for array subscription for the cases 
where the array index is a template parameter.
Note that the `ArraySubscriptExpr::getBase()` might return something which 
might not be a base in the cases where either LHS or RHS of the 
`ArraySubscriptExpr` is a template declaration - I think the fundemental fix 
would include more changes with `ArraySubscriptExpr::getBase()` in the presence 
of template dependent types, but I thought it would involve a bigger scope of 
refactoring since many of the code is touching it.

Please also keep in mind this is my first patch, so I would be very glad for 
any kind of comment if I did something wrong.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78643

Files:
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/depend_template_subscription.cpp


Index: clang/test/OpenMP/depend_template_subscription.cpp
===
--- /dev/null
+++ clang/test/OpenMP/depend_template_subscription.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -verify -fopenmp -std=c++11 %s -Wuninitialized
+
+#ifndef HEADER
+#define HEADER
+
+template
+void test(double *A, IndexT k)
+{
+  #pragma omp task depend(out: A[k]) // expected-no-diagnostics
+  {
+;
+  }
+}
+
+int driver(double *A)
+{
+  int k = 0;
+  test(A, k);
+  return 0;
+}
+
+#endif
\ No newline at end of file
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -15902,20 +15902,25 @@
   continue;
 }
 
-auto *ASE = dyn_cast(SimpleExpr);
-if (!RefExpr->IgnoreParenImpCasts()->isLValue() ||
-(ASE &&
- !ASE->getBase()
-  ->getType()
-  .getNonReferenceType()
-  ->isPointerType() &&
- !ASE->getBase()->getType().getNonReferenceType()->isArrayType())) 
{
+if (!RefExpr->IgnoreParenImpCasts()->isLValue()) {
   Diag(ELoc, diag::err_omp_expected_addressable_lvalue_or_array_item)
   << (LangOpts.OpenMP >= 50 ? 1 : 0)
   << (LangOpts.OpenMP >= 50 ? 1 : 0) << RefExpr->getSourceRange();
   continue;
 }
 
+if (auto *ASE = dyn_cast(SimpleExpr)) {
+  QualType BaseType = ASE->getBase()->getType();
+  if (!BaseType->isDependentType() &&
+  !BaseType.getNonReferenceType()->isPointerType() &&
+  !BaseType.getNonReferenceType()->isArrayType()) {
+Diag(ELoc, diag::err_omp_expected_addressable_lvalue_or_array_item)
+<< (LangOpts.OpenMP >= 50 ? 1 : 0)
+<< (LangOpts.OpenMP >= 50 ? 1 : 0) << 
RefExpr->getSourceRange();
+continue;
+  }
+}
+
 ExprResult Res;
 {
   Sema::TentativeAnalysisScope Trap(*this);


Index: clang/test/OpenMP/depend_template_subscription.cpp
===
--- /dev/null
+++ clang/test/OpenMP/depend_template_subscription.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -verify -fopenmp -std=c++11 %s -Wuninitialized
+
+#ifndef HEADER
+#define HEADER
+
+template
+void test(double *A, IndexT k)
+{
+  #pragma omp task depend(out: A[k]) // expected-no-diagnostics
+  {
+;
+  }
+}
+
+int driver(double *A)
+{
+  int k = 0;
+  test(A, k);
+  return 0;
+}
+
+#endif
\ No newline at end of file
Index: clang/lib/Sema/SemaOpenMP.cpp
===
--- clang/lib/Sema/SemaOpenMP.cpp
+++ clang/lib/Sema/SemaOpenMP.cpp
@@ -15902,20 +15902,25 @@
   continue;
 }
 
-auto *ASE = dyn_cast(SimpleExpr);
-if (!RefExpr->IgnoreParenImpCasts()->isLValue() ||
-(ASE &&
- !ASE->getBase()
-  ->getType()
-  .getNonReferenceType()
-  ->isPointerType() &&
- !ASE->getBase()->getType().getNonReferenceType()->isArrayType())) {
+if (!RefExpr->IgnoreParenImpCasts()->isLValue()) {
   Diag(ELoc, diag::err_omp_expected_addressable_lvalue_or_array_item)
   << (LangOpts.OpenMP >= 50 ? 1 : 0)
   << (LangOpts.OpenMP >= 50 ? 1 : 0) << RefExpr->getSourceRange();
   continue;
 }
 
+if (auto *ASE = dyn_cast(SimpleExpr)) {
+  QualType BaseType = ASE->getBase()->getType();
+  if (!BaseType->isDependentType() &&
+  !BaseType.getNonReferenceType()->isPointerType() &&
+  !BaseType.getNonReferenceType()->isArrayType()) {
+Diag(ELoc, diag::err_omp_expected_addressable_lva