Skip to content

Avoid unevaluated implicit private #92055

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

SunilKuravinakop
Copy link
Contributor

For every variable used under #pragma omp task directive (DeclRefExpr) an ImplicitPrivateVariable is created in the AST, if private or shared clauses are not present. If the variable has the property of non_odr_use_unevaluated e.g. for statements which use sizeof( i ) i will have non_odr_use_unevaluated . In such cases CodeGen was asserting by avoiding emitting of LLVM IR for such variables. To prevent this assertion this checkin avoids adding the ImplicitPrivateVariable for variables with non_odr_use_unevaluated.

Sunil Kuravinakop added 4 commits May 9, 2024 12:09
…cit Private variable" DeclRefExpr.

  Changes to be committed:
 	modified:   clang/lib/Sema/SemaOpenMP.cpp
directive (when variable can be non_odr_use_unevaluated).

  Changes to be committed:
 	modified:   clang/test/OpenMP/task_ast_print.cpp
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:openmp OpenMP related changes to Clang labels May 14, 2024
@llvmbot
Copy link
Member

llvmbot commented May 14, 2024

@llvm/pr-subscribers-clang

Author: None (SunilKuravinakop)

Changes

For every variable used under #pragma omp task directive (DeclRefExpr) an ImplicitPrivateVariable is created in the AST, if private or shared clauses are not present. If the variable has the property of non_odr_use_unevaluated e.g. for statements which use sizeof( i ) i will have non_odr_use_unevaluated . In such cases CodeGen was asserting by avoiding emitting of LLVM IR for such variables. To prevent this assertion this checkin avoids adding the ImplicitPrivateVariable for variables with non_odr_use_unevaluated.


Full diff: https://github.com/llvm/llvm-project/pull/92055.diff

2 Files Affected:

  • (modified) clang/lib/Sema/SemaOpenMP.cpp (+2-1)
  • (modified) clang/test/OpenMP/task_ast_print.cpp (+34)
diff --git a/clang/lib/Sema/SemaOpenMP.cpp b/clang/lib/Sema/SemaOpenMP.cpp
index 7d00cf6fb5b6a..6110e5229b076 100644
--- a/clang/lib/Sema/SemaOpenMP.cpp
+++ b/clang/lib/Sema/SemaOpenMP.cpp
@@ -3757,7 +3757,8 @@ class DSAAttrChecker final : public StmtVisitor<DSAAttrChecker, void> {
   void VisitDeclRefExpr(DeclRefExpr *E) {
     if (TryCaptureCXXThisMembers || E->isTypeDependent() ||
         E->isValueDependent() || E->containsUnexpandedParameterPack() ||
-        E->isInstantiationDependent())
+        E->isInstantiationDependent() ||
+        E->isNonOdrUse() == clang::NOUR_Unevaluated)
       return;
     if (auto *VD = dyn_cast<VarDecl>(E->getDecl())) {
       // Check the datasharing rules for the expressions in the clauses.
diff --git a/clang/test/OpenMP/task_ast_print.cpp b/clang/test/OpenMP/task_ast_print.cpp
index 12923e6ab4244..9d545c5f6716c 100644
--- a/clang/test/OpenMP/task_ast_print.cpp
+++ b/clang/test/OpenMP/task_ast_print.cpp
@@ -5,6 +5,7 @@
 // RUN: %clang_cc1 -verify -Wno-vla -fopenmp-simd -ast-print %s | FileCheck %s
 // RUN: %clang_cc1 -fopenmp-simd -x c++ -std=c++11 -emit-pch -o %t %s
 // RUN: %clang_cc1 -fopenmp-simd -std=c++11 -include-pch %t -verify -Wno-vla %s -ast-print | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fopenmp -ast-dump  %s | FileCheck %s --check-prefix=DUMP
 // expected-no-diagnostics
 
 #ifndef HEADER
@@ -208,4 +209,37 @@ int main(int argc, char **argv) {
 extern template int S<int>::TS;
 extern template long S<long>::TS;
 
+int
+implicit_firstprivate() {
+
+#pragma omp parallel num_threads(1)
+  {
+    int i = 0;
+    // DUMP : OMPTaskDirective
+    // DUMP-NEXT : OMPFirstprivateClause
+    // DUMP-NEXT : DeclRefExpr {{.+}} 'i' {{.+}} refers_to_enclosing_variable_or_capture
+    #pragma omp task
+    {
+	int j = sizeof(i);
+	j = i;
+    }
+  }
+}
+
+int
+no_implicit_firstprivate() {
+
+#pragma omp parallel num_threads(1)
+  {
+    int i = 0;
+    // DUMP : OMPTaskDirective
+    // DUMP-NEXT : CapturedStmt
+    // DUMP : DeclRefExpr {{.+}} 'i' {{.+}} non_odr_use_unevaluated refers_to_enclosing_variable_or_capture
+    #pragma omp task
+    {
+	int j = sizeof(i);
+    }
+  }
+}
+
 #endif

  Changes to be committed:
 	modified:   clang/test/OpenMP/task_ast_print.cpp
@chichunchen chichunchen requested a review from alexey-bataev May 14, 2024 17:59
int i = 0;
// DUMP: OMPTaskDirective
// DUMP-NEXT: OMPFirstprivateClause
// DUMP-NEXT: DeclRefExpr {{.+}} 'i' {{.+}} refers_to_enclosing_variable_or_capture
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could still match if non_odr_use_unevaluated shows up, can't it? Can we add a check that non_odr_use_unevaluated should not be part of the DeclRefRexpr under this firstprivate clause?

Sunil Kuravinakop added 2 commits May 15, 2024 02:32
  This could still match if non_odr_use_unevaluated shows up, can't it?
  Can we add a check that non_odr_use_unevaluated should not be part of
  the DeclRefRexpr under this firstprivate clause?

  An example of DeclRefExpr seen on "-Xclang -ast-dump" is:
     -DeclRefExpr 0x187e0e0 <line:225:6> 'int' lvalue Var 0x187da20 'i' 'int' refers_to_enclosing_variable_or_capture

  Changes to be committed:
 	modified:   clang/test/OpenMP/task_ast_print.cpp
 DUMP-NOT: DeclRefExpr {{.+}} 'i' {{.+}} non_odr_use_unevaluated

  Changes to be committed:
 	modified:   clang/test/OpenMP/task_ast_print.cpp
Copy link
Member

@alexey-bataev alexey-bataev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG

@SunilKuravinakop
Copy link
Contributor Author

Alexey,
My colleagues (in my company), having contribution rights, are not around. Can you please merge this change to master?

@alexey-bataev alexey-bataev merged commit ce67fcf into llvm:main May 16, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:openmp OpenMP related changes to Clang clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants