[clang] 0818433 - Remove the outdated feature macro '__cpp_coroutines'

[Chuanqi Xu via cfe-commits]

Author: Chuanqi Xu
Date: 2023-03-17T13:52:18+08:00
New Revision: 0818433a5515df24c5d2cd95087e375ba8596cb0

URL: 
https://github.com/llvm/llvm-project/commit/0818433a5515df24c5d2cd95087e375ba8596cb0
DIFF: 
https://github.com/llvm/llvm-project/commit/0818433a5515df24c5d2cd95087e375ba8596cb0.diff

LOG: Remove the outdated feature macro '__cpp_coroutines'

The feature macro '__cpp_coroutines' is for coroutines TS. And the
coroutines TS is deprecated. So we should remove the feature macro too.

BTW, the corresponding feature macro for standard c++ coroutines is
'__cpp_impl_coroutine'.

Added: 


Modified: 
clang/lib/Frontend/InitPreprocessor.cpp
clang/test/Lexer/cxx-features.cpp
clang/test/Modules/Inputs/DependsOnModule.framework/Headers/coroutines.h
clang/test/Modules/Inputs/DependsOnModule.framework/Headers/not_coroutines.h

Removed: 




diff  --git a/clang/lib/Frontend/InitPreprocessor.cpp 
b/clang/lib/Frontend/InitPreprocessor.cpp
index ab00724af2fa9..0b39dd1c84f7d 100644
--- a/clang/lib/Frontend/InitPreprocessor.cpp
+++ b/clang/lib/Frontend/InitPreprocessor.cpp
@@ -707,10 +707,6 @@ static void InitializeCPlusPlusFeatureTestMacros(const 
LangOptions ,
   if (LangOpts.Char8)
 Builder.defineMacro("__cpp_char8_t", "202207L");
   Builder.defineMacro("__cpp_impl_destroying_delete", "201806L");
-
-  // TS features.
-  if (LangOpts.Coroutines)
-Builder.defineMacro("__cpp_coroutines", "201703L");
 }
 
 /// InitializeOpenCLFeatureTestMacros - Define OpenCL macros based on target

diff  --git a/clang/test/Lexer/cxx-features.cpp 
b/clang/test/Lexer/cxx-features.cpp
index 5dee2c8197c89..ca7f987b78ab5 100644
--- a/clang/test/Lexer/cxx-features.cpp
+++ b/clang/test/Lexer/cxx-features.cpp
@@ -356,9 +356,3 @@
 #if defined(NO_EXCEPTIONS) ? check(exceptions, 0, 0, 0, 0, 0, 0) : 
check(exceptions, 199711, 199711, 199711, 199711, 199711, 199711)
 #error "wrong value for __cpp_exceptions"
 #endif
-
-// --- TS features --
-
-#if check(coroutines, 0, 0, 0, 0, 201703, 201703)
-#error "wrong value for __cpp_coroutines"
-#endif

diff  --git 
a/clang/test/Modules/Inputs/DependsOnModule.framework/Headers/coroutines.h 
b/clang/test/Modules/Inputs/DependsOnModule.framework/Headers/coroutines.h
index 85281f5a09959..cae4099835d73 100644
--- a/clang/test/Modules/Inputs/DependsOnModule.framework/Headers/coroutines.h
+++ b/clang/test/Modules/Inputs/DependsOnModule.framework/Headers/coroutines.h
@@ -1,3 +1,3 @@
-#ifndef __cpp_coroutines
+#ifndef __cpp_impl_coroutine
 #error coroutines must be enabled
 #endif

diff  --git 
a/clang/test/Modules/Inputs/DependsOnModule.framework/Headers/not_coroutines.h 
b/clang/test/Modules/Inputs/DependsOnModule.framework/Headers/not_coroutines.h
index 9312b9ad8926e..bf135a2440662 100644
--- 
a/clang/test/Modules/Inputs/DependsOnModule.framework/Headers/not_coroutines.h
+++ 
b/clang/test/Modules/Inputs/DependsOnModule.framework/Headers/not_coroutines.h
@@ -1,3 +1,3 @@
-#ifdef __cpp_coroutines
+#ifdef __cpp_impl_coroutine
 #error coroutines must NOT be enabled
 #endif



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

[PATCH] D146187: [docs] Update the status for coroutines

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

> Should we also be updating InitPreprocessor.cpp at the same time, for 
> non-Windows targets?

I didn't address this since we didn't do this before. (Defining feature macro 
according to the targets).

Also I feel it may be bad for windows users. Since currently the coroutines on 
windows is not completely broken. It is still workable in some (or a lot?) 
situations. It is just out of maintenance.


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

https://reviews.llvm.org/D146187

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

[PATCH] D146090: [Clang] Updating handling of defaulted comparison operators to reflect changes from P2448R2

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9410-9421
+def ext_incorrect_defaulted_comparison_constexpr : Extension<
   "defaulted definition of %select{%sub{select_defaulted_comparison_kind}1|"
   "three-way comparison operator}0 "
-  "cannot be declared %select{constexpr|consteval}2 because "
+  "declared %select{constexpr|consteval}2 but "
   "%select{it|the corresponding implicit 'operator=='}0 "
-  "invokes a non-constexpr comparison function">;
+  "invokes a non-constexpr comparison function are a C++2b extension">, 
InGroup;
+def warn_cxx2b_incorrect_defaulted_comparison_constexpr : Warning<

rsmith wrote:
> The grammar of these diagnostics looks a bit peculiar. Suggested an 
> alternative, but it's still a bit awkward ("...but for which..."), maybe you 
> can find something better.
@aaron.ballman any suggestions for less awkward wording here? I don't think 
what there is here now is too bad but asking for a another opinion.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8808-8809
   if (FD->isConstexpr()) {
 if (CheckConstexprReturnType(*this, FD, CheckConstexprKind::Diagnose) &&
 CheckConstexprParameterTypes(*this, FD, CheckConstexprKind::Diagnose) 
&&
 !Info.Constexpr) {

rsmith wrote:
> We'll also be getting diagnostics here if the parameter and return types are 
> not literal types. You can pass `...::CheckValid` instead of `...::Diagnose` 
> to find out if there would be an error, but we should probably instead make 
> the `CheckConstexpr...` functions produce warnings (`ExtWarn` prior to C++23) 
> rather than errors in general. That's a much larger change, though -- there's 
> probably half a dozen diagnostics in the `CheckConstexpr*` functions that 
> will need to be updated to properly support P2448R2. If you just want to 
> implement this one part of P2448 for now and leave the rest for a later 
> change, that seems fine to me.
I don't think I have the bandwidth for a larger change but I can file a bug and 
assign myself to it and I will put in my work queue.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8813
+ (getLangOpts().CPlusPlus2b ? 
diag::warn_cxx2b_incorrect_defaulted_comparison_constexpr :
+
diag::ext_incorrect_defaulted_comparison_constexpr ))
   << FD->isImplicit() << (int)DCK << FD->isConsteval();

rsmith wrote:
> Don't need the parens around this. This layout is a little unusual, what does 
> clang-format do here?
My bad, I forgot to apply clang-format.


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

https://reviews.llvm.org/D146090

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

[PATCH] D146090: [Clang] Updating handling of defaulted comparison operators to reflect changes from P2448R2

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik updated this revision to Diff 505986.
shafik added a comment.

- Added release note
- Update CXX status


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

https://reviews.llvm.org/D146090

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/CXX/class/class.compare/class.compare.default/p3.cpp
  clang/test/CXX/class/class.compare/class.compare.default/p4.cpp
  clang/www/cxx_status.html

Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -1482,7 +1482,7 @@
 
   Relaxing some constexpr restrictions
   https://wg21.link/P2448R2;>P2448R2
-  No
+  Partial
 
 
   Using unknown pointers and references in constant expressions
Index: clang/test/CXX/class/class.compare/class.compare.default/p4.cpp
===
--- clang/test/CXX/class/class.compare/class.compare.default/p4.cpp
+++ clang/test/CXX/class/class.compare/class.compare.default/p4.cpp
@@ -1,7 +1,9 @@
 // RUN: %clang_cc1 -std=c++2a -verify %s
+// RUN: %clang_cc1 -std=c++2a -Wc++2b-default-comp-relaxed-constexpr -verify=expected,extension %s
 
 // This test is for [class.compare.default]p3 as modified and renumbered to p4
 // by P2002R0.
+// Also covers modifications made by P2448R2 and extension warnings
 
 namespace std {
   struct strong_ordering {
@@ -76,14 +78,13 @@
 }
 
 struct H {
-  bool operator==(const H&) const; // expected-note {{here}}
+  bool operator==(const H&) const; // extension-note {{non-constexpr comparison function declared here}}
   constexpr std::strong_ordering operator<=>(const H&) const { return std::strong_ordering::equal; }
 };
 
 struct I {
-  H h; // expected-note {{used to compare}}
-  // expected-error@+1 {{defaulted definition of three-way comparison operator cannot be declared constexpr because the corresponding implicit 'operator==' invokes a non-constexpr comparison function}}
-  constexpr std::strong_ordering operator<=>(const I&) const = default;
+  H h; // extension-note {{non-constexpr comparison function would be used to compare member 'h'}}
+  constexpr std::strong_ordering operator<=>(const I&) const = default; // extension-warning {{implicit 'operator=='  invokes a non-constexpr comparison function is a C++2b extension}}
 };
 
 struct J {
@@ -144,3 +145,19 @@
   };
   bool test_d = D() == D();
 }
+
+namespace GH61238 {
+template  struct my_struct {
+A value; // extension-note {{non-constexpr comparison function would be used to compare member 'value'}}
+
+constexpr friend bool operator==(const my_struct &, const my_struct &) noexcept = default; // no diagnostic extension-warning {{declared constexpr but invokes a non-constexpr comparison function is a C++2b extension}}
+};
+
+struct non_constexpr_type {
+friend bool operator==(non_constexpr_type, non_constexpr_type) noexcept { // extension-note {{non-constexpr comparison function declared here}}
+return false;
+}
+};
+
+my_struct obj; // extension-note {{in instantiation of template class 'GH61238::my_struct' requested here}}
+}
Index: clang/test/CXX/class/class.compare/class.compare.default/p3.cpp
===
--- clang/test/CXX/class/class.compare/class.compare.default/p3.cpp
+++ clang/test/CXX/class/class.compare/class.compare.default/p3.cpp
@@ -1,6 +1,8 @@
 // This test is for the [class.compare.default]p3 added by P2002R0
+// Also covers modifications made by P2448R2 and extension warnings
 
 // RUN: %clang_cc1 -std=c++2a -verify %s
+// RUN: %clang_cc1 -std=c++2a -Wc++2b-default-comp-relaxed-constexpr -verify=expected,extension %s
 
 namespace std {
   struct strong_ordering {
@@ -80,10 +82,10 @@
 };
 
 struct C {
-  friend bool operator==(const C&, const C&); // expected-note {{previous}} expected-note 2{{here}}
+  friend bool operator==(const C&, const C&); // expected-note {{previous}} extension-note 2{{non-constexpr comparison function declared here}}
   friend bool operator!=(const C&, const C&) = default; // expected-note {{previous}}
 
-  friend std::strong_ordering operator<=>(const C&, const C&); // expected-note {{previous}} expected-note 2{{here}}
+  friend std::strong_ordering operator<=>(const C&, const C&); // expected-note {{previous}} extension-note 2{{non-constexpr comparison function declared here}}
   friend bool operator<(const C&, const C&) = default; // expected-note {{previous}}
   friend bool operator<=(const C&, const C&) = default; // expected-note {{previous}}
   friend bool operator>(const C&, const C&) = default; // expected-note {{previous}}
@@ -127,23 +129,23 @@
 
 struct E {
   A a;
-  C c; // expected-note 2{{non-constexpr comparison function would be used to compare member 'c'}}
+  C c; // 

[PATCH] D146089: [Sema] Fix null pointer dereference handleAlwaysInlineAttr.

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

Recommitted at e82d2e8c9ca34bcccb8fef67a8727543a978 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146089

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

[clang] e82d2e8 - Recommit "[Sema] Fix null pointer dereference handleAlwaysInlineAttr."

[Craig Topper via cfe-commits]

Author: Craig Topper
Date: 2023-03-16T21:37:35-07:00
New Revision: e82d2e8c9ca34bcccb8fef67a8727543a978

URL: 
https://github.com/llvm/llvm-project/commit/e82d2e8c9ca34bcccb8fef67a8727543a978
DIFF: 
https://github.com/llvm/llvm-project/commit/e82d2e8c9ca34bcccb8fef67a8727543a978.diff

LOG: Recommit "[Sema] Fix null pointer dereference handleAlwaysInlineAttr."

Remove use of constexpr if that failed on the build bots.

Original commit message:

It's possible for `getCalleeDecl()` to return a null pointer.

This was encountered by a user of our downstream compiler.

The case involved a DependentScopeDeclRefExpr.

Since this seems to only be for a warning diagnostic, I skipped
the diagnostic check if it returned null. But mabye there's a
different way to fix this.

Added: 


Modified: 
clang/lib/Sema/SemaStmtAttr.cpp
clang/test/Sema/attr-alwaysinline.cpp
clang/test/Sema/attr-noinline.cpp

Removed: 




diff  --git a/clang/lib/Sema/SemaStmtAttr.cpp b/clang/lib/Sema/SemaStmtAttr.cpp
index 6d443837a4c57..eeef85373ccb1 100644
--- a/clang/lib/Sema/SemaStmtAttr.cpp
+++ b/clang/lib/Sema/SemaStmtAttr.cpp
@@ -233,7 +233,8 @@ static Attr *handleNoInlineAttr(Sema , Stmt *St, const 
ParsedAttr ,
 
   for (const auto *CallExpr : CEF.getCallExprs()) {
 const Decl *Decl = CallExpr->getCalleeDecl();
-if (Decl->hasAttr() || Decl->hasAttr())
+if (Decl &&
+(Decl->hasAttr() || Decl->hasAttr()))
   S.Diag(St->getBeginLoc(), diag::warn_function_stmt_attribute_precedence)
   << A << (Decl->hasAttr() ? 0 : 1);
   }
@@ -259,7 +260,7 @@ static Attr *handleAlwaysInlineAttr(Sema , Stmt *St, 
const ParsedAttr ,
 
   for (const auto *CallExpr : CEF.getCallExprs()) {
 const Decl *Decl = CallExpr->getCalleeDecl();
-if (Decl->hasAttr() || Decl->hasAttr())
+if (Decl && (Decl->hasAttr() || 
Decl->hasAttr()))
   S.Diag(St->getBeginLoc(), diag::warn_function_stmt_attribute_precedence)
   << A << (Decl->hasAttr() ? 2 : 1);
   }

diff  --git a/clang/test/Sema/attr-alwaysinline.cpp 
b/clang/test/Sema/attr-alwaysinline.cpp
index 6b8e8f215a4b6..be3f74c8bfa9d 100644
--- a/clang/test/Sema/attr-alwaysinline.cpp
+++ b/clang/test/Sema/attr-alwaysinline.cpp
@@ -25,3 +25,16 @@ void foo() {
 }
 
 [[clang::always_inline]] static int i = bar(); // expected-warning 
{{'always_inline' attribute only applies to functions and statements}}
+
+// This used to crash the compiler.
+template
+int foo(int x) {
+  [[clang::always_inline]] return foo(x + 1);
+}
+
+// FIXME: This should warn that always_inline statement attribute has higher
+// precedence than the noinline function attribute.
+template [[gnu::noinline]]
+int bar(int x) {
+  [[clang::always_inline]] return bar(x + 1);
+}

diff  --git a/clang/test/Sema/attr-noinline.cpp 
b/clang/test/Sema/attr-noinline.cpp
index d35782f11adbb..ae0f80ca296eb 100644
--- a/clang/test/Sema/attr-noinline.cpp
+++ b/clang/test/Sema/attr-noinline.cpp
@@ -25,3 +25,16 @@ void foo() {
 }
 
 [[clang::noinline]] static int i = bar(); // expected-warning {{'noinline' 
attribute only applies to functions and statements}}
+
+// This used to crash the compiler.
+template
+int foo(int x) {
+  [[clang::noinline]] return foo(x + 1);
+}
+
+// FIXME: This should warn that noinline statement attribute has higher
+// precedence than the always_inline function attribute.
+template [[clang::always_inline]]
+int bar(int x) {
+  [[clang::noinline]] return bar(x + 1);
+}



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

[PATCH] D146090: [Clang] Updating handling of defaulted comparison operators to reflect changes from P2448R2

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik updated this revision to Diff 505985.
shafik marked 6 inline comments as done.
shafik added a comment.

- Updating diagnostic wording
- Adding Reference to changes from p2448r2 to the comments
- Modified p3 tests to have an extension pass as well


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

https://reviews.llvm.org/D146090

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/CXX/class/class.compare/class.compare.default/p3.cpp
  clang/test/CXX/class/class.compare/class.compare.default/p4.cpp

Index: clang/test/CXX/class/class.compare/class.compare.default/p4.cpp
===
--- clang/test/CXX/class/class.compare/class.compare.default/p4.cpp
+++ clang/test/CXX/class/class.compare/class.compare.default/p4.cpp
@@ -1,7 +1,9 @@
 // RUN: %clang_cc1 -std=c++2a -verify %s
+// RUN: %clang_cc1 -std=c++2a -Wc++2b-default-comp-relaxed-constexpr -verify=expected,extension %s
 
 // This test is for [class.compare.default]p3 as modified and renumbered to p4
 // by P2002R0.
+// Also covers modifications made by P2448R2 and extension warnings
 
 namespace std {
   struct strong_ordering {
@@ -76,14 +78,13 @@
 }
 
 struct H {
-  bool operator==(const H&) const; // expected-note {{here}}
+  bool operator==(const H&) const; // extension-note {{non-constexpr comparison function declared here}}
   constexpr std::strong_ordering operator<=>(const H&) const { return std::strong_ordering::equal; }
 };
 
 struct I {
-  H h; // expected-note {{used to compare}}
-  // expected-error@+1 {{defaulted definition of three-way comparison operator cannot be declared constexpr because the corresponding implicit 'operator==' invokes a non-constexpr comparison function}}
-  constexpr std::strong_ordering operator<=>(const I&) const = default;
+  H h; // extension-note {{non-constexpr comparison function would be used to compare member 'h'}}
+  constexpr std::strong_ordering operator<=>(const I&) const = default; // extension-warning {{implicit 'operator=='  invokes a non-constexpr comparison function is a C++2b extension}}
 };
 
 struct J {
@@ -144,3 +145,19 @@
   };
   bool test_d = D() == D();
 }
+
+namespace GH61238 {
+template  struct my_struct {
+A value; // extension-note {{non-constexpr comparison function would be used to compare member 'value'}}
+
+constexpr friend bool operator==(const my_struct &, const my_struct &) noexcept = default; // no diagnostic extension-warning {{declared constexpr but invokes a non-constexpr comparison function is a C++2b extension}}
+};
+
+struct non_constexpr_type {
+friend bool operator==(non_constexpr_type, non_constexpr_type) noexcept { // extension-note {{non-constexpr comparison function declared here}}
+return false;
+}
+};
+
+my_struct obj; // extension-note {{in instantiation of template class 'GH61238::my_struct' requested here}}
+}
Index: clang/test/CXX/class/class.compare/class.compare.default/p3.cpp
===
--- clang/test/CXX/class/class.compare/class.compare.default/p3.cpp
+++ clang/test/CXX/class/class.compare/class.compare.default/p3.cpp
@@ -1,6 +1,8 @@
 // This test is for the [class.compare.default]p3 added by P2002R0
+// Also covers modifications made by P2448R2 and extension warnings
 
 // RUN: %clang_cc1 -std=c++2a -verify %s
+// RUN: %clang_cc1 -std=c++2a -Wc++2b-default-comp-relaxed-constexpr -verify=expected,extension %s
 
 namespace std {
   struct strong_ordering {
@@ -80,10 +82,10 @@
 };
 
 struct C {
-  friend bool operator==(const C&, const C&); // expected-note {{previous}} expected-note 2{{here}}
+  friend bool operator==(const C&, const C&); // expected-note {{previous}} extension-note 2{{non-constexpr comparison function declared here}}
   friend bool operator!=(const C&, const C&) = default; // expected-note {{previous}}
 
-  friend std::strong_ordering operator<=>(const C&, const C&); // expected-note {{previous}} expected-note 2{{here}}
+  friend std::strong_ordering operator<=>(const C&, const C&); // expected-note {{previous}} extension-note 2{{non-constexpr comparison function declared here}}
   friend bool operator<(const C&, const C&) = default; // expected-note {{previous}}
   friend bool operator<=(const C&, const C&) = default; // expected-note {{previous}}
   friend bool operator>(const C&, const C&) = default; // expected-note {{previous}}
@@ -127,23 +129,23 @@
 
 struct E {
   A a;
-  C c; // expected-note 2{{non-constexpr comparison function would be used to compare member 'c'}}
+  C c; // extension-note 2{{non-constexpr comparison function would be used to compare member 'c'}}
   A b;
-  friend constexpr bool operator==(const E&, const E&) = default; // expected-error {{cannot be declared constexpr because it invokes a non-constexpr comparison function}}
+  friend constexpr bool 

[PATCH] D146187: [docs] Update the status for coroutines

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

> but we don't define __cpp_­impl_­coroutine: 
> http://eel.is/c++draft/tab:cpp.predefined.ft

We defined `__cpp_­impl_­coroutine`.  And `__cpp_coroutines` is for Coroutines 
TS. I forgot to remove this too. I'll handle it in a separate patch.


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

https://reviews.llvm.org/D146187

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

[PATCH] D146187: [docs] Update the status for coroutines

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 505979.
ChuanqiXu added a comment.

Address comments.


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

https://reviews.llvm.org/D146187

Files:
  clang/docs/ReleaseNotes.rst
  clang/www/cxx_status.html


Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -1222,10 +1222,8 @@
   https://wg21.link/p0912r5;>P0912R5
   
 Partial
-  The optimizer does not yet handle TLS with
-  __attribute__((const)) attribute correctly. There can be issues 
where the
-  coroutine may resume on a different thread. This feature requires 
further
-  analysis of the C++ Standard to determine what work is necessary for 
conformance.
+  Fully supported on all targets except Windows, which
+  still has some stability and ABI issues.
 
 
 
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -80,6 +80,8 @@
 ^
 - Support for out-of-line definitions of constrained templates has been 
improved.
   This partially fixes `#49620 
`_.
+- Announced C++20 Coroutines is fully supported on all targets except Windows, 
which
+  still has some stability and ABI issues.
 
 C++2b Feature Support
 ^


Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -1222,10 +1222,8 @@
   https://wg21.link/p0912r5;>P0912R5
   
 Partial
-  The optimizer does not yet handle TLS with
-  __attribute__((const)) attribute correctly. There can be issues where the
-  coroutine may resume on a different thread. This feature requires further
-  analysis of the C++ Standard to determine what work is necessary for conformance.
+  Fully supported on all targets except Windows, which
+  still has some stability and ABI issues.
 
 
 
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -80,6 +80,8 @@
 ^
 - Support for out-of-line definitions of constrained templates has been improved.
   This partially fixes `#49620 `_.
+- Announced C++20 Coroutines is fully supported on all targets except Windows, which
+  still has some stability and ABI issues.
 
 C++2b Feature Support
 ^
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

RE: [clang] 1029747 - [Sema] Fix null pointer dereference handleAlwaysInlineAttr.

[Voss, Matthew via cfe-commits]

LG. Thanks!

> -Original Message-
> From: Craig Topper 
> Sent: Thursday, March 16, 2023 8:01 PM
> To: Voss, Matthew 
> Cc: Craig Topper ; cfe-commits@lists.llvm.org
> Subject: Re: [clang] 1029747 - [Sema] Fix null pointer dereference
> handleAlwaysInlineAttr.
> 
> Thanks for the heads up. Looks like we have a different default C++ standard
> enabled than what the tests got in my local testing. I’ll revert and make a 
> fix.
> 
> > On Mar 16, 2023, at 7:53 PM, Voss, Matthew 
> wrote:
> >
> > Hi Craig,
> >
> > There are some failures on the PS5 buildbot after this change. Could you 
> > take a
> look?
> >
> > INVALID URI REMOVED
> >
> 16/builds/18474__;Iw!!JmoZiZGBv3RvKRSx!8a7C6K5aa0CE1QTOIygEPamFI5WL
> KVq
> > Q-REw0n6KCF54tqKP7df8KtY_duKszo3lif-L572623pQ5nsE68Mw6YhD$
> >
> > Thanks,
> > Matt
> >
> >> -Original Message-
> >> From: cfe-commits  On Behalf Of
> >> Craig Topper via cfe-commits
> >> Sent: Thursday, March 16, 2023 5:52 PM
> >> To: cfe-commits@lists.llvm.org
> >> Subject: [clang] 1029747 - [Sema] Fix null pointer dereference
> >> handleAlwaysInlineAttr.
> >>
> >>
> >> Author: Craig Topper
> >> Date: 2023-03-16T17:49:34-07:00
> >> New Revision: 10297470e953f4f3968c54c851c8af82b07af00b
> >>
> >> URL: INVALID URI REMOVED
> >>
> project/commit/10297470e953f4f3968c54c851c8af82b07af00b__;!!JmoZiZGBv
> >> 3RvKRSx!5T2bYoGu6_sqxRYd-ZFC--KklLgMVa3mP6dp-
> >> DaM426lCB_1eQvZAEGwE8Pe2BFekx30eRBU7KU4wdK2-3ln_WfRflk$
> >> DIFF: INVALID URI REMOVED
> >>
> project/commit/10297470e953f4f3968c54c851c8af82b07af00b.diff__;!!JmoZ
> >> iZ
> >> GBv3RvKRSx!5T2bYoGu6_sqxRYd-ZFC--KklLgMVa3mP6dp-
> >> DaM426lCB_1eQvZAEGwE8Pe2BFekx30eRBU7KU4wdK2-3lnHdJlBwk$
> >>
> >> LOG: [Sema] Fix null pointer dereference handleAlwaysInlineAttr.
> >>
> >> It's possible for `getCalleeDecl()` to return a null pointer.
> >>
> >> This was encountered by a user of our downstream compiler.
> >>
> >> The case involved a DependentScopeDeclRefExpr.
> >>
> >> Since this seems to only be for a warning diagnostic, I skipped the
> >> diagnostic check if it returned null. But mabye there's a different way to 
> >> fix
> this.
> >>
> >> Reviewed By: erichkeane
> >>
> >> Differential Revision:
> >> https://reviews.llvm.org/D146089
> >> iZGBv3RvKRSx!8a7C6K5aa0CE1QTOIygEPamFI5WLKVqQ-
> REw0n6KCF54tqKP7df8KtY_
> >> duKszo3lif-L572623pQ5nsE6xcR7geM$
> >> 3RvKRSx!5T2bYoGu6_sqxRYd-ZFC--KklLgMVa3mP6dp-
> >> DaM426lCB_1eQvZAEGwE8Pe2BFekx30eRBU7KU4wdK2-3lnaIroE7Q$
> >>
> >> Added:
> >>
> >>
> >> Modified:
> >>clang/lib/Sema/SemaStmtAttr.cpp
> >>clang/test/Sema/attr-alwaysinline.cpp
> >>clang/test/Sema/attr-noinline.cpp
> >>
> >> Removed:
> >>
> >>
> >>
> >>
> #
> >> ###
> >> diff  --git a/clang/lib/Sema/SemaStmtAttr.cpp
> >> b/clang/lib/Sema/SemaStmtAttr.cpp index 6d443837a4c5..eeef85373ccb
> >> 100644
> >> --- a/clang/lib/Sema/SemaStmtAttr.cpp
> >> +++ b/clang/lib/Sema/SemaStmtAttr.cpp
> >> @@ -233,7 +233,8 @@ static Attr *handleNoInlineAttr(Sema , Stmt
> >> *St, const ParsedAttr ,
> >>
> >>   for (const auto *CallExpr : CEF.getCallExprs()) {
> >> const Decl *Decl = CallExpr->getCalleeDecl();
> >> -if (Decl->hasAttr() || Decl->hasAttr())
> >> +if (Decl &&
> >> +(Decl->hasAttr() ||
> >> + Decl->hasAttr()))
> >>   S.Diag(St->getBeginLoc(),
> diag::warn_function_stmt_attribute_precedence)
> >>   << A << (Decl->hasAttr() ? 0 : 1);
> >>   }
> >> @@ -259,7 +260,7 @@ static Attr *handleAlwaysInlineAttr(Sema , Stmt
> >> *St, const ParsedAttr ,
> >>
> >>   for (const auto *CallExpr : CEF.getCallExprs()) {
> >> const Decl *Decl = CallExpr->getCalleeDecl();
> >> -if (Decl->hasAttr() || Decl->hasAttr())
> >> +if (Decl && (Decl->hasAttr() ||
> >> + Decl->hasAttr()))
> >>   S.Diag(St->getBeginLoc(),
> diag::warn_function_stmt_attribute_precedence)
> >>   << A << (Decl->hasAttr() ? 2 : 1);
> >>   }
> >>
> >> diff  --git a/clang/test/Sema/attr-alwaysinline.cpp
> >> b/clang/test/Sema/attr- alwaysinline.cpp index
> >> 6b8e8f215a4b..213d70407f48 100644
> >> --- a/clang/test/Sema/attr-alwaysinline.cpp
> >> +++ b/clang/test/Sema/attr-alwaysinline.cpp
> >> @@ -25,3 +25,22 @@ void foo() {
> >> }
> >>
> >> [[clang::always_inline]] static int i = bar(); // expected-warning
> {{'always_inline'
> >> attribute only applies to functions and statements}}
> >> +
> >> +// This used to crash the compiler.
> >> +template
> >> +int foo(int x) {
> >> +if constexpr (D > 1)
> >> +[[clang::always_inline]] return foo(x + 1);
> >> +else
> >> +return x;
> >> +}
> >> +
> >> +// FIXME: This should warn that always_inline statement attribute
> >> +has higher // precedence than the noinline function attribute.
> >> +template [[gnu::noinline]]
> >> +int bar(int x) {
> >> +if constexpr (D > 1)
> >> +[[clang::always_inline]] return bar(x + 1);
> >> +else
> >> +return x;
> >> +}
> >>
> >> diff  --git 

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

I don't remember clearly if we have a test for the initializer in the 
implementation unit. Ignore this if we already have one.




Comment at: clang/include/clang/Basic/Module.h:137
 ImplicitGlobalModuleFragment,
   };
 

We may be able to save 1 bit for ModuleKind by adding two field bits to Module: 
`IsImplementation` and `IsPartition`. Then we can remove 
`ModulePartitionInterface` and `ModulePartitionImplementation`. Then let's 
rename `ModuleInterfaceUnit` to `NamedModuleUnit`.
So we can judge module interface unit, implementation unit,  module partition 
interface and module partition implementation by `NamedModuleUnit ` and the two 
bits.



Comment at: clang/lib/Sema/SemaDecl.cpp:1672-1677
+  // A module implementation unit has visibility of the decls in its implicitly
+  // imported interface.
+  if (getLangOpts().CPlusPlusModules && NewM && OldM &&
+  NewM->Kind == Module::ModuleImplementationUnit &&
+  OldM->Kind == Module::ModuleInterfaceUnit && NewM->Name == OldM->Name)
+return false;

I feel slightly better to add a field in Sema:

```
Module *PrimaryModuleInterface = nullptr; // Only valid if we're parsing a 
module unit.
```

Then we can avoid the string compare here.  Also we can add it to 
`Sema::isUsableModule`. Now `Sema::isUsableModule` works because it falls to 
the string comparison.



Comment at: clang/lib/Sema/SemaModule.cpp:302
+  Module *Mod; // The module we are creating.
+  Module *Interface = nullptr; // The interface for an implementation.
   switch (MDK) {

Then we can avoid to declare this if we have `Sema::PrimaryModuleInterface `



Comment at: clang/lib/Sema/SemaModule.cpp:408-409
+// module, if any.
+if (!ModuleScopes.empty())
+  Context.addModuleInitializer(ModuleScopes.back().Module, Import);
+

ModuleScopes can't be empty here.



Comment at: clang/test/CXX/module/basic/basic.def.odr/p4.cppm:155-156
+
+  // FIXME: Issue #61427 Internal-linkage declarations in the interface TU
+  // should not be not visible here.
   (void)_var_module_linkage; // FIXME: Should not be visible here.

nit: I am a little bit tired to add FIXME in the tests... let's not try to add 
more...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126959

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

[clang] e8a632d - Revert "[Sema] Fix null pointer dereference handleAlwaysInlineAttr."

[Craig Topper via cfe-commits]

Author: Craig Topper
Date: 2023-03-16T20:05:33-07:00
New Revision: e8a632dac2204d66293dd5e0ba92eb65941c4aa3

URL: 
https://github.com/llvm/llvm-project/commit/e8a632dac2204d66293dd5e0ba92eb65941c4aa3
DIFF: 
https://github.com/llvm/llvm-project/commit/e8a632dac2204d66293dd5e0ba92eb65941c4aa3.diff

LOG: Revert "[Sema] Fix null pointer dereference handleAlwaysInlineAttr."

This reverts commit 10297470e953f4f3968c54c851c8af82b07af00b.

This is failing on a build bot.

Added: 


Modified: 
clang/lib/Sema/SemaStmtAttr.cpp
clang/test/Sema/attr-alwaysinline.cpp
clang/test/Sema/attr-noinline.cpp

Removed: 




diff  --git a/clang/lib/Sema/SemaStmtAttr.cpp b/clang/lib/Sema/SemaStmtAttr.cpp
index eeef85373ccb1..6d443837a4c57 100644
--- a/clang/lib/Sema/SemaStmtAttr.cpp
+++ b/clang/lib/Sema/SemaStmtAttr.cpp
@@ -233,8 +233,7 @@ static Attr *handleNoInlineAttr(Sema , Stmt *St, const 
ParsedAttr ,
 
   for (const auto *CallExpr : CEF.getCallExprs()) {
 const Decl *Decl = CallExpr->getCalleeDecl();
-if (Decl &&
-(Decl->hasAttr() || Decl->hasAttr()))
+if (Decl->hasAttr() || Decl->hasAttr())
   S.Diag(St->getBeginLoc(), diag::warn_function_stmt_attribute_precedence)
   << A << (Decl->hasAttr() ? 0 : 1);
   }
@@ -260,7 +259,7 @@ static Attr *handleAlwaysInlineAttr(Sema , Stmt *St, 
const ParsedAttr ,
 
   for (const auto *CallExpr : CEF.getCallExprs()) {
 const Decl *Decl = CallExpr->getCalleeDecl();
-if (Decl && (Decl->hasAttr() || 
Decl->hasAttr()))
+if (Decl->hasAttr() || Decl->hasAttr())
   S.Diag(St->getBeginLoc(), diag::warn_function_stmt_attribute_precedence)
   << A << (Decl->hasAttr() ? 2 : 1);
   }

diff  --git a/clang/test/Sema/attr-alwaysinline.cpp 
b/clang/test/Sema/attr-alwaysinline.cpp
index 213d70407f485..6b8e8f215a4b6 100644
--- a/clang/test/Sema/attr-alwaysinline.cpp
+++ b/clang/test/Sema/attr-alwaysinline.cpp
@@ -25,22 +25,3 @@ void foo() {
 }
 
 [[clang::always_inline]] static int i = bar(); // expected-warning 
{{'always_inline' attribute only applies to functions and statements}}
-
-// This used to crash the compiler.
-template
-int foo(int x) {
-if constexpr (D > 1)
-[[clang::always_inline]] return foo(x + 1);
-else
-return x;
-}
-
-// FIXME: This should warn that always_inline statement attribute has higher
-// precedence than the noinline function attribute.
-template [[gnu::noinline]]
-int bar(int x) {
-if constexpr (D > 1)
-[[clang::always_inline]] return bar(x + 1);
-else
-return x;
-}

diff  --git a/clang/test/Sema/attr-noinline.cpp 
b/clang/test/Sema/attr-noinline.cpp
index a62ca1debcc5a..d35782f11adbb 100644
--- a/clang/test/Sema/attr-noinline.cpp
+++ b/clang/test/Sema/attr-noinline.cpp
@@ -25,22 +25,3 @@ void foo() {
 }
 
 [[clang::noinline]] static int i = bar(); // expected-warning {{'noinline' 
attribute only applies to functions and statements}}
-
-// This used to crash the compiler.
-template
-int foo(int x) {
-if constexpr (D > 1)
-[[clang::noinline]] return foo(x + 1);
-else
-return x;
-}
-
-// FIXME: This should warn that noinline statement attribute has higher
-// precedence than the always_inline function attribute.
-template [[clang::always_inline]]
-int bar(int x) {
-if constexpr (D > 1)
-[[clang::noinline]] return bar(x + 1);
-else
-return x;
-}



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

RE: [clang] 1029747 - [Sema] Fix null pointer dereference handleAlwaysInlineAttr.

[Voss, Matthew via cfe-commits]

Hi Craig,

There are some failures on the PS5 buildbot after this change. Could you take a 
look?

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

Thanks,
Matt 

> -Original Message-
> From: cfe-commits  On Behalf Of Craig
> Topper via cfe-commits
> Sent: Thursday, March 16, 2023 5:52 PM
> To: cfe-commits@lists.llvm.org
> Subject: [clang] 1029747 - [Sema] Fix null pointer dereference
> handleAlwaysInlineAttr.
> 
> 
> Author: Craig Topper
> Date: 2023-03-16T17:49:34-07:00
> New Revision: 10297470e953f4f3968c54c851c8af82b07af00b
> 
> URL: INVALID URI REMOVED
> project/commit/10297470e953f4f3968c54c851c8af82b07af00b__;!!JmoZiZGBv
> 3RvKRSx!5T2bYoGu6_sqxRYd-ZFC--KklLgMVa3mP6dp-
> DaM426lCB_1eQvZAEGwE8Pe2BFekx30eRBU7KU4wdK2-3ln_WfRflk$
> DIFF: INVALID URI REMOVED
> project/commit/10297470e953f4f3968c54c851c8af82b07af00b.diff__;!!JmoZiZ
> GBv3RvKRSx!5T2bYoGu6_sqxRYd-ZFC--KklLgMVa3mP6dp-
> DaM426lCB_1eQvZAEGwE8Pe2BFekx30eRBU7KU4wdK2-3lnHdJlBwk$
> 
> LOG: [Sema] Fix null pointer dereference handleAlwaysInlineAttr.
> 
> It's possible for `getCalleeDecl()` to return a null pointer.
> 
> This was encountered by a user of our downstream compiler.
> 
> The case involved a DependentScopeDeclRefExpr.
> 
> Since this seems to only be for a warning diagnostic, I skipped the diagnostic
> check if it returned null. But mabye there's a different way to fix this.
> 
> Reviewed By: erichkeane
> 
> Differential Revision:
> https://reviews.llvm.org/D146089
> 3RvKRSx!5T2bYoGu6_sqxRYd-ZFC--KklLgMVa3mP6dp-
> DaM426lCB_1eQvZAEGwE8Pe2BFekx30eRBU7KU4wdK2-3lnaIroE7Q$
> 
> Added:
> 
> 
> Modified:
> clang/lib/Sema/SemaStmtAttr.cpp
> clang/test/Sema/attr-alwaysinline.cpp
> clang/test/Sema/attr-noinline.cpp
> 
> Removed:
> 
> 
> 
> #
> ###
> diff  --git a/clang/lib/Sema/SemaStmtAttr.cpp
> b/clang/lib/Sema/SemaStmtAttr.cpp index 6d443837a4c5..eeef85373ccb
> 100644
> --- a/clang/lib/Sema/SemaStmtAttr.cpp
> +++ b/clang/lib/Sema/SemaStmtAttr.cpp
> @@ -233,7 +233,8 @@ static Attr *handleNoInlineAttr(Sema , Stmt *St,
> const ParsedAttr ,
> 
>for (const auto *CallExpr : CEF.getCallExprs()) {
>  const Decl *Decl = CallExpr->getCalleeDecl();
> -if (Decl->hasAttr() || Decl->hasAttr())
> +if (Decl &&
> +(Decl->hasAttr() ||
> + Decl->hasAttr()))
>S.Diag(St->getBeginLoc(), 
> diag::warn_function_stmt_attribute_precedence)
><< A << (Decl->hasAttr() ? 0 : 1);
>}
> @@ -259,7 +260,7 @@ static Attr *handleAlwaysInlineAttr(Sema , Stmt *St,
> const ParsedAttr ,
> 
>for (const auto *CallExpr : CEF.getCallExprs()) {
>  const Decl *Decl = CallExpr->getCalleeDecl();
> -if (Decl->hasAttr() || Decl->hasAttr())
> +if (Decl && (Decl->hasAttr() ||
> + Decl->hasAttr()))
>S.Diag(St->getBeginLoc(), 
> diag::warn_function_stmt_attribute_precedence)
><< A << (Decl->hasAttr() ? 2 : 1);
>}
> 
> diff  --git a/clang/test/Sema/attr-alwaysinline.cpp b/clang/test/Sema/attr-
> alwaysinline.cpp
> index 6b8e8f215a4b..213d70407f48 100644
> --- a/clang/test/Sema/attr-alwaysinline.cpp
> +++ b/clang/test/Sema/attr-alwaysinline.cpp
> @@ -25,3 +25,22 @@ void foo() {
>  }
> 
>  [[clang::always_inline]] static int i = bar(); // expected-warning 
> {{'always_inline'
> attribute only applies to functions and statements}}
> +
> +// This used to crash the compiler.
> +template
> +int foo(int x) {
> +if constexpr (D > 1)
> +[[clang::always_inline]] return foo(x + 1);
> +else
> +return x;
> +}
> +
> +// FIXME: This should warn that always_inline statement attribute has
> +higher // precedence than the noinline function attribute.
> +template [[gnu::noinline]]
> +int bar(int x) {
> +if constexpr (D > 1)
> +[[clang::always_inline]] return bar(x + 1);
> +else
> +return x;
> +}
> 
> diff  --git a/clang/test/Sema/attr-noinline.cpp b/clang/test/Sema/attr-
> noinline.cpp
> index d35782f11adb..a62ca1debcc5 100644
> --- a/clang/test/Sema/attr-noinline.cpp
> +++ b/clang/test/Sema/attr-noinline.cpp
> @@ -25,3 +25,22 @@ void foo() {
>  }
> 
>  [[clang::noinline]] static int i = bar(); // expected-warning {{'noinline' 
> attribute
> only applies to functions and statements}}
> +
> +// This used to crash the compiler.
> +template
> +int foo(int x) {
> +if constexpr (D > 1)
> +[[clang::noinline]] return foo(x + 1);
> +else
> +return x;
> +}
> +
> +// FIXME: This should warn that noinline statement attribute has higher
> +// precedence than the always_inline function attribute.
> +template [[clang::always_inline]] int bar(int x) {
> +if constexpr (D > 1)
> +[[clang::noinline]] return bar(x + 1);
> +else
> +return x;
> +}
> 
> 
> 
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> INVALID URI REMOVED
> commits__;!!JmoZiZGBv3RvKRSx!5T2bYoGu6_sqxRYd-ZFC--KklLgMVa3mP6dp-
> 

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

(I don't why I can't send emails in the original mail address. It told me that 
your email addressed is not recorded by MX, while I don't know what is MX. So I 
tried to reply your email here)

This FIXME was added 6 years ago for Modules TS. 
(https://github.com/llvm/llvm-project/commit/becb92dec85924969ac0c3b049e0a74def431453).
And I moved the test simply without modifying it actually.

And I guess it may refer to the problem we are discussing. The spec says the 
implementation unit will import the primary module interface implicitly.
But the current implementation doesn't **import** it actually but loading it. 
This is inconsistent with the wording and it is not consistent with the design 
intention in clang.
It resulted some problems we saw.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126959

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

[PATCH] D146269: MIPS: allow o32 abi with 64bit CPU and 64 abi with 32bit triple

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

Tests?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146269

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

[PATCH] D146269: MIPS: allow o32 abi with 64bit CPU

[YunQiang Su via Phabricator via cfe-commits]

wzssyqa created this revision.
wzssyqa added a reviewer: MaskRay.
Herald added subscribers: atanasyan, jrtc27, arichardson, sdardis.
Herald added a project: All.
wzssyqa requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

In general, MIPS support ELF format like

  ELF 32-bit LSB relocatable, MIPS, MIPS64 rel2 version 1 (SYSV)

and Linux's VDSO uses it.

Currently clang stop CMDs like

  clang -march=mips64r2 -mabi=32

While it is not needed now, since the the backend support the combination now.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146269

Files:
  clang/lib/Basic/Targets/Mips.cpp


Index: clang/lib/Basic/Targets/Mips.cpp
===
--- clang/lib/Basic/Targets/Mips.cpp
+++ clang/lib/Basic/Targets/Mips.cpp
@@ -238,12 +238,6 @@
 Diags.Report(diag::err_target_unsupported_cpu_for_micromips) << CPU;
 return false;
   }
-  // FIXME: It's valid to use O32 on a 64-bit CPU but the backend can't handle
-  //this yet. It's better to fail here than on the backend assertion.
-  if (processorSupportsGPR64() && ABI == "o32") {
-Diags.Report(diag::err_target_unsupported_abi) << ABI << CPU;
-return false;
-  }
 
   // 64-bit ABI's require 64-bit CPU's.
   if (!processorSupportsGPR64() && (ABI == "n32" || ABI == "n64")) {
@@ -251,24 +245,6 @@
 return false;
   }
 
-  // FIXME: It's valid to use O32 on a mips64/mips64el triple but the backend
-  //can't handle this yet. It's better to fail here than on the
-  //backend assertion.
-  if (getTriple().isMIPS64() && ABI == "o32") {
-Diags.Report(diag::err_target_unsupported_abi_for_triple)
-<< ABI << getTriple().str();
-return false;
-  }
-
-  // FIXME: It's valid to use N32/N64 on a mips/mipsel triple but the backend
-  //can't handle this yet. It's better to fail here than on the
-  //backend assertion.
-  if (getTriple().isMIPS32() && (ABI == "n32" || ABI == "n64")) {
-Diags.Report(diag::err_target_unsupported_abi_for_triple)
-<< ABI << getTriple().str();
-return false;
-  }
-
   // -fpxx is valid only for the o32 ABI
   if (FPMode == FPXX && (ABI == "n32" || ABI == "n64")) {
 Diags.Report(diag::err_unsupported_abi_for_opt) << "-mfpxx" << "o32";


Index: clang/lib/Basic/Targets/Mips.cpp
===
--- clang/lib/Basic/Targets/Mips.cpp
+++ clang/lib/Basic/Targets/Mips.cpp
@@ -238,12 +238,6 @@
 Diags.Report(diag::err_target_unsupported_cpu_for_micromips) << CPU;
 return false;
   }
-  // FIXME: It's valid to use O32 on a 64-bit CPU but the backend can't handle
-  //this yet. It's better to fail here than on the backend assertion.
-  if (processorSupportsGPR64() && ABI == "o32") {
-Diags.Report(diag::err_target_unsupported_abi) << ABI << CPU;
-return false;
-  }
 
   // 64-bit ABI's require 64-bit CPU's.
   if (!processorSupportsGPR64() && (ABI == "n32" || ABI == "n64")) {
@@ -251,24 +245,6 @@
 return false;
   }
 
-  // FIXME: It's valid to use O32 on a mips64/mips64el triple but the backend
-  //can't handle this yet. It's better to fail here than on the
-  //backend assertion.
-  if (getTriple().isMIPS64() && ABI == "o32") {
-Diags.Report(diag::err_target_unsupported_abi_for_triple)
-<< ABI << getTriple().str();
-return false;
-  }
-
-  // FIXME: It's valid to use N32/N64 on a mips/mipsel triple but the backend
-  //can't handle this yet. It's better to fail here than on the
-  //backend assertion.
-  if (getTriple().isMIPS32() && (ABI == "n32" || ABI == "n64")) {
-Diags.Report(diag::err_target_unsupported_abi_for_triple)
-<< ABI << getTriple().str();
-return false;
-  }
-
   // -fpxx is valid only for the o32 ABI
   if (FPMode == FPXX && (ABI == "n32" || ABI == "n64")) {
 Diags.Report(diag::err_unsupported_abi_for_opt) << "-mfpxx" << "o32";
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146089: [Sema] Fix null pointer dereference handleAlwaysInlineAttr.

[Craig Topper via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG10297470e953: [Sema] Fix null pointer dereference 
handleAlwaysInlineAttr. (authored by craig.topper).

Changed prior to commit:
  https://reviews.llvm.org/D146089?vs=505596=505961#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146089

Files:
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/Sema/attr-alwaysinline.cpp
  clang/test/Sema/attr-noinline.cpp


Index: clang/test/Sema/attr-noinline.cpp
===
--- clang/test/Sema/attr-noinline.cpp
+++ clang/test/Sema/attr-noinline.cpp
@@ -25,3 +25,22 @@
 }
 
 [[clang::noinline]] static int i = bar(); // expected-warning {{'noinline' 
attribute only applies to functions and statements}}
+
+// This used to crash the compiler.
+template
+int foo(int x) {
+if constexpr (D > 1)
+[[clang::noinline]] return foo(x + 1);
+else
+return x;
+}
+
+// FIXME: This should warn that noinline statement attribute has higher
+// precedence than the always_inline function attribute.
+template [[clang::always_inline]]
+int bar(int x) {
+if constexpr (D > 1)
+[[clang::noinline]] return bar(x + 1);
+else
+return x;
+}
Index: clang/test/Sema/attr-alwaysinline.cpp
===
--- clang/test/Sema/attr-alwaysinline.cpp
+++ clang/test/Sema/attr-alwaysinline.cpp
@@ -25,3 +25,22 @@
 }
 
 [[clang::always_inline]] static int i = bar(); // expected-warning 
{{'always_inline' attribute only applies to functions and statements}}
+
+// This used to crash the compiler.
+template
+int foo(int x) {
+if constexpr (D > 1)
+[[clang::always_inline]] return foo(x + 1);
+else
+return x;
+}
+
+// FIXME: This should warn that always_inline statement attribute has higher
+// precedence than the noinline function attribute.
+template [[gnu::noinline]]
+int bar(int x) {
+if constexpr (D > 1)
+[[clang::always_inline]] return bar(x + 1);
+else
+return x;
+}
Index: clang/lib/Sema/SemaStmtAttr.cpp
===
--- clang/lib/Sema/SemaStmtAttr.cpp
+++ clang/lib/Sema/SemaStmtAttr.cpp
@@ -233,7 +233,8 @@
 
   for (const auto *CallExpr : CEF.getCallExprs()) {
 const Decl *Decl = CallExpr->getCalleeDecl();
-if (Decl->hasAttr() || Decl->hasAttr())
+if (Decl &&
+(Decl->hasAttr() || Decl->hasAttr()))
   S.Diag(St->getBeginLoc(), diag::warn_function_stmt_attribute_precedence)
   << A << (Decl->hasAttr() ? 0 : 1);
   }
@@ -259,7 +260,7 @@
 
   for (const auto *CallExpr : CEF.getCallExprs()) {
 const Decl *Decl = CallExpr->getCalleeDecl();
-if (Decl->hasAttr() || Decl->hasAttr())
+if (Decl && (Decl->hasAttr() || 
Decl->hasAttr()))
   S.Diag(St->getBeginLoc(), diag::warn_function_stmt_attribute_precedence)
   << A << (Decl->hasAttr() ? 2 : 1);
   }


Index: clang/test/Sema/attr-noinline.cpp
===
--- clang/test/Sema/attr-noinline.cpp
+++ clang/test/Sema/attr-noinline.cpp
@@ -25,3 +25,22 @@
 }
 
 [[clang::noinline]] static int i = bar(); // expected-warning {{'noinline' attribute only applies to functions and statements}}
+
+// This used to crash the compiler.
+template
+int foo(int x) {
+if constexpr (D > 1)
+[[clang::noinline]] return foo(x + 1);
+else
+return x;
+}
+
+// FIXME: This should warn that noinline statement attribute has higher
+// precedence than the always_inline function attribute.
+template [[clang::always_inline]]
+int bar(int x) {
+if constexpr (D > 1)
+[[clang::noinline]] return bar(x + 1);
+else
+return x;
+}
Index: clang/test/Sema/attr-alwaysinline.cpp
===
--- clang/test/Sema/attr-alwaysinline.cpp
+++ clang/test/Sema/attr-alwaysinline.cpp
@@ -25,3 +25,22 @@
 }
 
 [[clang::always_inline]] static int i = bar(); // expected-warning {{'always_inline' attribute only applies to functions and statements}}
+
+// This used to crash the compiler.
+template
+int foo(int x) {
+if constexpr (D > 1)
+[[clang::always_inline]] return foo(x + 1);
+else
+return x;
+}
+
+// FIXME: This should warn that always_inline statement attribute has higher
+// precedence than the noinline function attribute.
+template [[gnu::noinline]]
+int bar(int x) {
+if constexpr (D > 1)
+[[clang::always_inline]] return bar(x + 1);
+else
+return x;
+}
Index: clang/lib/Sema/SemaStmtAttr.cpp
===
--- clang/lib/Sema/SemaStmtAttr.cpp
+++ clang/lib/Sema/SemaStmtAttr.cpp
@@ -233,7 +233,8 @@
 
   for (const auto *CallExpr : CEF.getCallExprs()) {
 const Decl *Decl = 

[clang] 1029747 - [Sema] Fix null pointer dereference handleAlwaysInlineAttr.

[Craig Topper via cfe-commits]

Author: Craig Topper
Date: 2023-03-16T17:49:34-07:00
New Revision: 10297470e953f4f3968c54c851c8af82b07af00b

URL: 
https://github.com/llvm/llvm-project/commit/10297470e953f4f3968c54c851c8af82b07af00b
DIFF: 
https://github.com/llvm/llvm-project/commit/10297470e953f4f3968c54c851c8af82b07af00b.diff

LOG: [Sema] Fix null pointer dereference handleAlwaysInlineAttr.

It's possible for `getCalleeDecl()` to return a null pointer.

This was encountered by a user of our downstream compiler.

The case involved a DependentScopeDeclRefExpr.

Since this seems to only be for a warning diagnostic, I skipped
the diagnostic check if it returned null. But mabye there's a
different way to fix this.

Reviewed By: erichkeane

Differential Revision: https://reviews.llvm.org/D146089

Added: 


Modified: 
clang/lib/Sema/SemaStmtAttr.cpp
clang/test/Sema/attr-alwaysinline.cpp
clang/test/Sema/attr-noinline.cpp

Removed: 




diff  --git a/clang/lib/Sema/SemaStmtAttr.cpp b/clang/lib/Sema/SemaStmtAttr.cpp
index 6d443837a4c5..eeef85373ccb 100644
--- a/clang/lib/Sema/SemaStmtAttr.cpp
+++ b/clang/lib/Sema/SemaStmtAttr.cpp
@@ -233,7 +233,8 @@ static Attr *handleNoInlineAttr(Sema , Stmt *St, const 
ParsedAttr ,
 
   for (const auto *CallExpr : CEF.getCallExprs()) {
 const Decl *Decl = CallExpr->getCalleeDecl();
-if (Decl->hasAttr() || Decl->hasAttr())
+if (Decl &&
+(Decl->hasAttr() || Decl->hasAttr()))
   S.Diag(St->getBeginLoc(), diag::warn_function_stmt_attribute_precedence)
   << A << (Decl->hasAttr() ? 0 : 1);
   }
@@ -259,7 +260,7 @@ static Attr *handleAlwaysInlineAttr(Sema , Stmt *St, 
const ParsedAttr ,
 
   for (const auto *CallExpr : CEF.getCallExprs()) {
 const Decl *Decl = CallExpr->getCalleeDecl();
-if (Decl->hasAttr() || Decl->hasAttr())
+if (Decl && (Decl->hasAttr() || 
Decl->hasAttr()))
   S.Diag(St->getBeginLoc(), diag::warn_function_stmt_attribute_precedence)
   << A << (Decl->hasAttr() ? 2 : 1);
   }

diff  --git a/clang/test/Sema/attr-alwaysinline.cpp 
b/clang/test/Sema/attr-alwaysinline.cpp
index 6b8e8f215a4b..213d70407f48 100644
--- a/clang/test/Sema/attr-alwaysinline.cpp
+++ b/clang/test/Sema/attr-alwaysinline.cpp
@@ -25,3 +25,22 @@ void foo() {
 }
 
 [[clang::always_inline]] static int i = bar(); // expected-warning 
{{'always_inline' attribute only applies to functions and statements}}
+
+// This used to crash the compiler.
+template
+int foo(int x) {
+if constexpr (D > 1)
+[[clang::always_inline]] return foo(x + 1);
+else
+return x;
+}
+
+// FIXME: This should warn that always_inline statement attribute has higher
+// precedence than the noinline function attribute.
+template [[gnu::noinline]]
+int bar(int x) {
+if constexpr (D > 1)
+[[clang::always_inline]] return bar(x + 1);
+else
+return x;
+}

diff  --git a/clang/test/Sema/attr-noinline.cpp 
b/clang/test/Sema/attr-noinline.cpp
index d35782f11adb..a62ca1debcc5 100644
--- a/clang/test/Sema/attr-noinline.cpp
+++ b/clang/test/Sema/attr-noinline.cpp
@@ -25,3 +25,22 @@ void foo() {
 }
 
 [[clang::noinline]] static int i = bar(); // expected-warning {{'noinline' 
attribute only applies to functions and statements}}
+
+// This used to crash the compiler.
+template
+int foo(int x) {
+if constexpr (D > 1)
+[[clang::noinline]] return foo(x + 1);
+else
+return x;
+}
+
+// FIXME: This should warn that noinline statement attribute has higher
+// precedence than the always_inline function attribute.
+template [[clang::always_inline]]
+int bar(int x) {
+if constexpr (D > 1)
+[[clang::noinline]] return bar(x + 1);
+else
+return x;
+}



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

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

[Andrew Gozillon via Phabricator via cfe-commits]

agozillon added a comment.

Recent commit added some more detail to the comment on the 
fembed-offload-object argument as requested by @awarzynski


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145815

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

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

[Andrew Gozillon via Phabricator via cfe-commits]

agozillon updated this revision to Diff 505957.
agozillon edited the summary of this revision.
agozillon added a comment.

- [Flang][Driver] Expand further on the embed-offload-object comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145815

Files:
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Driver/ToolChains/Flang.h
  flang/test/Driver/omp-frontend-forwarding.f90


Index: flang/test/Driver/omp-frontend-forwarding.f90
===
--- /dev/null
+++ flang/test/Driver/omp-frontend-forwarding.f90
@@ -0,0 +1,22 @@
+! Test that flang-new OpenMP and OpenMP offload related 
+! commands forward or expand to the appropriate commands 
+! for flang-new -fc1 as expected.
+
+! Test regular -fopenmp with no offload
+! RUN: %flang -### -fopenmp %s 2>&1 | FileCheck --check-prefixes=CHECK-OPENMP 
%s
+! CHECK-OPENMP: "{{[^"]*}}flang-new" "-fc1" {{.*}} "-fopenmp" {{.*}}.f90"
+! CHECK-OPENMP-NOT: "{{[^"]*}}flang-new" "-fc1" {{.*}} "-fopenmp" {{.*}} 
"-fopenmp-is-device" {{.*}}.f90"
+
+! Test regular -fopenmp with offload for basic fopenmp-is-device flag addition 
and correct fopenmp 
+! RUN: %flang -### -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa %s 2>&1 | 
FileCheck --check-prefixes=CHECK-OPENMP-IS-DEVICE %s
+! CHECK-OPENMP-IS-DEVICE: "{{[^"]*}}flang-new" "-fc1" {{.*}} "-fopenmp" {{.*}} 
"-fopenmp-is-device" {{.*}}.f90"
+
+! Testing fembed-offload-object and fopenmp-is-device
+! RUN: %flang -S -### %s -o %t 2>&1 \
+! RUN: -fopenmp --offload-arch=gfx90a \
+! RUN: --target=aarch64-unknown-linux-gnu \
+! RUN:   | FileCheck %s --check-prefixes=CHECK-OPENMP-EMBED
+! CHECK-OPENMP-EMBED: "{{[^"]*}}flang-new" "-fc1" "-triple" 
"aarch64-unknown-linux-gnu" {{.*}} "-fopenmp" {{.*}}.f90"
+! CHECK-OPENMP-EMBED-NEXT: "{{[^"]*}}flang-new" "-fc1" "-triple" 
"amdgcn-amd-amdhsa" {{.*}} "-fopenmp" {{.*}} "-fopenmp-is-device" {{.*}}.f90"
+! CHECK-OPENMP-EMBED: "{{[^"]*}}clang-offload-packager" {{.*}} 
"--image=file={{.*}}.bc,triple=amdgcn-amd-amdhsa,arch=gfx90a,kind=openmp"
+! CHECK-OPENMP-EMBED-NEXT: "{{[^"]*}}flang-new" "-fc1" "-triple" 
"aarch64-unknown-linux-gnu" {{.*}} "-fopenmp" {{.*}} 
"-fembed-offload-object={{.*}}.out" {{.*}}.bc"
Index: clang/lib/Driver/ToolChains/Flang.h
===
--- clang/lib/Driver/ToolChains/Flang.h
+++ clang/lib/Driver/ToolChains/Flang.h
@@ -56,6 +56,17 @@
   void addTargetOptions(const llvm::opt::ArgList ,
 llvm::opt::ArgStringList ) const;
 
+  /// Extract offload options from the driver arguments and add them to
+  /// the command arguments.
+  /// \param [in] C The current compilation for the driver invocation
+  /// \param [in] Inputs The input infomration on the current file inputs
+  /// \param [in] JA The job action
+  /// \param [in] Args The list of input driver arguments
+  /// \param [out] CmdArgs The list of output command arguments
+  void addOffloadOptions(Compilation , const InputInfoList ,
+ const JobAction , const llvm::opt::ArgList ,
+ llvm::opt::ArgStringList ) const;
+
   /// Extract other compilation options from the driver arguments and add them
   /// to the command arguments.
   ///
Index: clang/lib/Driver/ToolChains/Flang.cpp
===
--- clang/lib/Driver/ToolChains/Flang.cpp
+++ clang/lib/Driver/ToolChains/Flang.cpp
@@ -114,6 +114,35 @@
   // TODO: Add target specific flags, ABI, mtune option etc.
 }
 
+void Flang::addOffloadOptions(Compilation , const InputInfoList ,
+  const JobAction , const ArgList ,
+  ArgStringList ) const {
+  bool IsOpenMPDevice = JA.isDeviceOffloading(Action::OFK_OpenMP);
+  bool IsHostOffloadingAction = JA.isHostOffloading(Action::OFK_OpenMP) ||
+JA.isHostOffloading(C.getActiveOffloadKinds());
+
+  // Skips the primary input file, which is the input file that the compilation
+  // proccess will be executed upon (e.g. the host bitcode file) and
+  // adds the other secondary input (e.g. device bitcode files for embedding)
+  // to the embed offload object. This is condensed logic from the Clang driver
+  // for embedding offload objects during HostOffloading.
+  if (IsHostOffloadingAction) {
+for (size_t i = 1; i < Inputs.size(); ++i) {
+  if (Inputs[i].getType() != types::TY_Nothing)
+CmdArgs.push_back(
+Args.MakeArgString("-fembed-offload-object=" +
+   getToolChain().getInputFilename(Inputs[i])));
+}
+  }
+
+  if (IsOpenMPDevice) {
+// -fopenmp-is-device is passed along to tell the frontend that it is
+// generating code for a device, so that only the relevant code is
+// emitted.
+CmdArgs.push_back("-fopenmp-is-device");
+  }
+}
+
 static void 

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

[Andrew Gozillon via Phabricator via cfe-commits]

agozillon added inline comments.



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:128
+  if (IsHostOffloadingAction) {
+for (size_t i = 1; i < Inputs.size(); ++i) {
+  if (Inputs[i].getType() != types::TY_Nothing)

awarzynski wrote:
> jhuber6 wrote:
> > agozillon wrote:
> > > jhuber6 wrote:
> > > > agozillon wrote:
> > > > > awarzynski wrote:
> > > > > > agozillon wrote:
> > > > > > > awarzynski wrote:
> > > > > > > > What's the magic "1"? And given that the input count matters 
> > > > > > > > here - is there a test with multiple inputs?
> > > > > > > It aims to mimic the behavior of Clang: 
> > > > > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L4561
> > > > > > >  where the main input is skipped (the input currently being 
> > > > > > > compiled or embedded into etc.), when adding to 
> > > > > > > //-fembed-offload-object//. 
> > > > > > > 
> > > > > > > It does look different to Clang's as Clang has more cases and the 
> > > > > > > logic is spread across the constructJob invocation, but the first 
> > > > > > > if case is what the if statement inside of the loop and setting 
> > > > > > > the loop index variable to 1 do. The HostOffloadingInputs array 
> > > > > > > is what is being generated here, except we're skipping and 
> > > > > > > directly applying it as arguments.
> > > > > > > 
> > > > > > > I tried to condense it a little in this case! Perhaps it loses 
> > > > > > > readability though, I had hoped the comment might have kept it 
> > > > > > > clear
> > > > > > Thanks for the link - that code in Clang doesn't really clarify 
> > > > > > what makes `Inputs[0]` special 樂 . 
> > > > > > 
> > > > > > Let me rephrase my question - what's so special about the first 
> > > > > > input? (referred to in Clang as "main input") Is that something 
> > > > > > specific to OpenMP? For example, in this case:
> > > > > > ```
> > > > > > flang-new  -fopenmp  file.f90
> > > > > > ```
> > > > > > I assume that `inputs[0]` is "file.f90", so nothing will happen?
> > > > > > 
> > > > > > > I tried to condense it a little in this case! Perhaps it loses 
> > > > > > > readability though, I had hoped the comment might have kept it 
> > > > > > > clear
> > > > > > 
> > > > > > Nah, I think that your implementation is fine. It's my ignorance 
> > > > > > with respect to OpenMP that's the problem here ;-)
> > > > > It's not specific to OpenMP I believe, as far as I am aware Clang's 
> > > > > supported offload models (SYCL and CUDA as well as OpenMP) use it! In 
> > > > > Flang's case we only really care about OpenMP as I believe it's the 
> > > > > only offload programming model supported.
> > > > > 
> > > > > In the case of the command: 
> > > > > 
> > > > > ```
> > > > > flang-new -fopenmp file.f90
> > > > > ``` 
> > > > > The code should never be executed as no part of the command will 
> > > > > enable an offloading action (for device or host)! But yes inputs[0] 
> > > > > would indeed refer to file.f90.
> > > > > 
> > > > > However, this code becomes relevant when you utilise an option that 
> > > > > enables the clangDriver to perform some form of offloading action. 
> > > > > For example a command like: 
> > > > > 
> > > > > ```
> > > > > flang-new -fopenmp --offload-arch=gfx90a file.f90 
> > > > > ```
> > > > > Will trigger two phase compilation, one for the host device (your 
> > > > > resident CPU in this command) and one for the device (gfx90a in this 
> > > > > command), the regular host pass will invoke like your provided 
> > > > > command and the device pass will then invoke with -fopenmp-is-device 
> > > > > in addition alongside the device triple. This generates two bitcode 
> > > > > files from the one file, one containing the host code from the file, 
> > > > > the other the device code (extracted from OpenMP target regions or 
> > > > > declare target etc.). 
> > > > > 
> > > > > However, now we have two files, with both parts of our program, we 
> > > > > need to conjoin them together, the clangDriver generates an 
> > > > > embeddable fat-binary/binary using the clang-offload-packager and 
> > > > > then invokes flang-new again, and this is where the above code 
> > > > > becomes relevant, as this binary (or multiple binaries, if we target 
> > > > > multiple devices in the same program) is what is passed to 
> > > > > -fembed-offload-object! And inputs[0] in this case would refer to the 
> > > > > output from the original host pass, which is what we want to embed 
> > > > > the device binary into, so we wish to skip this original host output 
> > > > > and only pass the subsequent inputs (which should be device binaries 
> > > > > when the clangDriver initiates a host offloading action) we want to 
> > > > > embed as -fembed-offload-object arguments. 
> > > > > 
> > > > > The offloading driver is quite complex and my knowledge of it is not 
> > > > > perfect as I am not our resident expert on it 

[libunwind] a7aade1 - [runtimes] Synchronize warnings flags between libc++/libc++abi/libunwind

[Nikolas Klauser via cfe-commits]

Author: Nikolas Klauser
Date: 2023-03-17T00:40:59+01:00
New Revision: a7aade1f36eb60161235b66bca46db12e5326a0c

URL: 
https://github.com/llvm/llvm-project/commit/a7aade1f36eb60161235b66bca46db12e5326a0c
DIFF: 
https://github.com/llvm/llvm-project/commit/a7aade1f36eb60161235b66bca46db12e5326a0c.diff

LOG: [runtimes] Synchronize warnings flags between libc++/libc++abi/libunwind

This mostly keeps the same warning flags. The most important exceptions are 
`-Wpedantic` and `-Wconversion`, which are now removed from libc++abi and 
libunwind.

Reviewed By: ldionne, #libunwind, #libc, #libc_abi

Spies: mikhail.ramalho, phosek, libcxx-commits

Differential Revision: https://reviews.llvm.org/D144252

Added: 
runtimes/cmake/Modules/WarningFlags.cmake

Modified: 
libcxx/CMakeLists.txt
libcxxabi/CMakeLists.txt
libcxxabi/src/CMakeLists.txt
libcxxabi/src/demangle/ItaniumDemangle.h
libunwind/CMakeLists.txt
libunwind/src/CMakeLists.txt

Removed: 




diff  --git a/libcxx/CMakeLists.txt b/libcxx/CMakeLists.txt
index 3309edd9fc4c8..cf572af74bd13 100644
--- a/libcxx/CMakeLists.txt
+++ b/libcxx/CMakeLists.txt
@@ -24,6 +24,7 @@ set(LIBCXX_BINARY_DIR  ${CMAKE_CURRENT_BINARY_DIR})
 set(LIBCXX_BINARY_INCLUDE_DIR "${LIBCXX_BINARY_DIR}/include/c++build")
 
 include(GNUInstallDirs)
+include(WarningFlags)
 
 # Require out of source build.
 include(MacroEnsureOutOfSourceBuild)
@@ -563,71 +564,6 @@ function(cxx_add_basic_build_flags target)
   target_compile_options(${target} PUBLIC "${LIBCXX_ADDITIONAL_COMPILE_FLAGS}")
 endfunction()
 
-# Warning flags ===
-function(cxx_add_warning_flags target)
-  target_compile_definitions(${target} PUBLIC 
-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
-  if (MSVC)
-# -W4 is the cl.exe/clang-cl equivalent of -Wall. (In cl.exe and clang-cl,
-# -Wall is equivalent to -Weverything in GCC style compiler drivers.)
-target_add_compile_flags_if_supported(${target} PRIVATE -W4)
-  else()
-target_add_compile_flags_if_supported(${target} PRIVATE -Wall)
-  endif()
-  target_add_compile_flags_if_supported(${target} PRIVATE -Wextra
-  -W
-  -Wwrite-strings
-  -Wno-unused-parameter
-  -Wno-long-long
-  -Werror=return-type
-  -Wextra-semi
-  -Wundef
-  -Wunused-template
-  -Wformat-nonliteral)
-  if ("${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang")
-target_add_compile_flags_if_supported(${target} PRIVATE
-  -Wno-user-defined-literals
-  -Wno-covered-switch-default
-  -Wno-suggest-override
-)
-if (LIBCXX_TARGETING_CLANG_CL)
-  target_add_compile_flags_if_supported(${target} PRIVATE
--Wno-c++98-compat
--Wno-c++98-compat-pedantic
--Wno-c++11-compat
--Wno-undef
--Wno-reserved-id-macro
--Wno-gnu-include-next
--Wno-gcc-compat # For ignoring "'diagnose_if' is a clang extension" 
warnings
--Wno-zero-as-null-pointer-constant # FIXME: Remove this and fix all 
occurrences.
--Wno-deprecated-dynamic-exception-spec # For auto_ptr
--Wno-sign-conversion
--Wno-old-style-cast
--Wno-deprecated # FIXME: Remove this and fix all occurrences.
--Wno-shift-sign-overflow # FIXME: Why do we need this with clang-cl 
but not clang?
--Wno-double-promotion # FIXME: remove me
-  )
-endif()
-  elseif("${CMAKE_CXX_COMPILER_ID}" MATCHES "GNU")
-target_add_compile_flags_if_supported(${target} PRIVATE
-  -Wno-attributes
-  -Wno-literal-suffix
-  -Wno-c++14-compat
-  -Wno-noexcept-type
-  -Wno-suggest-override)
-  endif()
-  if (LIBCXX_ENABLE_WERROR)
-target_add_compile_flags_if_supported(${target} PRIVATE -Werror)
-target_add_compile_flags_if_supported(${target} PRIVATE -WX)
-  else()
-# TODO(EricWF) Remove this. We shouldn't be suppressing errors when 
-Werror is
-# added elsewhere.
-target_add_compile_flags_if_supported(${target} PRIVATE -Wno-error)
-  endif()
-  if (LIBCXX_ENABLE_PEDANTIC)
-target_add_compile_flags_if_supported(${target} PRIVATE -pedantic)
-  endif()
-endfunction()
-
 # Exception flags =
 function(cxx_add_exception_flags target)
   if (LIBCXX_ENABLE_EXCEPTIONS)
@@ -910,7 +846,7 @@ endif()
 # Setup all common build flags 
=
 function(cxx_add_common_build_flags 

[PATCH] D146211: Make globals used for array initialization codegen constant

[Eli Friedman via Phabricator via cfe-commits]

efriedma accepted this revision.
efriedma 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/D146211/new/

https://reviews.llvm.org/D146211

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

The approach of attempting to adjust the depth here is not correct, and we 
should rip this whole mechanism out and reimplement it.  Consider this closely 
related testcase:

  template
  concept Concept = true;
  
  template
  struct A
  {
template V>
void C(V&& t);
  };
  
  template<>
  template V>
  void A::C(V&& t)
  {}

Clang rejects this because it's trying to compare `Concept` from the 
declaration against `Concept` from the explicit specialization.

The right way to fix that and the issue being addressed here is that, rather 
than adjusting the depths, we should substitute the outer template arguments 
from the scope specifier (`A::`) into the constraint before performing the 
comparison. (In the special case where none of the outer template parameters 
are used by the inner template, that does effectively just adjust the depths of 
any inner template parameters.)

The relevant language rule was introduced by the resolution of CA104 in P2103R0 
 
(which Clang's status page incorrectly marks as complete in Clang 10). In 
particular, see temp.constr.decl/4  
for the rule that we should implement, but don't:

> When determining whether a given introduced constraint-expression `C1` of a 
> declaration in an instantiated specialization of a templated class is 
> equivalent ([temp.over.link]) to the corresponding constraint-expression `C2` 
> of a declaration outside the class body, `C1` is instantiated. If the 
> instantiation results in an invalid expression, the constraint-expressions 
> are not equivalent.

So, in this case, we're supposed to instantiate the constraint expression 
`Concept`, substituting the template arguments of `A` into it (in a SFINAE 
context).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146255: [clang] Unconditionally add autolink hints for frameworks.

[Juergen Ributzka via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG29e2a4eff8f6: [clang] Unconditionally add autolink hints for 
frameworks. (authored by ributzka).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146255

Files:
  clang/lib/Lex/ModuleMap.cpp
  clang/test/Modules/use-exportas-for-link.m


Index: clang/test/Modules/use-exportas-for-link.m
===
--- clang/test/Modules/use-exportas-for-link.m
+++ clang/test/Modules/use-exportas-for-link.m
@@ -31,15 +31,17 @@
 #endif
 
 // RUN: %clang_cc1 -emit-llvm -o - -fmodules-cache-path=%t -DE -fmodules 
-fimplicit-module-maps -F %S/Inputs/exportas-link %s | FileCheck 
--check-prefix=CHECK_E %s
-// CHECK_E: !llvm.linker.options = !{![[MODULE:[0-9]+]]}
-// CHECK_E: ![[MODULE]] = !{!"-framework", !"SomeKitCore"}
+// CHECK_E: !llvm.linker.options = !{![[MODULE1:[0-9]+]], ![[MODULE2:[0-9]+]]}
+// CHECK_E: ![[MODULE1]] = !{!"-framework", !"OtherKit"}
+// CHECK_E: ![[MODULE2]] = !{!"-framework", !"SomeKitCore"}
 #ifdef E
 @import OtherKit;
 #endif
 
 // RUN: %clang_cc1 -emit-llvm -o - -fmodules-cache-path=%t -DF -fmodules 
-fimplicit-module-maps -F %S/Inputs/exportas-link %s | FileCheck 
--check-prefix=CHECK_F %s
-// CHECK_F: !llvm.linker.options = !{![[MODULE:[0-9]+]]}
-// CHECK_F: ![[MODULE]] = !{!"-framework", !"SomeKit"}
+// CHECK_F: !llvm.linker.options = !{![[MODULE1:[0-9]+]], ![[MODULE2:[0-9]+]]}
+// CHECK_F: ![[MODULE1]] = !{!"-framework", !"OtherKit"}
+// CHECK_F: ![[MODULE2]] = !{!"-framework", !"SomeKit"}
 #ifdef F
 @import OtherKit;
 #endif
Index: clang/lib/Lex/ModuleMap.cpp
===
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -930,27 +930,13 @@
 
 /// For a framework module, infer the framework against which we
 /// should link.
-static void inferFrameworkLink(Module *Mod, const DirectoryEntry *FrameworkDir,
-   FileManager ) {
+static void inferFrameworkLink(Module *Mod) {
   assert(Mod->IsFramework && "Can only infer linking for framework modules");
   assert(!Mod->isSubFramework() &&
  "Can only infer linking for top-level frameworks");
 
-  SmallString<128> LibName;
-  LibName += FrameworkDir->getName();
-  llvm::sys::path::append(LibName, Mod->Name);
-
-  // The library name of a framework has more than one possible extension since
-  // the introduction of the text-based dynamic library format. We need to 
check
-  // for both before we give up.
-  for (const char *extension : {"", ".tbd"}) {
-llvm::sys::path::replace_extension(LibName, extension);
-if (FileMgr.getFile(LibName)) {
-  Mod->LinkLibraries.push_back(Module::LinkLibrary(Mod->Name,
-   /*IsFramework=*/true));
-  return;
-}
-  }
+  Mod->LinkLibraries.push_back(Module::LinkLibrary(Mod->Name,
+   /*IsFramework=*/true));
 }
 
 Module *ModuleMap::inferFrameworkModule(const DirectoryEntry *FrameworkDir,
@@ -1129,9 +1115,8 @@
 
   // If the module is a top-level framework, automatically link against the
   // framework.
-  if (!Result->isSubFramework()) {
-inferFrameworkLink(Result, FrameworkDir, FileMgr);
-  }
+  if (!Result->isSubFramework())
+inferFrameworkLink(Result);
 
   return Result;
 }
@@ -2185,9 +2170,8 @@
   // If the active module is a top-level framework, and there are no link
   // libraries, automatically link against the framework.
   if (ActiveModule->IsFramework && !ActiveModule->isSubFramework() &&
-  ActiveModule->LinkLibraries.empty()) {
-inferFrameworkLink(ActiveModule, Directory, SourceMgr.getFileManager());
-  }
+  ActiveModule->LinkLibraries.empty())
+inferFrameworkLink(ActiveModule);
 
   // If the module meets all requirements but is still unavailable, mark the
   // whole tree as unavailable to prevent it from building.


Index: clang/test/Modules/use-exportas-for-link.m
===
--- clang/test/Modules/use-exportas-for-link.m
+++ clang/test/Modules/use-exportas-for-link.m
@@ -31,15 +31,17 @@
 #endif
 
 // RUN: %clang_cc1 -emit-llvm -o - -fmodules-cache-path=%t -DE -fmodules -fimplicit-module-maps -F %S/Inputs/exportas-link %s | FileCheck --check-prefix=CHECK_E %s
-// CHECK_E: !llvm.linker.options = !{![[MODULE:[0-9]+]]}
-// CHECK_E: ![[MODULE]] = !{!"-framework", !"SomeKitCore"}
+// CHECK_E: !llvm.linker.options = !{![[MODULE1:[0-9]+]], ![[MODULE2:[0-9]+]]}
+// CHECK_E: ![[MODULE1]] = !{!"-framework", !"OtherKit"}
+// CHECK_E: ![[MODULE2]] = !{!"-framework", !"SomeKitCore"}
 #ifdef E
 @import OtherKit;
 #endif
 
 // RUN: %clang_cc1 -emit-llvm -o - -fmodules-cache-path=%t -DF -fmodules -fimplicit-module-maps -F %S/Inputs/exportas-link %s | FileCheck --check-prefix=CHECK_F %s

[clang] 29e2a4e - [clang] Unconditionally add autolink hints for frameworks.

[Juergen Ributzka via cfe-commits]

Author: Juergen Ributzka
Date: 2023-03-16T15:31:17-07:00
New Revision: 29e2a4eff8f62d12754f31f90d70e9a346bc2075

URL: 
https://github.com/llvm/llvm-project/commit/29e2a4eff8f62d12754f31f90d70e9a346bc2075
DIFF: 
https://github.com/llvm/llvm-project/commit/29e2a4eff8f62d12754f31f90d70e9a346bc2075.diff

LOG: [clang] Unconditionally add autolink hints for frameworks.

Clang infers framework autolink hints when parsing a modulemap. In order to do
so, it checks if the module is a framework and if there is a framework binary
or TBD file in the SDK. Only when Clang finds the filei, then the autolink hint
is added to the module metadata.

During a project build many clang processes perform this check, which causes
many stat calls - even for modules/frameworks that are not even used.

The linker is already resilient to non-existing framework links that come from
the autolink metadata, so there is no need for Clang to do this check.

Instead the autolink hints are now added unconditionally and the linker only
needs to do the check once. This reduces the overall number of stat calls.

This fixes rdar://106578342.

Differential Revision: https://reviews.llvm.org/D146255

Added: 


Modified: 
clang/lib/Lex/ModuleMap.cpp
clang/test/Modules/use-exportas-for-link.m

Removed: 




diff  --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp
index 5973b4ae06240..8dead93b03734 100644
--- a/clang/lib/Lex/ModuleMap.cpp
+++ b/clang/lib/Lex/ModuleMap.cpp
@@ -930,27 +930,13 @@ Module *ModuleMap::createHeaderUnit(SourceLocation Loc, 
StringRef Name,
 
 /// For a framework module, infer the framework against which we
 /// should link.
-static void inferFrameworkLink(Module *Mod, const DirectoryEntry *FrameworkDir,
-   FileManager ) {
+static void inferFrameworkLink(Module *Mod) {
   assert(Mod->IsFramework && "Can only infer linking for framework modules");
   assert(!Mod->isSubFramework() &&
  "Can only infer linking for top-level frameworks");
 
-  SmallString<128> LibName;
-  LibName += FrameworkDir->getName();
-  llvm::sys::path::append(LibName, Mod->Name);
-
-  // The library name of a framework has more than one possible extension since
-  // the introduction of the text-based dynamic library format. We need to 
check
-  // for both before we give up.
-  for (const char *extension : {"", ".tbd"}) {
-llvm::sys::path::replace_extension(LibName, extension);
-if (FileMgr.getFile(LibName)) {
-  Mod->LinkLibraries.push_back(Module::LinkLibrary(Mod->Name,
-   /*IsFramework=*/true));
-  return;
-}
-  }
+  Mod->LinkLibraries.push_back(Module::LinkLibrary(Mod->Name,
+   /*IsFramework=*/true));
 }
 
 Module *ModuleMap::inferFrameworkModule(const DirectoryEntry *FrameworkDir,
@@ -1129,9 +1115,8 @@ Module *ModuleMap::inferFrameworkModule(const 
DirectoryEntry *FrameworkDir,
 
   // If the module is a top-level framework, automatically link against the
   // framework.
-  if (!Result->isSubFramework()) {
-inferFrameworkLink(Result, FrameworkDir, FileMgr);
-  }
+  if (!Result->isSubFramework())
+inferFrameworkLink(Result);
 
   return Result;
 }
@@ -2185,9 +2170,8 @@ void ModuleMapParser::parseModuleDecl() {
   // If the active module is a top-level framework, and there are no link
   // libraries, automatically link against the framework.
   if (ActiveModule->IsFramework && !ActiveModule->isSubFramework() &&
-  ActiveModule->LinkLibraries.empty()) {
-inferFrameworkLink(ActiveModule, Directory, SourceMgr.getFileManager());
-  }
+  ActiveModule->LinkLibraries.empty())
+inferFrameworkLink(ActiveModule);
 
   // If the module meets all requirements but is still unavailable, mark the
   // whole tree as unavailable to prevent it from building.

diff  --git a/clang/test/Modules/use-exportas-for-link.m 
b/clang/test/Modules/use-exportas-for-link.m
index 6f5fd596b4e69..722c65c2973a7 100644
--- a/clang/test/Modules/use-exportas-for-link.m
+++ b/clang/test/Modules/use-exportas-for-link.m
@@ -31,15 +31,17 @@
 #endif
 
 // RUN: %clang_cc1 -emit-llvm -o - -fmodules-cache-path=%t -DE -fmodules 
-fimplicit-module-maps -F %S/Inputs/exportas-link %s | FileCheck 
--check-prefix=CHECK_E %s
-// CHECK_E: !llvm.linker.options = !{![[MODULE:[0-9]+]]}
-// CHECK_E: ![[MODULE]] = !{!"-framework", !"SomeKitCore"}
+// CHECK_E: !llvm.linker.options = !{![[MODULE1:[0-9]+]], ![[MODULE2:[0-9]+]]}
+// CHECK_E: ![[MODULE1]] = !{!"-framework", !"OtherKit"}
+// CHECK_E: ![[MODULE2]] = !{!"-framework", !"SomeKitCore"}
 #ifdef E
 @import OtherKit;
 #endif
 
 // RUN: %clang_cc1 -emit-llvm -o - -fmodules-cache-path=%t -DF -fmodules 
-fimplicit-module-maps -F %S/Inputs/exportas-link %s | FileCheck 
--check-prefix=CHECK_F %s
-// CHECK_F: !llvm.linker.options = !{![[MODULE:[0-9]+]]}
-// CHECK_F: 

[PATCH] D146255: [clang] Unconditionally add autolink hints for frameworks.

[Juergen Ributzka via Phabricator via cfe-commits]

ributzka added a comment.

Thanks for the prompt reviews


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146255

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

[PATCH] D146255: [clang] Unconditionally add autolink hints for frameworks.

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese accepted this revision.
Bigcheese added a comment.

lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146255

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

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: flang/test/Driver/omp-frontend-forwarding.f90:1
+! REQUIRES: amdgpu-registered-target
+

jhuber6 wrote:
> agozillon wrote:
> > awarzynski wrote:
> > > Given that you use `-###`, I think that this can be skipped (please 
> > > double check).
> > It does appear that it can be, at the very least I can swap in an NVIIDIA 
> > arch when I haven't configured the project to target it and it has no 
> > issues! Thank you. 
> I'm not completely familiar with Flangs status on this, do we have tests in 
> tree that perform the entire build and check `-ccc-print-bindings/phases` 
> like we do in Clang?
> I'm not completely familiar with Flangs status on this

I don't recall any other efforts to support offloading in Flang's compiler 
driver - very nice to see this being worked on!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145815

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

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

In D143967#4200331 , @dfaust wrote:

> In D143967#4197288 , @eddyz87 wrote:
>
>> ...
>> I think there are two sides to this:
>>
>> - conceptual: is it ok to allow annotations for CVR modifiers?
>
> Maybe someone more an expert in DWARF could chime in whether this is 
> problematic?
> As far as I know it would be "ok" in the sense that it should not break DWARF 
> or cause issues for DWARF consumers which do not know about the tags.
>
> My initial reaction was that placing the annotations on CVR modifiers makes 
> less sense conceptually than placing them on the types proper.

I agree conceptually.

> But, I suppose it is a question of what precisely the annotated type is. e.g. 
> in `volatile int __attribute__((btf_type_tag("__b"))) b;` is it the case that:
>
> - type tag "__b" applies to the type `volatile int` (i.e. cvr quals  "bind 
> more closely" than the tag), or
> - type tag "__b" and `volatile` both apply to `int` (i.e. cvr quals and type 
> tags are "equal")
>
> For all other cases the new "btf:type_tag" format places the annotation as a 
> child of exactly the type that is annotated, so I think the answer determines 
> where the annotation logically belongs in DWARF:
>
> - If the tag applies to `volatile int` then it should be a child of the 
> volatile DIE.
> - If the tag applies to `int` it should be a child of the integer type DIE.

I'm aware of three use-cases for "btf_type_tag": "percpu", "user", "rcu" marks 
in kernel code. First is used for per-cpu variables, second is used to mark 
pointers to userspace memory, third to mark the data that should be read with 
RCU lock. For these use-cases there is no difference between e.g. `const __user 
*ptr` and `__user const *ptr`. I don't think the difference was ever thought 
through.

> At the moment I can't say that one is more correct than the other, so I guess 
> I have no real objection placing the annotation on the CVR modifier.
>
>> - technical: what is the point where such reordering is easiest to 
>> implement? For LLVM / pahole stack the path of least resistance is to modify 
>> DWARF generation (but again @dblaikie might disagree). How hard is it to 
>> adjust DI generation in GCC in this way?
>
> It is not a difficult change in GCC. Some added complexity but not too much. 
> I have a prototype that seems to work from initial testing.

Same here, it is a small change in clang code, but I don't like that it has to 
be handled as a special case.

> It is probably simpler than instead modifying the internal transformation to 
> BTF to reorder tags/CVR qualifiers as kernel currently requires.
>
> But I don't think ease of implementation should be a factor unless we are 
> sure the format makes sense.
> We are changing the format because the old one was flawed, let's try to make 
> sure we aren't just replacing it with something flawed in a different way 
> because it is easy.

The main issue is that kernel currently expects type tags before modifiers. 
Even if the kernel code would be modified there is still an issue of backwards 
compatibility. During kernel build BTF is generated from DWARF and is embedded 
in a kernel image, it is done by tool named pahole 
. If we forgo modifiers reordering the 
embedded BTF would not match format supported by kernel.

So, I assume that reordering _has_ to happen somewhere. Either at DWARF 
generation stage, or in two places:

- compiler BPF backend, when BPF programs are compiled;
- `pahole`, when BTF is generated from DWARF.

Let me prototype pahole change to see how much of hassle that is, it will take 
a day or two.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143967

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

[PATCH] D143704: [flang] Feature list plugin

[Johannes Doerfert via Phabricator via cfe-commits]

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

LG, as it doesn't affect core features.




Comment at: flang/examples/FeatureList/FeatureList.cpp:40-43
+const auto [it, ins] = frequencies.insert({name, 1});
+if (!ins) {
+  frequencies[name] = it->second + 1;
+}

you could used  and avoid a second lookup in line 42.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143704

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

[PATCH] D125171: Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:1372
+.. _AlwaysBreakBeforeFunctionParameters:
+
+**AlwaysBreakBeforeFunctionParameters** (``Boolean``) 
:versionbadge:`clang-format 16.0`

jrmolin wrote:
> MyDeveloperDay wrote:
> > Regererate
> How do I re-generate? I'm happy to! I just don't see where to do that.
docs/tools/dump_format_style.py but ensure you move your example to Format.h 
first or its going to wipe out these changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125171

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

[PATCH] D146247: [clang-format] Support TypeScript satisfies operator

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

I would like to see an annotator test just to be sure its getting annotated 
correctly.

apart from that it looks pretty good, would you add this to the release notes 
please?




Comment at: clang/unittests/Format/FormatTestJS.cpp:2186
+TEST_F(FormatTestJS, SatisfiesSyntax) {
+  verifyFormat("let x = foo satisfies Type;");
+  verifyFormat("let x = (a + b) satisfies\n"

would you take say this example and add it as an annotator test 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146247

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

[PATCH] D146164: Fix nomerge attribute not working with __builtin_trap().

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added a comment.

In D146164#4200414 , @rsmith wrote:

> FYI: this attribute appears to not work on (at least) x86 and arm currently, 
> because the backend also does some merging: https://godbolt.org/z/d43M83oax

Thanks for reporting it. I'll look into it later.




Comment at: clang/lib/CodeGen/CGExpr.cpp:3626-3627
   }
-
+  if (InNoMergeAttributedStmt)
+TrapCall->addFnAttr(llvm::Attribute::NoMerge);
   return TrapCall;

rsmith wrote:
> There are 496 calls to `Builder.CreateCall` in clang's `CodeGen`. Do they all 
> need this change? If not, how can we be confident we've found all the ones 
> that do? (From a quick check, at least MSVC's `__fastfail` builtin seems like 
> it would also benefit from this handling.)
> 
> Would it be reasonable to make clang's `CGBuilder` do this for every call 
> instruction we create?
I think all of them (builtin functions) should have that change, theoretically, 
because they are not different from `__builtin_trap`. Probably we should change 
the `Builder.CreateCall` to take a list of attributes that will be added to 
that CallInst and update all calls to `CreateCall`. Is there any easier 
approach?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146164

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

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 accepted this revision.
jhuber6 added a comment.

LGTM, it's much simpler for now since Flang doesn't support CUDA, HIP, OpenCL, 
OpenMP, etc.




Comment at: flang/test/Driver/omp-frontend-forwarding.f90:1
+! REQUIRES: amdgpu-registered-target
+

agozillon wrote:
> awarzynski wrote:
> > Given that you use `-###`, I think that this can be skipped (please double 
> > check).
> It does appear that it can be, at the very least I can swap in an NVIIDIA 
> arch when I haven't configured the project to target it and it has no issues! 
> Thank you. 
I'm not completely familiar with Flangs status on this, do we have tests in 
tree that perform the entire build and check `-ccc-print-bindings/phases` like 
we do in Clang?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145815

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

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

[Andrzej Warzynski via Phabricator via cfe-commits]

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

LGTM, thanks for working on this! Would be great for somebody with a bit more 
experience with offloading to OK this as well :) @tschuett or perhaps @jhuber6 ?




Comment at: clang/lib/Driver/ToolChains/Flang.cpp:128
+  if (IsHostOffloadingAction) {
+for (size_t i = 1; i < Inputs.size(); ++i) {
+  if (Inputs[i].getType() != types::TY_Nothing)

jhuber6 wrote:
> agozillon wrote:
> > jhuber6 wrote:
> > > agozillon wrote:
> > > > awarzynski wrote:
> > > > > agozillon wrote:
> > > > > > awarzynski wrote:
> > > > > > > What's the magic "1"? And given that the input count matters here 
> > > > > > > - is there a test with multiple inputs?
> > > > > > It aims to mimic the behavior of Clang: 
> > > > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L4561
> > > > > >  where the main input is skipped (the input currently being 
> > > > > > compiled or embedded into etc.), when adding to 
> > > > > > //-fembed-offload-object//. 
> > > > > > 
> > > > > > It does look different to Clang's as Clang has more cases and the 
> > > > > > logic is spread across the constructJob invocation, but the first 
> > > > > > if case is what the if statement inside of the loop and setting the 
> > > > > > loop index variable to 1 do. The HostOffloadingInputs array is what 
> > > > > > is being generated here, except we're skipping and directly 
> > > > > > applying it as arguments.
> > > > > > 
> > > > > > I tried to condense it a little in this case! Perhaps it loses 
> > > > > > readability though, I had hoped the comment might have kept it clear
> > > > > Thanks for the link - that code in Clang doesn't really clarify what 
> > > > > makes `Inputs[0]` special 樂 . 
> > > > > 
> > > > > Let me rephrase my question - what's so special about the first 
> > > > > input? (referred to in Clang as "main input") Is that something 
> > > > > specific to OpenMP? For example, in this case:
> > > > > ```
> > > > > flang-new  -fopenmp  file.f90
> > > > > ```
> > > > > I assume that `inputs[0]` is "file.f90", so nothing will happen?
> > > > > 
> > > > > > I tried to condense it a little in this case! Perhaps it loses 
> > > > > > readability though, I had hoped the comment might have kept it clear
> > > > > 
> > > > > Nah, I think that your implementation is fine. It's my ignorance with 
> > > > > respect to OpenMP that's the problem here ;-)
> > > > It's not specific to OpenMP I believe, as far as I am aware Clang's 
> > > > supported offload models (SYCL and CUDA as well as OpenMP) use it! In 
> > > > Flang's case we only really care about OpenMP as I believe it's the 
> > > > only offload programming model supported.
> > > > 
> > > > In the case of the command: 
> > > > 
> > > > ```
> > > > flang-new -fopenmp file.f90
> > > > ``` 
> > > > The code should never be executed as no part of the command will enable 
> > > > an offloading action (for device or host)! But yes inputs[0] would 
> > > > indeed refer to file.f90.
> > > > 
> > > > However, this code becomes relevant when you utilise an option that 
> > > > enables the clangDriver to perform some form of offloading action. For 
> > > > example a command like: 
> > > > 
> > > > ```
> > > > flang-new -fopenmp --offload-arch=gfx90a file.f90 
> > > > ```
> > > > Will trigger two phase compilation, one for the host device (your 
> > > > resident CPU in this command) and one for the device (gfx90a in this 
> > > > command), the regular host pass will invoke like your provided command 
> > > > and the device pass will then invoke with -fopenmp-is-device in 
> > > > addition alongside the device triple. This generates two bitcode files 
> > > > from the one file, one containing the host code from the file, the 
> > > > other the device code (extracted from OpenMP target regions or declare 
> > > > target etc.). 
> > > > 
> > > > However, now we have two files, with both parts of our program, we need 
> > > > to conjoin them together, the clangDriver generates an embeddable 
> > > > fat-binary/binary using the clang-offload-packager and then invokes 
> > > > flang-new again, and this is where the above code becomes relevant, as 
> > > > this binary (or multiple binaries, if we target multiple devices in the 
> > > > same program) is what is passed to -fembed-offload-object! And 
> > > > inputs[0] in this case would refer to the output from the original host 
> > > > pass, which is what we want to embed the device binary into, so we wish 
> > > > to skip this original host output and only pass the subsequent inputs 
> > > > (which should be device binaries when the clangDriver initiates a host 
> > > > offloading action) we want to embed as -fembed-offload-object 
> > > > arguments. 
> > > > 
> > > > The offloading driver is quite complex and my knowledge of it is not 
> > > > perfect as I am not our resident 

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

[Alexander Shaposhnikov via Phabricator via cfe-commits]

alexander-shaposhnikov added a comment.

@shafik - it's a bit early to review this patch, this was the first attempt to 
fix the issue related to the current behavior of  
clang::Sema::TemplateParameterListsAreEqual that causes Clang to mishandle 
out-of-line definitions involving constraints (and possibly is the root cause 
of a few other issues related to concepts).
It's still work-in-progress. What's happening is roughly the following: there 
is a mismatch of depths of types (TemplateTypeParmType) referenced by 
ConceptSpecializationExpr. 
E.g. for the code

  template 
  concept Concept = true;
  
  template
  struct a
  {
template
void c(T1&& t);
  };
  
  template<>
  template
  void a::c(T2&& t)
  {}



  lldb) p OldConstr->dump()
  ConceptSpecializationExpr 0x64bc4c50 '_Bool' Concept 0x64bc4608 
'Concept'
  |-ImplicitConceptSpecializationDecl 0x64bc4be0 
  | `-TemplateArgument type 'type-parameter-1-0'
  |   `-TemplateTypeParmType 0x64bc4b70 'type-parameter-1-0' dependent 
depth 1 index 0
  `-TemplateArgument type 'T1'
`-TemplateTypeParmType 0x64bc4ba0 'T1' dependent depth 1 index 0
  `-TemplateTypeParm 0x64bc4ac8 'T1'
  
  (lldb) p NewConstr->dump()
  ConceptSpecializationExpr 0x64bc5140 '_Bool' Concept 0x64bc4608 
'Concept'
  |-ImplicitConceptSpecializationDecl 0x64bc50d0 
  | `-TemplateArgument type 'type-parameter-0-0'
  |   `-TemplateTypeParmType 0x64bc4580 'type-parameter-0-0' dependent 
depth 0 index 0
  `-TemplateArgument type 'T2'
`-TemplateTypeParmType 0x64bc5090 'T2' dependent depth 0 index 0
  `-TemplateTypeParm 0x64bc4ff0 'T2'

Inside Sema::AreConstraintExpressionsEqual there is some logic do adjust the 
depths, but at the moment i can't claim that i fully understand it - still 
investigating.
I think @erichkeane has also looked into the issue / debugged what's going on 
here - if I'm missing something / or something is not right - any corrections 
would be appreciated.
My current plan is to try to adopt the suggestion from

In D146178#4199382 , @erichkeane 
wrote:

> In D146178#4199263 , @erichkeane 
> wrote:
>
>> After a night of sleeping on it, the use of a AdjustConstraintDepth::Diff 
>> and AdjustConstraintDepth::Value feels like a hacky solution to SOMETHING 
>> here, though I'm not sure what yet. The depth of a template shouldn't change 
>> between the equality and constraint checking, it is usually a property of 
>> the specialization level.  I could definitely believe that 
>> `getTemplateInstantiationArgs` needs some sort of change here to better 
>> detect its own state, but this solution doesn't seem right to me.
>
> I debugged a bit: It isn't correct I think that `FunctionTemplateDecl` 
> doesn't pick up its template arguments in `getTemplateInstantiationArgs`.  
> Additionally, instead of picking up its `DeclContext`, it probably needs its 
> `LexicalDeclContext` as the next step, which I believe fixes the problem 
> (plus your change in SemaOverload.cpp)
>
> EDIT: `FunctionTemplateDecl` doesn't pick up its template arguments on 
> purpose: there ARE no template arguments, so that is me being silly.  
> However, the difference here is that it isn't picking its lexical decl 
> context here.  There is likely a similar solution here for VarTemplateDecl.
>
> I'm leaning toward the solution here being in the CalculateTemplateDepth here 
> instead: Recognize those two types, and set the correct DeclContext to pass 
> to the getTemplateInstantiationArgs (that is, the lexical decl context).

and see if i can make it functional, this will take some time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D143704: [flang] Feature list plugin

[Ethan Luis McDonough via Phabricator via cfe-commits]

elmcdonough added a comment.
Herald added a subscriber: jplehr.

ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143704

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

[PATCH] D146255: [clang] Unconditionally add autolink hints for frameworks.

[Pete Cooper via Phabricator via cfe-commits]

pete accepted this revision.
pete 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/D146255/new/

https://reviews.llvm.org/D146255

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

[PATCH] D146163: Experimental new python bindings for clang/llvm based on Cython

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks resigned from this revision.
aeubanks added a comment.

not familiar enough with python, but there should probably be an RFC for this


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146163

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

[PATCH] D146165: docs: add some documentation on Windows SDK search

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

Nice, thanks for doing this!




Comment at: clang/docs/UsersManual.rst:4537
+
+TODO: This is not yet implemented.
+

But isn't this how clang-cl finds stuff when running in a "Visual Studio 
Command Prompt" / setenv / vcvars or whatever it's called these days?

I think this would be the most common case for people building from the command 
line, and I'd suggest perhaps starting the list with this one (I realize 
they're currently sorted by precedence, but sorting by most common first would 
also be valid I think).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146165

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

[PATCH] D146234: [clang] Fix crash when handling nested immediate invocations

[Mariya Podchishchaeva via Phabricator via cfe-commits]

Fznamznon added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:17897
+if (SemaRef.FailedImmediateInvocations.contains(E))
+  CurrentII->setInt(1);
+  } else {

shafik wrote:
> It is not obvious to me why this is correct, can you please explain.
The visitor which this function is a part of traverses all subexpressions of a 
given ImmediateInvocationCandidate, it is saved to `CurrentII` member. Each 
ImmediateIvocationCandidate is a pair of integer and a pointer to ConstantExpr. 
The non-zero integer indicates that we should not try to evaluate particular 
immediate invocation. So, this patch, once found out that current expression 
contains a subexpression from another evaluation context whose evaluation 
failed before, marks that we shouldn't evaluate it to avoid double diagnosing. 
Otherwise, if some subexpression was found in the same context that current 
ImmediateInvocationCandidate belongs to, mark that we should not evaluate it 
instead, again to avoid double diagnosing. Hope that helps.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146234

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

I also don't get what is going on here, either a more detailed summary or more 
inline comments should help.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/lib/Sema/SemaOverload.cpp:1303
+OldTemplate->getTemplateParameters(), false, TPL_TemplateMatch,
+SourceLocation(), false /* PartialOrdering */);
 bool SameReturnType = Context.hasSameType(Old->getDeclaredReturnType(),

nit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146164: Fix nomerge attribute not working with __builtin_trap().

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

FYI: this attribute appears to not work on (at least) x86 and arm currently, 
because the backend also does some merging: https://godbolt.org/z/d43M83oax




Comment at: clang/lib/CodeGen/CGExpr.cpp:3626-3627
   }
-
+  if (InNoMergeAttributedStmt)
+TrapCall->addFnAttr(llvm::Attribute::NoMerge);
   return TrapCall;

There are 496 calls to `Builder.CreateCall` in clang's `CodeGen`. Do they all 
need this change? If not, how can we be confident we've found all the ones that 
do? (From a quick check, at least MSVC's `__fastfail` builtin seems like it 
would also benefit from this handling.)

Would it be reasonable to make clang's `CGBuilder` do this for every call 
instruction we create?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146164

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

[PATCH] D146054: [RISCV] Add -print-supported-marchs and -march=help support

[Alex Bradbury via Phabricator via cfe-commits]

asb added inline comments.



Comment at: llvm/lib/Support/RISCVISAInfo.cpp:153
+  std::set TempSet(Cmp);
+  for (auto E : SupportedExtensions)
+TempSet.insert(E);

craig.topper wrote:
> Question for the community. Should we maintain SupportedExtensions in the 
> order we want printed instead of sorting?
+1 on Kito's suggestion to print by the canonical order. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146054

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

[PATCH] D144981: [Driver] Allow to collect `-save-stats` data to a file specified in the environment variable.

[Volodymyr Sapsai 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 rGa845aeb5d6c8: [Driver] Allow to collect `-save-stats` data 
to a file specified in the… (authored by vsapsai).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144981

Files:
  clang/docs/CommandGuide/clang.rst
  clang/include/clang/Driver/Driver.h
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/FrontendOptions.h
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/test/Driver/save-stats.c
  clang/tools/driver/driver.cpp

Index: clang/tools/driver/driver.cpp
===
--- clang/tools/driver/driver.cpp
+++ clang/tools/driver/driver.cpp
@@ -307,6 +307,9 @@
   TheDriver.CCPrintProcessStats =
   checkEnvVar("CC_PRINT_PROC_STAT", "CC_PRINT_PROC_STAT_FILE",
 TheDriver.CCPrintStatReportFilename);
+  TheDriver.CCPrintInternalStats =
+  checkEnvVar("CC_PRINT_INTERNAL_STAT", "CC_PRINT_INTERNAL_STAT_FILE",
+TheDriver.CCPrintInternalStatReportFilename);
 
   return true;
 }
Index: clang/test/Driver/save-stats.c
===
--- clang/test/Driver/save-stats.c
+++ clang/test/Driver/save-stats.c
@@ -26,3 +26,14 @@
 
 // RUN: %clang -target x86_64-linux-unknown -save-stats=obj -flto -o obj/dir/save-stats.exe %s -### 2>&1 | FileCheck %s -check-prefix=CHECK-LTO-OBJ
 // CHECK-LTO-OBJ: "-plugin-opt=stats-file=obj/dir{{/|}}save-stats.stats"
+
+// RUN: env CC_PRINT_INTERNAL_STAT=1 \
+// RUN: %clang -target x86_64-apple-darwin %s -### 2>&1 | FileCheck %s -check-prefix=CHECK-ENV
+// CHECK-ENV: "-stats-file=-"
+// CHECK-ENV-NO: "stats-file-append"
+
+// RUN: env CC_PRINT_INTERNAL_STAT=1 \
+// RUN: CC_PRINT_INTERNAL_STAT_FILE=/tmp/stats.json \
+// RUN: %clang -target x86_64-apple-darwin %s -### 2>&1 | FileCheck %s -check-prefix=CHECK-ENV-FILE
+// CHECK-ENV-FILE: "-stats-file=/tmp/stats.json"
+// CHECK-ENV-FILE: "-stats-file-append"
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -1087,9 +1087,12 @@
   }
   StringRef StatsFile = getFrontendOpts().StatsFile;
   if (!StatsFile.empty()) {
+llvm::sys::fs::OpenFlags FileFlags = llvm::sys::fs::OF_TextWithCRLF;
+if (getFrontendOpts().AppendStats)
+  FileFlags |= llvm::sys::fs::OF_Append;
 std::error_code EC;
-auto StatS = std::make_unique(
-StatsFile, EC, llvm::sys::fs::OF_TextWithCRLF);
+auto StatS =
+std::make_unique(StatsFile, EC, FileFlags);
 if (EC) {
   getDiagnostics().Report(diag::warn_fe_unable_to_open_stats_file)
   << StatsFile << EC.message();
Index: clang/lib/Driver/ToolChains/CommonArgs.cpp
===
--- clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -1766,22 +1766,29 @@
  const InputInfo ,
  const Driver ) {
   const Arg *A = Args.getLastArg(options::OPT_save_stats_EQ);
-  if (!A)
+  if (!A && !D.CCPrintInternalStats)
 return {};
 
-  StringRef SaveStats = A->getValue();
   SmallString<128> StatsFile;
-  if (SaveStats == "obj" && Output.isFilename()) {
-StatsFile.assign(Output.getFilename());
-llvm::sys::path::remove_filename(StatsFile);
-  } else if (SaveStats != "cwd") {
-D.Diag(diag::err_drv_invalid_value) << A->getAsString(Args) << SaveStats;
-return {};
-  }
+  if (A) {
+StringRef SaveStats = A->getValue();
+if (SaveStats == "obj" && Output.isFilename()) {
+  StatsFile.assign(Output.getFilename());
+  llvm::sys::path::remove_filename(StatsFile);
+} else if (SaveStats != "cwd") {
+  D.Diag(diag::err_drv_invalid_value) << A->getAsString(Args) << SaveStats;
+  return {};
+}
 
-  StringRef BaseName = llvm::sys::path::filename(Input.getBaseInput());
-  llvm::sys::path::append(StatsFile, BaseName);
-  llvm::sys::path::replace_extension(StatsFile, "stats");
+StringRef BaseName = llvm::sys::path::filename(Input.getBaseInput());
+llvm::sys::path::append(StatsFile, BaseName);
+llvm::sys::path::replace_extension(StatsFile, "stats");
+  } else {
+assert(D.CCPrintInternalStats);
+StatsFile.assign(D.CCPrintInternalStatReportFilename.empty()
+ ? "-"
+ : D.CCPrintInternalStatReportFilename);
+  }
   return StatsFile;
 }
 
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- 

[clang] a845aeb - [Driver] Allow to collect `-save-stats` data to a file specified in the environment variable.

[Volodymyr Sapsai via cfe-commits]

Author: Volodymyr Sapsai
Date: 2023-03-16T11:57:59-07:00
New Revision: a845aeb5d6c869146fa24194a7d0182a4787cad8

URL: 
https://github.com/llvm/llvm-project/commit/a845aeb5d6c869146fa24194a7d0182a4787cad8
DIFF: 
https://github.com/llvm/llvm-project/commit/a845aeb5d6c869146fa24194a7d0182a4787cad8.diff

LOG: [Driver] Allow to collect `-save-stats` data to a file specified in the 
environment variable.

Using two environment variables `CC_PRINT_INTERNAL_STAT` and
`CC_PRINT_INTERNAL_STAT_FILE` to work like `CC_PRINT_PROC_STAT`.

The purpose of the change is to allow collecting the internal stats
without modifying the build scripts. Write all stats to a single file
to simplify aggregating the data.

Differential Revision: https://reviews.llvm.org/D144981

Added: 


Modified: 
clang/docs/CommandGuide/clang.rst
clang/include/clang/Driver/Driver.h
clang/include/clang/Driver/Options.td
clang/include/clang/Frontend/FrontendOptions.h
clang/lib/Driver/Driver.cpp
clang/lib/Driver/ToolChains/Clang.cpp
clang/lib/Driver/ToolChains/CommonArgs.cpp
clang/lib/Frontend/CompilerInstance.cpp
clang/test/Driver/save-stats.c
clang/tools/driver/driver.cpp

Removed: 




diff  --git a/clang/docs/CommandGuide/clang.rst 
b/clang/docs/CommandGuide/clang.rst
index e076818697eb1..0722979885afb 100644
--- a/clang/docs/CommandGuide/clang.rst
+++ b/clang/docs/CommandGuide/clang.rst
@@ -598,6 +598,16 @@ Driver Options
   directory (:option:`-save-stats`/"-save-stats=cwd") or the directory
   of the output file ("-save-state=obj").
 
+  You can also use environment variables to control the statistics reporting.
+  Setting ``CC_PRINT_INTERNAL_STAT`` to ``1`` enables the feature, the report
+  goes to stdout in JSON format.
+
+  Setting ``CC_PRINT_INTERNAL_STAT_FILE`` to a file path makes it report
+  statistics to the given file in the JSON format.
+
+  Note that ``-save-stats`` take precedence over ``CC_PRINT_INTERNAL_STAT``
+  and ``CC_PRINT_INTERNAL_STAT_FILE``.
+
 .. option:: -integrated-as, -no-integrated-as
 
   Used to enable and disable, respectively, the use of the integrated

diff  --git a/clang/include/clang/Driver/Driver.h 
b/clang/include/clang/Driver/Driver.h
index c85658d7c56e3..e3e98bad99127 100644
--- a/clang/include/clang/Driver/Driver.h
+++ b/clang/include/clang/Driver/Driver.h
@@ -194,6 +194,9 @@ class Driver {
   /// The file to log CC_PRINT_PROC_STAT_FILE output to, if enabled.
   std::string CCPrintStatReportFilename;
 
+  /// The file to log CC_PRINT_INTERNAL_STAT_FILE output to, if enabled.
+  std::string CCPrintInternalStatReportFilename;
+
   /// The file to log CC_PRINT_OPTIONS output to, if enabled.
   std::string CCPrintOptionsFilename;
 
@@ -258,6 +261,10 @@ class Driver {
   /// performance report to CC_PRINT_PROC_STAT_FILE or to stdout.
   unsigned CCPrintProcessStats : 1;
 
+  /// Set CC_PRINT_INTERNAL_STAT mode, which causes the driver to dump internal
+  /// performance report to CC_PRINT_INTERNAL_STAT_FILE or to stdout.
+  unsigned CCPrintInternalStats : 1;
+
   /// Pointer to the ExecuteCC1Tool function, if available.
   /// When the clangDriver lib is used through clang.exe, this provides a
   /// shortcut for executing the -cc1 command-line directly, in the same

diff  --git a/clang/include/clang/Driver/Options.td 
b/clang/include/clang/Driver/Options.td
index a2dbef1cc7cfe..a05e61ac0e92f 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -6147,6 +6147,9 @@ def print_stats : Flag<["-"], "print-stats">,
 def stats_file : Joined<["-"], "stats-file=">,
   HelpText<"Filename to write statistics to">,
   MarshallingInfoString>;
+def stats_file_append : Flag<["-"], "stats-file-append">,
+  HelpText<"If stats should be appended to stats-file instead of overwriting 
it">,
+  MarshallingInfoFlag>;
 def fdump_record_layouts_simple : Flag<["-"], "fdump-record-layouts-simple">,
   HelpText<"Dump record layout information in a simple form used for testing">,
   MarshallingInfoFlag>;

diff  --git a/clang/include/clang/Frontend/FrontendOptions.h 
b/clang/include/clang/Frontend/FrontendOptions.h
index ef04f3b236d87..85183a3812b42 100644
--- a/clang/include/clang/Frontend/FrontendOptions.h
+++ b/clang/include/clang/Frontend/FrontendOptions.h
@@ -278,6 +278,8 @@ class FrontendOptions {
   /// Show frontend performance metrics and statistics.
   unsigned ShowStats : 1;
 
+  unsigned AppendStats : 1;
+
   /// print the supported cpus for the current target
   unsigned PrintSupportedCPUs : 1;
 
@@ -511,16 +513,16 @@ class FrontendOptions {
 public:
   FrontendOptions()
   : DisableFree(false), RelocatablePCH(false), ShowHelp(false),
-ShowStats(false), TimeTrace(false), ShowVersion(false),
-FixWhatYouCan(false), FixOnlyWarnings(false), FixAndRecompile(false),
-FixToTemporaries(false), ARCMTMigrateEmitARCErrors(false),
-

[PATCH] D144981: [Driver] Allow to collect `-save-stats` data to a file specified in the environment variable.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Thanks for the review!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144981

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

[PATCH] D143813: [ClangFE] Check that __sync builtins are naturally aligned.

[Steven Johnson via Phabricator via cfe-commits]

srj added a comment.

> (I certainly agree everyone should be using __atomic and not __sync.)

Halide still uses the `__sync` builtins for 32-bit targets, for reasons I don't 
have at hand now (IIRC there were downstream users with tooling issues, I will 
re-check). (I don't think we are using `__sync` for 64-bit targets; if we are, 
that's a mistake on our part.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143813

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

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added inline comments.



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:128
+  if (IsHostOffloadingAction) {
+for (size_t i = 1; i < Inputs.size(); ++i) {
+  if (Inputs[i].getType() != types::TY_Nothing)

agozillon wrote:
> jhuber6 wrote:
> > agozillon wrote:
> > > awarzynski wrote:
> > > > agozillon wrote:
> > > > > awarzynski wrote:
> > > > > > What's the magic "1"? And given that the input count matters here - 
> > > > > > is there a test with multiple inputs?
> > > > > It aims to mimic the behavior of Clang: 
> > > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L4561
> > > > >  where the main input is skipped (the input currently being compiled 
> > > > > or embedded into etc.), when adding to //-fembed-offload-object//. 
> > > > > 
> > > > > It does look different to Clang's as Clang has more cases and the 
> > > > > logic is spread across the constructJob invocation, but the first if 
> > > > > case is what the if statement inside of the loop and setting the loop 
> > > > > index variable to 1 do. The HostOffloadingInputs array is what is 
> > > > > being generated here, except we're skipping and directly applying it 
> > > > > as arguments.
> > > > > 
> > > > > I tried to condense it a little in this case! Perhaps it loses 
> > > > > readability though, I had hoped the comment might have kept it clear
> > > > Thanks for the link - that code in Clang doesn't really clarify what 
> > > > makes `Inputs[0]` special 樂 . 
> > > > 
> > > > Let me rephrase my question - what's so special about the first input? 
> > > > (referred to in Clang as "main input") Is that something specific to 
> > > > OpenMP? For example, in this case:
> > > > ```
> > > > flang-new  -fopenmp  file.f90
> > > > ```
> > > > I assume that `inputs[0]` is "file.f90", so nothing will happen?
> > > > 
> > > > > I tried to condense it a little in this case! Perhaps it loses 
> > > > > readability though, I had hoped the comment might have kept it clear
> > > > 
> > > > Nah, I think that your implementation is fine. It's my ignorance with 
> > > > respect to OpenMP that's the problem here ;-)
> > > It's not specific to OpenMP I believe, as far as I am aware Clang's 
> > > supported offload models (SYCL and CUDA as well as OpenMP) use it! In 
> > > Flang's case we only really care about OpenMP as I believe it's the only 
> > > offload programming model supported.
> > > 
> > > In the case of the command: 
> > > 
> > > ```
> > > flang-new -fopenmp file.f90
> > > ``` 
> > > The code should never be executed as no part of the command will enable 
> > > an offloading action (for device or host)! But yes inputs[0] would indeed 
> > > refer to file.f90.
> > > 
> > > However, this code becomes relevant when you utilise an option that 
> > > enables the clangDriver to perform some form of offloading action. For 
> > > example a command like: 
> > > 
> > > ```
> > > flang-new -fopenmp --offload-arch=gfx90a file.f90 
> > > ```
> > > Will trigger two phase compilation, one for the host device (your 
> > > resident CPU in this command) and one for the device (gfx90a in this 
> > > command), the regular host pass will invoke like your provided command 
> > > and the device pass will then invoke with -fopenmp-is-device in addition 
> > > alongside the device triple. This generates two bitcode files from the 
> > > one file, one containing the host code from the file, the other the 
> > > device code (extracted from OpenMP target regions or declare target 
> > > etc.). 
> > > 
> > > However, now we have two files, with both parts of our program, we need 
> > > to conjoin them together, the clangDriver generates an embeddable 
> > > fat-binary/binary using the clang-offload-packager and then invokes 
> > > flang-new again, and this is where the above code becomes relevant, as 
> > > this binary (or multiple binaries, if we target multiple devices in the 
> > > same program) is what is passed to -fembed-offload-object! And inputs[0] 
> > > in this case would refer to the output from the original host pass, which 
> > > is what we want to embed the device binary into, so we wish to skip this 
> > > original host output and only pass the subsequent inputs (which should be 
> > > device binaries when the clangDriver initiates a host offloading action) 
> > > we want to embed as -fembed-offload-object arguments. 
> > > 
> > > The offloading driver is quite complex and my knowledge of it is not 
> > > perfect as I am not our resident expert on it unfortunately (so if anyone 
> > > sees anything incorrect, please do chime in and correct me)! 
> > > 
> > > But hopefully this answers your question and gives you an idea of when 
> > > and how this -fembed-offload-object comes into play, essentially a way to 
> > > get the device binaries we wish to insert into the host binary, so it can 
> > > load the binaries at runtime and execute them. Currently upstream Flang 
> > > 

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[David Faust via Phabricator via cfe-commits]

dfaust added a comment.

In D143967#4197288 , @eddyz87 wrote:

> ...
> I think there are two sides to this:
>
> - conceptual: is it ok to allow annotations for CVR modifiers?

Maybe someone more an expert in DWARF could chime in whether this is 
problematic?
As far as I know it would be "ok" in the sense that it should not break DWARF 
or cause issues for DWARF consumers which do not know about the tags.

My initial reaction was that placing the annotations on CVR modifiers makes 
less sense conceptually than placing them on the types proper.
But, I suppose it is a question of what precisely the annotated type is. e.g. 
in `volatile int __attribute__((btf_type_tag("__b"))) b;` is it the case that:

- type tag "__b" applies to the type `volatile int` (i.e. cvr quals  "bind more 
closely" than the tag), or
- type tag "__b" and `volatile` both apply to `int` (i.e. cvr quals and type 
tags are "equal")

For all other cases the new "btf:type_tag" format places the annotation as a 
child of exactly the type that is annotated, so I think the answer determines 
where the annotation logically belongs in DWARF:

- If the tag applies to `volatile int` then it should be a child of the 
volatile DIE.
- If the tag applies to `int` it should be a child of the integer type DIE.

At the moment I can't say that one is more correct than the other, so I guess I 
have no real objection placing the annotation on the CVR modifier.

> - technical: what is the point where such reordering is easiest to implement? 
> For LLVM / pahole stack the path of least resistance is to modify DWARF 
> generation (but again @dblaikie might disagree). How hard is it to adjust DI 
> generation in GCC in this way?

It is not a difficult change in GCC. Some added complexity but not too much. I 
have a prototype that seems to work from initial testing.
It is probably simpler than instead modifying the internal transformation to 
BTF to reorder tags/CVR qualifiers as kernel currently requires.

But I don't think ease of implementation should be a factor unless we are sure 
the format makes sense.
We are changing the format because the old one was flawed, let's try to make 
sure we aren't just replacing it with something flawed in a different way 
because it is easy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143967

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

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

[Andrew Gozillon via Phabricator via cfe-commits]

agozillon added inline comments.



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:128
+  if (IsHostOffloadingAction) {
+for (size_t i = 1; i < Inputs.size(); ++i) {
+  if (Inputs[i].getType() != types::TY_Nothing)

jhuber6 wrote:
> agozillon wrote:
> > awarzynski wrote:
> > > agozillon wrote:
> > > > awarzynski wrote:
> > > > > What's the magic "1"? And given that the input count matters here - 
> > > > > is there a test with multiple inputs?
> > > > It aims to mimic the behavior of Clang: 
> > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L4561
> > > >  where the main input is skipped (the input currently being compiled or 
> > > > embedded into etc.), when adding to //-fembed-offload-object//. 
> > > > 
> > > > It does look different to Clang's as Clang has more cases and the logic 
> > > > is spread across the constructJob invocation, but the first if case is 
> > > > what the if statement inside of the loop and setting the loop index 
> > > > variable to 1 do. The HostOffloadingInputs array is what is being 
> > > > generated here, except we're skipping and directly applying it as 
> > > > arguments.
> > > > 
> > > > I tried to condense it a little in this case! Perhaps it loses 
> > > > readability though, I had hoped the comment might have kept it clear
> > > Thanks for the link - that code in Clang doesn't really clarify what 
> > > makes `Inputs[0]` special 樂 . 
> > > 
> > > Let me rephrase my question - what's so special about the first input? 
> > > (referred to in Clang as "main input") Is that something specific to 
> > > OpenMP? For example, in this case:
> > > ```
> > > flang-new  -fopenmp  file.f90
> > > ```
> > > I assume that `inputs[0]` is "file.f90", so nothing will happen?
> > > 
> > > > I tried to condense it a little in this case! Perhaps it loses 
> > > > readability though, I had hoped the comment might have kept it clear
> > > 
> > > Nah, I think that your implementation is fine. It's my ignorance with 
> > > respect to OpenMP that's the problem here ;-)
> > It's not specific to OpenMP I believe, as far as I am aware Clang's 
> > supported offload models (SYCL and CUDA as well as OpenMP) use it! In 
> > Flang's case we only really care about OpenMP as I believe it's the only 
> > offload programming model supported.
> > 
> > In the case of the command: 
> > 
> > ```
> > flang-new -fopenmp file.f90
> > ``` 
> > The code should never be executed as no part of the command will enable an 
> > offloading action (for device or host)! But yes inputs[0] would indeed 
> > refer to file.f90.
> > 
> > However, this code becomes relevant when you utilise an option that enables 
> > the clangDriver to perform some form of offloading action. For example a 
> > command like: 
> > 
> > ```
> > flang-new -fopenmp --offload-arch=gfx90a file.f90 
> > ```
> > Will trigger two phase compilation, one for the host device (your resident 
> > CPU in this command) and one for the device (gfx90a in this command), the 
> > regular host pass will invoke like your provided command and the device 
> > pass will then invoke with -fopenmp-is-device in addition alongside the 
> > device triple. This generates two bitcode files from the one file, one 
> > containing the host code from the file, the other the device code 
> > (extracted from OpenMP target regions or declare target etc.). 
> > 
> > However, now we have two files, with both parts of our program, we need to 
> > conjoin them together, the clangDriver generates an embeddable 
> > fat-binary/binary using the clang-offload-packager and then invokes 
> > flang-new again, and this is where the above code becomes relevant, as this 
> > binary (or multiple binaries, if we target multiple devices in the same 
> > program) is what is passed to -fembed-offload-object! And inputs[0] in this 
> > case would refer to the output from the original host pass, which is what 
> > we want to embed the device binary into, so we wish to skip this original 
> > host output and only pass the subsequent inputs (which should be device 
> > binaries when the clangDriver initiates a host offloading action) we want 
> > to embed as -fembed-offload-object arguments. 
> > 
> > The offloading driver is quite complex and my knowledge of it is not 
> > perfect as I am not our resident expert on it unfortunately (so if anyone 
> > sees anything incorrect, please do chime in and correct me)! 
> > 
> > But hopefully this answers your question and gives you an idea of when and 
> > how this -fembed-offload-object comes into play, essentially a way to get 
> > the device binaries we wish to insert into the host binary, so it can load 
> > the binaries at runtime and execute them. Currently upstream Flang doesn't 
> > utilise this option of course, but we intend to use this as part of our 
> > OpenMP offloading efforts for AMD devices (whilst leaving the door open for 
> > other vendors devices as well). 

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added inline comments.



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:128
+  if (IsHostOffloadingAction) {
+for (size_t i = 1; i < Inputs.size(); ++i) {
+  if (Inputs[i].getType() != types::TY_Nothing)

agozillon wrote:
> awarzynski wrote:
> > agozillon wrote:
> > > awarzynski wrote:
> > > > What's the magic "1"? And given that the input count matters here - is 
> > > > there a test with multiple inputs?
> > > It aims to mimic the behavior of Clang: 
> > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L4561
> > >  where the main input is skipped (the input currently being compiled or 
> > > embedded into etc.), when adding to //-fembed-offload-object//. 
> > > 
> > > It does look different to Clang's as Clang has more cases and the logic 
> > > is spread across the constructJob invocation, but the first if case is 
> > > what the if statement inside of the loop and setting the loop index 
> > > variable to 1 do. The HostOffloadingInputs array is what is being 
> > > generated here, except we're skipping and directly applying it as 
> > > arguments.
> > > 
> > > I tried to condense it a little in this case! Perhaps it loses 
> > > readability though, I had hoped the comment might have kept it clear
> > Thanks for the link - that code in Clang doesn't really clarify what makes 
> > `Inputs[0]` special 樂 . 
> > 
> > Let me rephrase my question - what's so special about the first input? 
> > (referred to in Clang as "main input") Is that something specific to 
> > OpenMP? For example, in this case:
> > ```
> > flang-new  -fopenmp  file.f90
> > ```
> > I assume that `inputs[0]` is "file.f90", so nothing will happen?
> > 
> > > I tried to condense it a little in this case! Perhaps it loses 
> > > readability though, I had hoped the comment might have kept it clear
> > 
> > Nah, I think that your implementation is fine. It's my ignorance with 
> > respect to OpenMP that's the problem here ;-)
> It's not specific to OpenMP I believe, as far as I am aware Clang's supported 
> offload models (SYCL and CUDA as well as OpenMP) use it! In Flang's case we 
> only really care about OpenMP as I believe it's the only offload programming 
> model supported.
> 
> In the case of the command: 
> 
> ```
> flang-new -fopenmp file.f90
> ``` 
> The code should never be executed as no part of the command will enable an 
> offloading action (for device or host)! But yes inputs[0] would indeed refer 
> to file.f90.
> 
> However, this code becomes relevant when you utilise an option that enables 
> the clangDriver to perform some form of offloading action. For example a 
> command like: 
> 
> ```
> flang-new -fopenmp --offload-arch=gfx90a file.f90 
> ```
> Will trigger two phase compilation, one for the host device (your resident 
> CPU in this command) and one for the device (gfx90a in this command), the 
> regular host pass will invoke like your provided command and the device pass 
> will then invoke with -fopenmp-is-device in addition alongside the device 
> triple. This generates two bitcode files from the one file, one containing 
> the host code from the file, the other the device code (extracted from OpenMP 
> target regions or declare target etc.). 
> 
> However, now we have two files, with both parts of our program, we need to 
> conjoin them together, the clangDriver generates an embeddable 
> fat-binary/binary using the clang-offload-packager and then invokes flang-new 
> again, and this is where the above code becomes relevant, as this binary (or 
> multiple binaries, if we target multiple devices in the same program) is what 
> is passed to -fembed-offload-object! And inputs[0] in this case would refer 
> to the output from the original host pass, which is what we want to embed the 
> device binary into, so we wish to skip this original host output and only 
> pass the subsequent inputs (which should be device binaries when the 
> clangDriver initiates a host offloading action) we want to embed as 
> -fembed-offload-object arguments. 
> 
> The offloading driver is quite complex and my knowledge of it is not perfect 
> as I am not our resident expert on it unfortunately (so if anyone sees 
> anything incorrect, please do chime in and correct me)! 
> 
> But hopefully this answers your question and gives you an idea of when and 
> how this -fembed-offload-object comes into play, essentially a way to get the 
> device binaries we wish to insert into the host binary, so it can load the 
> binaries at runtime and execute them. Currently upstream Flang doesn't 
> utilise this option of course, but we intend to use this as part of our 
> OpenMP offloading efforts for AMD devices (whilst leaving the door open for 
> other vendors devices as well). We are trying to re-use/mimic as much of the 
> existing machinery that the clangDriver implements as we can! 
>  
The compiler requires at least one input file to run, otherwise it exits 

[PATCH] D146234: [clang] Fix crash when handling nested immediate invocations

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Thank you for the fix.




Comment at: clang/lib/Sema/SemaExpr.cpp:17897
+if (SemaRef.FailedImmediateInvocations.contains(E))
+  CurrentII->setInt(1);
+  } else {

It is not obvious to me why this is correct, can you please explain.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146234

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

[PATCH] D146247: [clang-format] Support TypeScript satisfies operator

[Taymon A. Beal via Phabricator via cfe-commits]

taymonbeal created this revision.
taymonbeal added reviewers: MyDeveloperDay, owenpan.
Herald added a project: All.
taymonbeal requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

The satisfies operator was added in TypeScript 4.9.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146247

Files:
  clang/lib/Format/FormatToken.h
  clang/lib/Format/TokenAnnotator.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/unittests/Format/FormatTestJS.cpp

Index: clang/unittests/Format/FormatTestJS.cpp
===
--- clang/unittests/Format/FormatTestJS.cpp
+++ clang/unittests/Format/FormatTestJS.cpp
@@ -2182,6 +2182,21 @@
getGoogleJSStyleWithColumns(40));
 }
 
+TEST_F(FormatTestJS, SatisfiesSyntax) {
+  verifyFormat("let x = foo satisfies Type;");
+  verifyFormat("let x = (a + b) satisfies\n"
+   "LongTypeIsLong;",
+   getGoogleJSStyleWithColumns(30));
+  verifyFormat("let x = [{x: 1} satisfies Type];");
+  verifyFormat("x = x satisfies [A, B];");
+  verifyFormat("x = x satisfies {a: string};");
+  verifyFormat("x = x satisfies (string);");
+  verifyFormat("x = x! satisfies (string);");
+  verifyFormat("let x = something.someFunction() satisfies\n"
+   "LongTypeIsLong;",
+   getGoogleJSStyleWithColumns(50));
+}
+
 TEST_F(FormatTestJS, TypeArguments) {
   verifyFormat("class X {}");
   verifyFormat("new X();");
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -556,10 +556,11 @@
   // must also be part of it.
   ProbablyBracedList = LBraceStack.back()->is(TT_BracedListLBrace);
 
-  ProbablyBracedList = ProbablyBracedList ||
+  ProbablyBracedList =
+  ProbablyBracedList ||
(Style.isJavaScript() &&
-NextTok->isOneOf(Keywords.kw_of, Keywords.kw_in,
- Keywords.kw_as));
+   NextTok->isOneOf(Keywords.kw_of, Keywords.kw_in, Keywords.kw_as,
+Keywords.kw_satisfies));
   ProbablyBracedList = ProbablyBracedList ||
(Style.isCpp() && NextTok->is(tok::l_paren));
 
@@ -1213,7 +1214,8 @@
   Keywords.kw_let, Keywords.kw_var, tok::kw_const,
   Keywords.kw_abstract, Keywords.kw_extends, Keywords.kw_implements,
   Keywords.kw_instanceof, Keywords.kw_interface,
-  Keywords.kw_override, Keywords.kw_throws, Keywords.kw_from));
+  Keywords.kw_override, Keywords.kw_satisfies, Keywords.kw_throws,
+  Keywords.kw_from));
 }
 
 static bool mustBeJSIdentOrValue(const AdditionalKeywords ,
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -1882,7 +1882,8 @@
 }
   }
   if (Current.Next &&
-  Current.Next->isOneOf(TT_BinaryOperator, Keywords.kw_as)) {
+  Current.Next->isOneOf(TT_BinaryOperator, Keywords.kw_as,
+Keywords.kw_satisfies)) {
 Current.setType(TT_NonNullAssertion);
 return;
   }
@@ -2074,7 +2075,7 @@
   return false;
 
 if (Tok.Previous->isOneOf(TT_LeadingJavaAnnotation, Keywords.kw_instanceof,
-  Keywords.kw_as)) {
+  Keywords.kw_as, Keywords.kw_satisfies)) {
   return false;
 }
 if (Style.isJavaScript() && Tok.Previous->is(Keywords.kw_in))
@@ -2725,7 +2726,8 @@
 return prec::Relational;
   }
   if (Style.isJavaScript() &&
-  Current->isOneOf(Keywords.kw_in, Keywords.kw_as)) {
+  Current->isOneOf(Keywords.kw_in, Keywords.kw_as,
+   Keywords.kw_satisfies)) {
 return prec::Relational;
   }
   if (Current->is(TT_BinaryOperator) || Current->is(tok::comma))
@@ -4268,11 +4270,12 @@
 (!Left.Previous || !Left.Previous->is(tok::period))) {
   return true;
 }
-if (Left.isOneOf(tok::kw_for, Keywords.kw_as) && Left.Previous &&
-Left.Previous->is(tok::period) && Right.is(tok::l_paren)) {
+if (Left.isOneOf(tok::kw_for, Keywords.kw_as, Keywords.kw_satisfies) &&
+Left.Previous && Left.Previous->is(tok::period) &&
+Right.is(tok::l_paren)) {
   return false;
 }
-if (Left.is(Keywords.kw_as) &&
+if (Left.isOneOf(Keywords.kw_as, Keywords.kw_satisfies) &&
 Right.isOneOf(tok::l_square, tok::l_brace, tok::l_paren)) {
   return true;
 }
@@ -4303,8 +4306,8 @@
 if (Right.is(TT_NonNullAssertion))
   return false;
 if (Left.is(TT_NonNullAssertion) 

[PATCH] D144634: [Clang][OpenMP] Support for Code Generation of loop bind clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/AST/StmtOpenMP.cpp:2362
+const HelperExprs ) {
+  this->LoopDirCrParmV = new LoopDirCrParam(C, StartLoc, EndLoc, CollapsedNum,
+Clauses, AssociatedStmt, Exprs);

We use tail allocation, this is definetely a mem leak.



Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:7815-7851
+  for (const auto *C : S.getClausesOfKind()) {
+OpenMPBindClauseKind bindParam = C->getBindKind();
+switch (bindParam) {
+case OMPC_BIND_parallel: {
+  OMPForDirective *ompForD = OMPForDirective::Create(
+  *(S.LoopDirCrParmV->C), S.LoopDirCrParmV->StartLoc,
+  S.LoopDirCrParmV->EndLoc, S.LoopDirCrParmV->CollapsedNum, 
newAClauses,

This is a bad place for this kind of operation here, it should be handled in 
Sema


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

https://reviews.llvm.org/D144634

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

[PATCH] D144634: [Clang][OpenMP] Support for Code Generation of loop bind clause

[Sunil K via Phabricator via cfe-commits]

koops updated this revision to Diff 505869.
koops added a comment.
Herald added subscribers: jplehr, sunshaoce.

1. formatting
2. Adding lit test
3. Removing bind clause from the set of clauses passed during bind(parallel) to 
the OMPForDirective and bind(teams) to the OMPDistributeDirective.


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

https://reviews.llvm.org/D144634

Files:
  clang/include/clang/AST/StmtOpenMP.h
  clang/lib/AST/StmtOpenMP.cpp
  clang/lib/CodeGen/CGStmtOpenMP.cpp

Index: clang/lib/CodeGen/CGStmtOpenMP.cpp
===
--- clang/lib/CodeGen/CGStmtOpenMP.cpp
+++ clang/lib/CodeGen/CGStmtOpenMP.cpp
@@ -7803,6 +7803,52 @@
   CGF.EmitStmt(CS);
 }
   };
+  /* Bind */
+  SmallVector newClauses;
+  for (auto clausePtr : S.clauses()) {
+if (clausePtr->getClauseKind() != llvm::omp::Clause::OMPC_bind) {
+  newClauses.push_back(clausePtr);
+}
+  }
+  ArrayRef newAClauses(newClauses);
+
+  for (const auto *C : S.getClausesOfKind()) {
+OpenMPBindClauseKind bindParam = C->getBindKind();
+switch (bindParam) {
+case OMPC_BIND_parallel: {
+  OMPForDirective *ompForD = OMPForDirective::Create(
+  *(S.LoopDirCrParmV->C), S.LoopDirCrParmV->StartLoc,
+  S.LoopDirCrParmV->EndLoc, S.LoopDirCrParmV->CollapsedNum, newAClauses,
+  S.LoopDirCrParmV->AssociatedStmt, S.LoopDirCrParmV->Exprs, NULL, 0);
+  EmitOMPForDirective(*ompForD);
+  return;
+  break;
+}
+case OMPC_BIND_teams: {
+  OMPDistributeDirective *ompDistD = OMPDistributeDirective::Create(
+  *(S.LoopDirCrParmV->C), S.LoopDirCrParmV->StartLoc,
+  S.LoopDirCrParmV->EndLoc, S.LoopDirCrParmV->CollapsedNum, newAClauses,
+  S.LoopDirCrParmV->AssociatedStmt, S.LoopDirCrParmV->Exprs);
+  EmitOMPDistributeDirective(*ompDistD);
+  return;
+  break;
+}
+case OMPC_BIND_thread: {
+  /*
+  OMPSimdDirective *ompSimdD = OMPSimdDirective::Create(
+  *(S.LoopDirCrParmV->C), S.LoopDirCrParmV->StartLoc,
+  S.LoopDirCrParmV->EndLoc, S.LoopDirCrParmV->CollapsedNum,
+  newAClauses, S.LoopDirCrParmV->AssociatedStmt,
+  S.LoopDirCrParmV->Exprs);
+  EmitOMPSimdDirective(*ompSimdD);
+  return;
+  */
+  break;
+}
+case OMPC_BIND_unknown:
+  return;
+}
+  }
   OMPLexicalScope Scope(*this, S, OMPD_unknown);
   CGM.getOpenMPRuntime().emitInlinedDirective(*this, OMPD_loop, CodeGen);
 }
Index: clang/lib/AST/StmtOpenMP.cpp
===
--- clang/lib/AST/StmtOpenMP.cpp
+++ clang/lib/AST/StmtOpenMP.cpp
@@ -2340,6 +2340,10 @@
   Dir->setDependentInits(Exprs.DependentInits);
   Dir->setFinalsConditions(Exprs.FinalsConditions);
   Dir->setPreInits(Exprs.PreInits);
+
+  Dir->LoopParamInit(C, StartLoc, EndLoc, CollapsedNum, Clauses, AssociatedStmt,
+ Exprs);
+
   return Dir;
 }
 
@@ -2351,6 +2355,14 @@
   numLoopChildren(CollapsedNum, OMPD_loop), CollapsedNum);
 }
 
+void OMPGenericLoopDirective::LoopParamInit(
+const ASTContext , SourceLocation StartLoc, SourceLocation EndLoc,
+unsigned CollapsedNum, ArrayRef Clauses, Stmt *AssociatedStmt,
+const HelperExprs ) {
+  this->LoopDirCrParmV = new LoopDirCrParam(C, StartLoc, EndLoc, CollapsedNum,
+Clauses, AssociatedStmt, Exprs);
+}
+
 OMPTeamsGenericLoopDirective *OMPTeamsGenericLoopDirective::Create(
 const ASTContext , SourceLocation StartLoc, SourceLocation EndLoc,
 unsigned CollapsedNum, ArrayRef Clauses, Stmt *AssociatedStmt,
Index: clang/include/clang/AST/StmtOpenMP.h
===
--- clang/include/clang/AST/StmtOpenMP.h
+++ clang/include/clang/AST/StmtOpenMP.h
@@ -5945,6 +5945,30 @@
  unsigned CollapsedNum, ArrayRef Clauses,
  Stmt *AssociatedStmt, const HelperExprs );
 
+  struct LoopDirCrParam {
+const ASTContext *C;
+SourceLocation StartLoc;
+SourceLocation EndLoc;
+unsigned CollapsedNum;
+ArrayRef Clauses;
+Stmt *AssociatedStmt;
+const HelperExprs Exprs;
+
+LoopDirCrParam(const ASTContext , SourceLocation StartLoc,
+   SourceLocation EndLoc, unsigned CollapsedNum,
+   ArrayRef Clauses, Stmt *AssociatedStmt,
+   const HelperExprs )
+: C(), StartLoc(StartLoc), EndLoc(EndLoc), CollapsedNum(CollapsedNum),
+  Clauses(Clauses), AssociatedStmt(AssociatedStmt), Exprs(Exprs) {}
+  };
+
+  void LoopParamInit(const ASTContext , SourceLocation StartLoc,
+ SourceLocation EndLoc, unsigned CollapsedNum,
+ ArrayRef Clauses, Stmt *AssociatedStmt,
+ const HelperExprs );
+
+  struct LoopDirCrParam *LoopDirCrParmV;
+
   /// Creates an empty directive with a place for \a NumClauses clauses.
   ///
   /// \param C AST 

[PATCH] D146089: [Sema] Fix null pointer dereference handleAlwaysInlineAttr.

[Erich Keane via Phabricator via cfe-commits]

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

Please add the test cases requested, plus a Release Note.  Otherwise LGTM.




Comment at: clang/test/Sema/attr-alwaysinline.cpp:36
+return x;
+}
+

craig.topper wrote:
> erichkeane wrote:
> > craig.topper wrote:
> > > erichkeane wrote:
> > > > craig.topper wrote:
> > > > > erichkeane wrote:
> > > > > > erichkeane wrote:
> > > > > > > craig.topper wrote:
> > > > > > > > erichkeane wrote:
> > > > > > > > > craig.topper wrote:
> > > > > > > > > > erichkeane wrote:
> > > > > > > > > > > Can you add a test that shows that we warn on 
> > > > > > > > > > > instantiation?  This shouldn't be a dependent declrefexpr 
> > > > > > > > > > > when instantiated.
> > > > > > > > > > > 
> > > > > > > > > > > Additionally, this would make sure that we're properly 
> > > > > > > > > > > propoagating `always_inline`.
> > > > > > > > > > Should this warn
> > > > > > > > > > 
> > > > > > > > > > ```
> > > > > > > > > > template [[gnu::noinline]]   
> > > > > > > > > >  
> > > > > > > > > > int bar(int x) {
> > > > > > > > > >  
> > > > > > > > > > if constexpr (D > 1)
> > > > > > > > > >  
> > > > > > > > > > [[clang::always_inline]] return bar(x + 1);
> > > > > > > > > >  
> > > > > > > > > > else
> > > > > > > > > >  
> > > > > > > > > > return x;   
> > > > > > > > > >  
> > > > > > > > > > }   
> > > > > > > > > >  
> > > > > > > > > > 
> > > > > > > > > >  
> > > > > > > > > > int baz(int x) {
> > > > > > > > > >  
> > > > > > > > > >   return bar<5>(x); 
> > > > > > > > > >  
> > > > > > > > > > }  
> > > > > > > > > > ```
> > > > > > > > > Yes, I would expect that to warn.
> > > > > > > > It looks like handleAlwaysInlineAttr only gets called once so 
> > > > > > > > it doesn't get called after instantiation.
> > > > > > > Hmm... thats unfortunate.  That means we're perhaps not 
> > > > > > > instantiating it correctly.  I'll take some time to poke around 
> > > > > > > as I get a chacne.
> > > > > > Ok, it looks like SemaStmt.cpp's `BuildAttributedStmt` needs to 
> > > > > > handle the attributes.  We should some day do that automatically 
> > > > > > for a majority o f attributes, but for now, just adding the call 
> > > > > > there is sufficient.
> > > > > What I do need to add?
> > > > You should be able to follow what is already there, but I suspect you 
> > > > just need to call some sort of 'handle' function for those to do this.  
> > > > There is one there already youc an crib off of.
> > > Can I do that as a separate patch? Fixing the crash seems like a net 
> > > improvement.
> > I'd prefer they be done together: They are a situation where you cannot 
> > properly test either patch without the other one being fixed, so I believe 
> > they are intrinsically linked.
> I thought not crashing on code that doesn't need to warn would still be an 
> improvement.
> 
> The warning that's being lost doesn't seem all that serious. It's just 
> telling that an alwaysinline statement attribute has priority over a noinline 
> function attribute.
> 
> We don't warn for this
> 
> ```
> void foo();
> 
> void bar() {
>   [[clang::always_inline]] foo();
> }
> ```
> 
> If foo's definition is in another translation unit, the always_inline can't 
> happen without LTO. So foo is effectively noinline without LTO.
>>The warning that's being lost doesn't seem all that serious. It's just 
>>telling that an alwaysinline statement attribute has priority over a noinline 
>>function attribute.
Isn't it saying that the statement diagnostic is losing to the function 
attribute?

As far as that example, I wouldn't expect the diagnostic to show there, as you 
mentioned.

That said, I thought this was going to be a bit more trivial than it is, I 
thought this was going to be a mild drive-by.  There needs to be some level of 
extracting the getCallExprs checks in both cases into Sema, and have that 
called upon instantiation.

I think we're OK now with this, but PLEASE make sure to put a number of tests 
in for the template cases with a "FIXME".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146089

___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D146244: [clangd] Show used symbols on #include line hover.

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo created this revision.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
VitaNuo requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146244

Files:
  clang-tools-extra/clangd/Hover.cpp
  clang-tools-extra/clangd/Hover.h
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/IncludeCleaner.h
  clang-tools-extra/clangd/unittests/HoverTests.cpp

Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -10,6 +10,7 @@
 #include "Annotations.h"
 #include "Config.h"
 #include "Hover.h"
+#include "TestFS.h"
 #include "TestIndex.h"
 #include "TestTU.h"
 #include "index/MemIndex.h"
@@ -20,6 +21,7 @@
 
 #include "gtest/gtest.h"
 #include 
+#include 
 #include 
 #include 
 
@@ -2941,7 +2943,7 @@
 )cpp"),
"int foo();", "#include \"foo.h\"", "#include \"foo.h\"",
[](HoverInfo ) { HI.Provider = "\"foo.h\""; }},
-   {Annotations(
+  {Annotations(
R"cpp(
  #include "foo.h"
 
@@ -2954,8 +2956,9 @@
   private:
 int val;
 };
-   )cpp", "", "", [](HoverInfo ) { HI.Provider = ""; }},
-   {Annotations(
+   )cpp",
+   "", "", [](HoverInfo ) { HI.Provider = ""; }},
+  {Annotations(
R"cpp(
 #include "bar.h"
 
@@ -2963,20 +2966,23 @@
 )cpp"),
R"cpp(
   int foo();
-   )cpp", "#include \"foo.h\"", "", [](HoverInfo ) { HI.Provider = "\"foo.h\""; }},
-   {Annotations(
+   )cpp",
+   "#include \"foo.h\"", "",
+   [](HoverInfo ) { HI.Provider = "\"foo.h\""; }},
+  {Annotations(
R"cpp(
 #include "bar.h"
 
 int F = [[f^oo]](); 
 )cpp"),
"int foo();",
-R"cpp(
+   R"cpp(
   #include "foo.h"
   #include "foobar.h"
 
-)cpp", "int foo();", [](HoverInfo ) { HI.Provider = "\"foo.h\""; }},
-{Annotations(
+)cpp",
+   "int foo();", [](HoverInfo ) { HI.Provider = "\"foo.h\""; }},
+  {Annotations(
R"cpp(
 #include "bar.h"
 
@@ -2984,18 +2990,19 @@
 )cpp"),
R"cpp(
   #include "foobar.h"
-   )cpp", R"cpp(
+   )cpp",
+   R"cpp(
   #include "foo.h"
   #include "foobar.h"
 
-)cpp", "int foo();", [](HoverInfo ) { HI.Provider = "\"foobar.h\""; }},
+)cpp",
+   "int foo();", [](HoverInfo ) { HI.Provider = "\"foobar.h\""; }},
   {Annotations(
R"cpp(
   #include "foo.h"
   int F = [[^MACRO]];
 )cpp"),
-   "#define MACRO 5",
-   "", "",
+   "#define MACRO 5", "", "",
[](HoverInfo ) { HI.Provider = "\"foo.h\""; }},
   {Annotations(
R"cpp(
@@ -3004,8 +3011,7 @@
 
   int F = [[M^ACRO]];
 )cpp"),
-   "#define MACRO 5",
-   "#define MACRO 10", "",
+   "#define MACRO 5", "#define MACRO 10", "",
[](HoverInfo ) { HI.Provider = "\"foo.h\""; }},
   {Annotations(
R"cpp(
@@ -3014,16 +3020,14 @@
 
   int F = [[MA^CRO]](5);
 )cpp"),
-   "#define MACRO(X) X+1",
-   "#include \"foobar.h\"", "#define MACRO(X) X+3",
+   "#define MACRO(X) X+1", "#include \"foobar.h\"", "#define MACRO(X) X+3",
[](HoverInfo ) { HI.Provider = "\"foo.h\""; }},
   {Annotations(
R"cpp(
   #include "foo.h"
   int [[^F]] = MACRO(5);
 )cpp"),
-   "#define MACRO(X) (X+1)",
-   "", "",
+   "#define MACRO(X) (X+1)", "", "",
[](HoverInfo ) { HI.Provider = ""; }},
   {Annotations(
R"cpp(
@@ -3032,7 +3036,7 @@
 )cpp"),
"#define MACRO 5", "#include \"foo.h\"", "",
[](HoverInfo ) { HI.Provider = "\"foo.h\""; }},
-   {Annotations(
+  {Annotations(
R"cpp(
   #include "bar.h"
   int F = [[MACR^O]];
@@ -3040,9 +3044,9 @@
"#define MACRO 5", R"cpp(
 #include "foo.h"
 #include "foobar.h"
-  )cpp", "#define MACRO 10",
-   [](HoverInfo ) { HI.Provider = "\"foobar.h\""; }},
-   {Annotations(
+  )cpp",
+   "#define MACRO 10", [](HoverInfo ) { HI.Provider = "\"foobar.h\""; }},
+  {Annotations(
R"cpp(
 #include "bar.h"
 
@@ -3051,21 +3055,21 @@
"", R"cpp(
 // IWYU pragma: private, include "foo.h"
 int foo();
-  )cpp", "",
-   [](HoverInfo ) { HI.Provider = "\"bar.h\""; }},
-

[clang] 4c8ee1a - [Debugify] Use ModuleAnalysisManager in instrumentation

[Arthur Eubanks via cfe-commits]

Author: Arthur Eubanks
Date: 2023-03-16T09:49:59-07:00
New Revision: 4c8ee1ac8221ef2d66b7f62b848a76d94196d87a

URL: 
https://github.com/llvm/llvm-project/commit/4c8ee1ac8221ef2d66b7f62b848a76d94196d87a
DIFF: 
https://github.com/llvm/llvm-project/commit/4c8ee1ac8221ef2d66b7f62b848a76d94196d87a.diff

LOG: [Debugify] Use ModuleAnalysisManager in instrumentation

In preparation for doing module checks of PreservedAnalyses.

Added: 


Modified: 
clang/lib/CodeGen/BackendUtil.cpp
llvm/include/llvm/Transforms/Utils/Debugify.h
llvm/lib/Transforms/Utils/Debugify.cpp
llvm/tools/opt/NewPMDriver.cpp

Removed: 




diff  --git a/clang/lib/CodeGen/BackendUtil.cpp 
b/clang/lib/CodeGen/BackendUtil.cpp
index 485eb3ad2ab8..95da681eb3bc 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -857,7 +857,7 @@ void EmitAssemblyHelper::RunOptimizationPipeline(
 if (!CodeGenOpts.DIBugsReportFilePath.empty())
   Debugify.setOrigDIVerifyBugsReportFilePath(
   CodeGenOpts.DIBugsReportFilePath);
-Debugify.registerCallbacks(PIC, FAM);
+Debugify.registerCallbacks(PIC, MAM);
   }
   // Attempt to load pass plugins and register their callbacks with PB.
   for (auto  : CodeGenOpts.PassPlugins) {

diff  --git a/llvm/include/llvm/Transforms/Utils/Debugify.h 
b/llvm/include/llvm/Transforms/Utils/Debugify.h
index 795768037da7..d4440942a64e 100644
--- a/llvm/include/llvm/Transforms/Utils/Debugify.h
+++ b/llvm/include/llvm/Transforms/Utils/Debugify.h
@@ -193,7 +193,7 @@ class DebugifyEachInstrumentation {
 
 public:
   void registerCallbacks(PassInstrumentationCallbacks ,
- FunctionAnalysisManager );
+ ModuleAnalysisManager );
   // Used within DebugifyMode::SyntheticDebugInfo mode.
   void setDIStatsMap(DebugifyStatsMap ) { DIStatsMap =  }
   const DebugifyStatsMap () const { return *DIStatsMap; }

diff  --git a/llvm/lib/Transforms/Utils/Debugify.cpp 
b/llvm/lib/Transforms/Utils/Debugify.cpp
index 5a310db1c164..93cad0888a56 100644
--- a/llvm/lib/Transforms/Utils/Debugify.cpp
+++ b/llvm/lib/Transforms/Utils/Debugify.cpp
@@ -1029,51 +1029,58 @@ static bool isIgnoredPass(StringRef PassID) {
 }
 
 void DebugifyEachInstrumentation::registerCallbacks(
-PassInstrumentationCallbacks , FunctionAnalysisManager ) {
-  PIC.registerBeforeNonSkippedPassCallback([this, ](StringRef P, Any IR) {
+PassInstrumentationCallbacks , ModuleAnalysisManager ) {
+  PIC.registerBeforeNonSkippedPassCallback([this, ](StringRef P, Any IR) {
 if (isIgnoredPass(P))
   return;
 PreservedAnalyses PA;
 PA.preserveSet();
-if (const auto **F = any_cast()) {
-  applyDebugify(*const_cast(*F),
-Mode, DebugInfoBeforePass, P);
-  FAM.invalidate(*const_cast(*F), PA);
-} else if (const auto **M = any_cast()) {
-  applyDebugify(*const_cast(*M),
-Mode, DebugInfoBeforePass, P);
-  for (Function  : *const_cast(*M))
-FAM.invalidate(F, PA);
-}
-  });
-  PIC.registerAfterPassCallback([this](StringRef P, Any IR,
-   const PreservedAnalyses ) {
-if (isIgnoredPass(P))
-  return;
 if (const auto **CF = any_cast()) {
-  auto  = *const_cast(*CF);
-  Module  = *F.getParent();
-  auto It = F.getIterator();
-  if (Mode == DebugifyMode::SyntheticDebugInfo)
-checkDebugifyMetadata(M, make_range(It, std::next(It)), P,
-  "CheckFunctionDebugify", /*Strip=*/true, 
DIStatsMap);
-  else
-checkDebugInfoMetadata(
-  M, make_range(It, std::next(It)), *DebugInfoBeforePass,
-  "CheckModuleDebugify (original debuginfo)",
-  P, OrigDIVerifyBugsReportFilePath);
+  Function  = *const_cast(*CF);
+  applyDebugify(F, Mode, DebugInfoBeforePass, P);
+  MAM.getResult(*F.getParent())
+  .getManager()
+  .invalidate(F, PA);
 } else if (const auto **CM = any_cast()) {
-  auto  = *const_cast(*CM);
-  if (Mode == DebugifyMode::SyntheticDebugInfo)
-   checkDebugifyMetadata(M, M.functions(), P, "CheckModuleDebugify",
-/*Strip=*/true, DIStatsMap);
-  else
-checkDebugInfoMetadata(
-  M, M.functions(), *DebugInfoBeforePass,
-  "CheckModuleDebugify (original debuginfo)",
-  P, OrigDIVerifyBugsReportFilePath);
+  Module  = *const_cast(*CM);
+  applyDebugify(M, Mode, DebugInfoBeforePass, P);
+  MAM.invalidate(M, PA);
 }
   });
+  PIC.registerAfterPassCallback(
+  [this, ](StringRef P, Any IR, const PreservedAnalyses ) {
+if (isIgnoredPass(P))
+  return;
+PreservedAnalyses PA;
+PA.preserveSet();
+if (const auto **CF = any_cast()) {
+  auto  = *const_cast(*CF);
+  Module  = *F.getParent();
+  auto It = 

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

[Andrew Gozillon via Phabricator via cfe-commits]

agozillon added a comment.

Deleted the clang test that was forwarding to Flang and merged with the 
omp-frontend-forwarding.f90 test where relevant. Second push was because I 
forgot to add a missing newline, which I seem to do frequently...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145815

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

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

[Andrew Gozillon via Phabricator via cfe-commits]

agozillon updated this revision to Diff 505844.
agozillon added a comment.

- Readd missing newline


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145815

Files:
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Driver/ToolChains/Flang.h
  flang/test/Driver/omp-frontend-forwarding.f90


Index: flang/test/Driver/omp-frontend-forwarding.f90
===
--- /dev/null
+++ flang/test/Driver/omp-frontend-forwarding.f90
@@ -0,0 +1,22 @@
+! Test that flang-new OpenMP and OpenMP offload related 
+! commands forward or expand to the appropriate commands 
+! for flang-new -fc1 as expected.
+
+! Test regular -fopenmp with no offload
+! RUN: %flang -### -fopenmp %s 2>&1 | FileCheck --check-prefixes=CHECK-OPENMP 
%s
+! CHECK-OPENMP: "{{[^"]*}}flang-new" "-fc1" {{.*}} "-fopenmp" {{.*}}.f90"
+! CHECK-OPENMP-NOT: "{{[^"]*}}flang-new" "-fc1" {{.*}} "-fopenmp" {{.*}} 
"-fopenmp-is-device" {{.*}}.f90"
+
+! Test regular -fopenmp with offload for basic fopenmp-is-device flag addition 
and correct fopenmp 
+! RUN: %flang -### -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa %s 2>&1 | 
FileCheck --check-prefixes=CHECK-OPENMP-IS-DEVICE %s
+! CHECK-OPENMP-IS-DEVICE: "{{[^"]*}}flang-new" "-fc1" {{.*}} "-fopenmp" {{.*}} 
"-fopenmp-is-device" {{.*}}.f90"
+
+! Testing fembed-offload-object and fopenmp-is-device
+! RUN: %flang -S -### %s -o %t 2>&1 \
+! RUN: -fopenmp --offload-arch=gfx90a \
+! RUN: --target=aarch64-unknown-linux-gnu \
+! RUN:   | FileCheck %s --check-prefixes=CHECK-OPENMP-EMBED
+! CHECK-OPENMP-EMBED: "{{[^"]*}}flang-new" "-fc1" "-triple" 
"aarch64-unknown-linux-gnu" {{.*}} "-fopenmp" {{.*}}.f90"
+! CHECK-OPENMP-EMBED-NEXT: "{{[^"]*}}flang-new" "-fc1" "-triple" 
"amdgcn-amd-amdhsa" {{.*}} "-fopenmp" {{.*}} "-fopenmp-is-device" {{.*}}.f90"
+! CHECK-OPENMP-EMBED: "{{[^"]*}}clang-offload-packager" {{.*}} 
"--image=file={{.*}}.bc,triple=amdgcn-amd-amdhsa,arch=gfx90a,kind=openmp"
+! CHECK-OPENMP-EMBED-NEXT: "{{[^"]*}}flang-new" "-fc1" "-triple" 
"aarch64-unknown-linux-gnu" {{.*}} "-fopenmp" {{.*}} 
"-fembed-offload-object={{.*}}.out" {{.*}}.bc"
Index: clang/lib/Driver/ToolChains/Flang.h
===
--- clang/lib/Driver/ToolChains/Flang.h
+++ clang/lib/Driver/ToolChains/Flang.h
@@ -56,6 +56,17 @@
   void addTargetOptions(const llvm::opt::ArgList ,
 llvm::opt::ArgStringList ) const;
 
+  /// Extract offload options from the driver arguments and add them to
+  /// the command arguments.
+  /// \param [in] C The current compilation for the driver invocation
+  /// \param [in] Inputs The input infomration on the current file inputs
+  /// \param [in] JA The job action
+  /// \param [in] Args The list of input driver arguments
+  /// \param [out] CmdArgs The list of output command arguments
+  void addOffloadOptions(Compilation , const InputInfoList ,
+ const JobAction , const llvm::opt::ArgList ,
+ llvm::opt::ArgStringList ) const;
+
   /// Extract other compilation options from the driver arguments and add them
   /// to the command arguments.
   ///
Index: clang/lib/Driver/ToolChains/Flang.cpp
===
--- clang/lib/Driver/ToolChains/Flang.cpp
+++ clang/lib/Driver/ToolChains/Flang.cpp
@@ -114,6 +114,33 @@
   // TODO: Add target specific flags, ABI, mtune option etc.
 }
 
+void Flang::addOffloadOptions(Compilation , const InputInfoList ,
+  const JobAction , const ArgList ,
+  ArgStringList ) const {
+  bool IsOpenMPDevice = JA.isDeviceOffloading(Action::OFK_OpenMP);
+  bool IsHostOffloadingAction = JA.isHostOffloading(Action::OFK_OpenMP) ||
+JA.isHostOffloading(C.getActiveOffloadKinds());
+
+  // Skips primary input file, but adds other input to the offload object. This
+  // is condensed logic from the Clang driver for embedding offload objects
+  // during HostOffloading.
+  if (IsHostOffloadingAction) {
+for (size_t i = 1; i < Inputs.size(); ++i) {
+  if (Inputs[i].getType() != types::TY_Nothing)
+CmdArgs.push_back(
+Args.MakeArgString("-fembed-offload-object=" +
+   getToolChain().getInputFilename(Inputs[i])));
+}
+  }
+
+  if (IsOpenMPDevice) {
+// -fopenmp-is-device is passed along to tell the frontend that it is
+// generating code for a device, so that only the relevant code is
+// emitted.
+CmdArgs.push_back("-fopenmp-is-device");
+  }
+}
+
 static void addFloatingPointOptions(const Driver , const ArgList ,
 ArgStringList ) {
   StringRef FPContract;
@@ -327,6 +354,9 @@
   // Add other compile options
   addOtherOptions(Args, CmdArgs);
 
+  // Offloading related options
+  addOffloadOptions(C, Inputs, 

[PATCH] D146234: [clang] Fix crash when handling nested immediate invocations

[Mariya Podchishchaeva via Phabricator via cfe-commits]

Fznamznon created this revision.
Herald added a project: All.
Fznamznon requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Before this patch it was expected that if there was several immediate
invocations they all belong to the same expression evaluation context.
During parsing of non local variable initializer a new evaluation context is
pushed, so code like this

  namespace scope {
  struct channel {
  consteval channel(const char* name) noexcept { }
  };
  consteval const char* make_channel_name(const char* name) { return name;}
  
  channel rsx_log(make_channel_name("rsx_log"));
  }

produced a nested immediate invocation whose subexpressions are attached
to different expression evaluation contexts. The constructor call
belongs to TU context and `make_channel_name` call to context of
variable initializer.

This patch removes this assumption and adds tracking of previously
failed immediate invocations, so it is possible when handling an
immediate invocation th check that its subexpressions from possibly another
evaluation context contains errors and not produce duplicate
diagnostics.

Fixes https://github.com/llvm/llvm-project/issues/58207


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146234

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/cxx2a-consteval.cpp

Index: clang/test/SemaCXX/cxx2a-consteval.cpp
===
--- clang/test/SemaCXX/cxx2a-consteval.cpp
+++ clang/test/SemaCXX/cxx2a-consteval.cpp
@@ -1050,3 +1050,41 @@
 
 }
 }
+
+namespace GH58207 {
+struct tester {
+consteval tester(const char* name) noexcept { }
+};
+consteval const char* make_name(const char* name) { return name;}
+consteval const char* pad(int P) { return "thestring"; }
+
+int bad = 10; // expected-note 6{{declared here}}
+
+tester glob1(make_name("glob1"));
+tester glob2(make_name("glob2"));
+constexpr tester cglob(make_name("cglob"));
+tester paddedglob(make_name(pad(bad))); // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+// expected-note {{read of non-const variable 'bad' is not allowed in a constant expression}}
+
+constexpr tester glob3 = { make_name("glob3") };
+constexpr tester glob4 = { make_name(pad(bad)) }; // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+  // expected-error {{constexpr variable 'glob4' must be initialized by a constant expression}} \
+  // expected-note 2{{read of non-const variable 'bad' is not allowed in a constant expression}}
+
+auto V = make_name(pad(3));
+auto V1 = make_name(pad(bad)); // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+   // expected-note {{read of non-const variable 'bad' is not allowed in a constant expression}}
+
+
+void foo() {
+  static tester loc1(make_name("loc1"));
+  static constexpr tester loc2(make_name("loc2"));
+  static tester paddedloc(make_name(pad(bad))); // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+// expected-note {{read of non-const variable 'bad' is not allowed in a constant expression}}
+}
+
+void bar() {
+  static tester paddedloc(make_name(pad(bad))); // expected-error {{call to consteval function 'GH58207::make_name' is not a constant expression}} \
+// expected-note {{read of non-const variable 'bad' is not allowed in a constant expression}}
+}
+}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -17843,6 +17843,7 @@
   bool Result = CE->EvaluateAsConstantExpr(
   Eval, SemaRef.getASTContext(), ConstantExprKind::ImmediateInvocation);
   if (!Result || !Notes.empty()) {
+SemaRef.FailedImmediateInvocations.insert(CE);
 Expr *InnerExpr = CE->getSubExpr()->IgnoreImplicit();
 if (auto *FunctionalCast = dyn_cast(InnerExpr))
   InnerExpr = FunctionalCast->getSubExpr();
@@ -17887,10 +17888,16 @@
  [E](Sema::ImmediateInvocationCandidate Elem) {
return Elem.getPointer() == E;
  });
-  assert(It != IISet.rend() &&
- "ConstantExpr marked IsImmediateInvocation should "
- "be present");
-  It->setInt(1); // Mark as deleted
+  // It is possible that some subexpression of current immediate invocation
+  // was handled from another expression evaluation context. Do not handle
+  // current immediate invocation if some of its subexpressions failed
+  

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

[Andrew Gozillon via Phabricator via cfe-commits]

agozillon added a comment.

In D145815#4199848 , @tschuett wrote:

> Flang will also support OpenACC for offload. It is very similar to OpenMP.

You are correct, thank you for reminding me! No idea how I forgot it, my 
apologies.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145815

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

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

[Thorsten via Phabricator via cfe-commits]

tschuett added a comment.

Flang will also support OpenACC for offload. It is very similar to OpenMP.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145815

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

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

[Andrew Gozillon via Phabricator via cfe-commits]

agozillon updated this revision to Diff 505842.
agozillon added a comment.

- [Flang][Driver] Try to fix failing omp-frontend-forwarding.f90 test
- [Flang][Driver] Simplify omp-frontend-forwarding.f90 test
- [Flang][Driver][Test] Delete flang-omp test from Clang and merge tests into 
omp-frontend-forwarding.f90


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145815

Files:
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Driver/ToolChains/Flang.h
  flang/test/Driver/omp-frontend-forwarding.f90


Index: flang/test/Driver/omp-frontend-forwarding.f90
===
--- /dev/null
+++ flang/test/Driver/omp-frontend-forwarding.f90
@@ -0,0 +1,22 @@
+! Test that flang-new OpenMP and OpenMP offload related 
+! commands forward or expand to the appropriate commands 
+! for flang-new -fc1 as expected.
+
+! Test regular -fopenmp with no offload
+! RUN: %flang -### -fopenmp %s 2>&1 | FileCheck --check-prefixes=CHECK-OPENMP 
%s
+! CHECK-OPENMP: "{{[^"]*}}flang-new" "-fc1" {{.*}} "-fopenmp" {{.*}}.f90"
+! CHECK-OPENMP-NOT: "{{[^"]*}}flang-new" "-fc1" {{.*}} "-fopenmp" {{.*}} 
"-fopenmp-is-device" {{.*}}.f90"
+
+! Test regular -fopenmp with offload for basic fopenmp-is-device flag addition 
and correct fopenmp 
+! RUN: %flang -### -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa %s 2>&1 | 
FileCheck --check-prefixes=CHECK-OPENMP-IS-DEVICE %s
+! CHECK-OPENMP-IS-DEVICE: "{{[^"]*}}flang-new" "-fc1" {{.*}} "-fopenmp" {{.*}} 
"-fopenmp-is-device" {{.*}}.f90"
+
+! Testing fembed-offload-object and fopenmp-is-device
+! RUN: %flang -S -### %s -o %t 2>&1 \
+! RUN: -fopenmp --offload-arch=gfx90a \
+! RUN: --target=aarch64-unknown-linux-gnu \
+! RUN:   | FileCheck %s --check-prefixes=CHECK-OPENMP-EMBED
+! CHECK-OPENMP-EMBED: "{{[^"]*}}flang-new" "-fc1" "-triple" 
"aarch64-unknown-linux-gnu" {{.*}} "-fopenmp" {{.*}}.f90"
+! CHECK-OPENMP-EMBED-NEXT: "{{[^"]*}}flang-new" "-fc1" "-triple" 
"amdgcn-amd-amdhsa" {{.*}} "-fopenmp" {{.*}} "-fopenmp-is-device" {{.*}}.f90"
+! CHECK-OPENMP-EMBED: "{{[^"]*}}clang-offload-packager" {{.*}} 
"--image=file={{.*}}.bc,triple=amdgcn-amd-amdhsa,arch=gfx90a,kind=openmp"
+! CHECK-OPENMP-EMBED-NEXT: "{{[^"]*}}flang-new" "-fc1" "-triple" 
"aarch64-unknown-linux-gnu" {{.*}} "-fopenmp" {{.*}} 
"-fembed-offload-object={{.*}}.out" {{.*}}.bc"
\ No newline at end of file
Index: clang/lib/Driver/ToolChains/Flang.h
===
--- clang/lib/Driver/ToolChains/Flang.h
+++ clang/lib/Driver/ToolChains/Flang.h
@@ -56,6 +56,17 @@
   void addTargetOptions(const llvm::opt::ArgList ,
 llvm::opt::ArgStringList ) const;
 
+  /// Extract offload options from the driver arguments and add them to
+  /// the command arguments.
+  /// \param [in] C The current compilation for the driver invocation
+  /// \param [in] Inputs The input infomration on the current file inputs
+  /// \param [in] JA The job action
+  /// \param [in] Args The list of input driver arguments
+  /// \param [out] CmdArgs The list of output command arguments
+  void addOffloadOptions(Compilation , const InputInfoList ,
+ const JobAction , const llvm::opt::ArgList ,
+ llvm::opt::ArgStringList ) const;
+
   /// Extract other compilation options from the driver arguments and add them
   /// to the command arguments.
   ///
Index: clang/lib/Driver/ToolChains/Flang.cpp
===
--- clang/lib/Driver/ToolChains/Flang.cpp
+++ clang/lib/Driver/ToolChains/Flang.cpp
@@ -114,6 +114,33 @@
   // TODO: Add target specific flags, ABI, mtune option etc.
 }
 
+void Flang::addOffloadOptions(Compilation , const InputInfoList ,
+  const JobAction , const ArgList ,
+  ArgStringList ) const {
+  bool IsOpenMPDevice = JA.isDeviceOffloading(Action::OFK_OpenMP);
+  bool IsHostOffloadingAction = JA.isHostOffloading(Action::OFK_OpenMP) ||
+JA.isHostOffloading(C.getActiveOffloadKinds());
+
+  // Skips primary input file, but adds other input to the offload object. This
+  // is condensed logic from the Clang driver for embedding offload objects
+  // during HostOffloading.
+  if (IsHostOffloadingAction) {
+for (size_t i = 1; i < Inputs.size(); ++i) {
+  if (Inputs[i].getType() != types::TY_Nothing)
+CmdArgs.push_back(
+Args.MakeArgString("-fembed-offload-object=" +
+   getToolChain().getInputFilename(Inputs[i])));
+}
+  }
+
+  if (IsOpenMPDevice) {
+// -fopenmp-is-device is passed along to tell the frontend that it is
+// generating code for a device, so that only the relevant code is
+// emitted.
+CmdArgs.push_back("-fopenmp-is-device");
+  }
+}
+
 static void addFloatingPointOptions(const Driver , const 

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

[Andrew Gozillon via Phabricator via cfe-commits]

agozillon added inline comments.



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:128
+  if (IsHostOffloadingAction) {
+for (size_t i = 1; i < Inputs.size(); ++i) {
+  if (Inputs[i].getType() != types::TY_Nothing)

awarzynski wrote:
> agozillon wrote:
> > awarzynski wrote:
> > > What's the magic "1"? And given that the input count matters here - is 
> > > there a test with multiple inputs?
> > It aims to mimic the behavior of Clang: 
> > https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L4561
> >  where the main input is skipped (the input currently being compiled or 
> > embedded into etc.), when adding to //-fembed-offload-object//. 
> > 
> > It does look different to Clang's as Clang has more cases and the logic is 
> > spread across the constructJob invocation, but the first if case is what 
> > the if statement inside of the loop and setting the loop index variable to 
> > 1 do. The HostOffloadingInputs array is what is being generated here, 
> > except we're skipping and directly applying it as arguments.
> > 
> > I tried to condense it a little in this case! Perhaps it loses readability 
> > though, I had hoped the comment might have kept it clear
> Thanks for the link - that code in Clang doesn't really clarify what makes 
> `Inputs[0]` special 樂 . 
> 
> Let me rephrase my question - what's so special about the first input? 
> (referred to in Clang as "main input") Is that something specific to OpenMP? 
> For example, in this case:
> ```
> flang-new  -fopenmp  file.f90
> ```
> I assume that `inputs[0]` is "file.f90", so nothing will happen?
> 
> > I tried to condense it a little in this case! Perhaps it loses readability 
> > though, I had hoped the comment might have kept it clear
> 
> Nah, I think that your implementation is fine. It's my ignorance with respect 
> to OpenMP that's the problem here ;-)
It's not specific to OpenMP I believe, as far as I am aware Clang's supported 
offload models (SYCL and CUDA as well as OpenMP) use it! In Flang's case we 
only really care about OpenMP as I believe it's the only offload programming 
model supported.

In the case of the command: 

```
flang-new -fopenmp file.f90
``` 
The code should never be executed as no part of the command will enable an 
offloading action (for device or host)! But yes inputs[0] would indeed refer to 
file.f90.

However, this code becomes relevant when you utilise an option that enables the 
clangDriver to perform some form of offloading action. For example a command 
like: 

```
flang-new -fopenmp --offload-arch=gfx90a file.f90 
```
Will trigger two phase compilation, one for the host device (your resident CPU 
in this command) and one for the device (gfx90a in this command), the regular 
host pass will invoke like your provided command and the device pass will then 
invoke with -fopenmp-is-device in addition alongside the device triple. This 
generates two bitcode files from the one file, one containing the host code 
from the file, the other the device code (extracted from OpenMP target regions 
or declare target etc.). 

However, now we have two files, with both parts of our program, we need to 
conjoin them together, the clangDriver generates an embeddable 
fat-binary/binary using the clang-offload-packager and then invokes flang-new 
again, and this is where the above code becomes relevant, as this binary (or 
multiple binaries, if we target multiple devices in the same program) is what 
is passed to -fembed-offload-object! And inputs[0] in this case would refer to 
the output from the original host pass, which is what we want to embed the 
device binary into, so we wish to skip this original host output and only pass 
the subsequent inputs (which should be device binaries when the clangDriver 
initiates a host offloading action) we want to embed as -fembed-offload-object 
arguments. 

The offloading driver is quite complex and my knowledge of it is not perfect as 
I am not our resident expert on it unfortunately (so if anyone sees anything 
incorrect, please do chime in and correct me)! 

But hopefully this answers your question and gives you an idea of when and how 
this -fembed-offload-object comes into play, essentially a way to get the 
device binaries we wish to insert into the host binary, so it can load the 
binaries at runtime and execute them. Currently upstream Flang doesn't utilise 
this option of course, but we intend to use this as part of our OpenMP 
offloading efforts for AMD devices (whilst leaving the door open for other 
vendors devices as well). We are trying to re-use/mimic as much of the existing 
machinery that the clangDriver implements as we can! 
 



Comment at: clang/test/Driver/flang/flang-omp.f90:6
+! Test regular -fopenmp with no offload
+! RUN: %clang --driver-mode=flang -### -fopenmp %s 2>&1 | FileCheck 
--check-prefixes=CHECK-OPENMP %s
+! CHECK-OPENMP: "{{[^"]*}}flang-new" "-fc1" {{.*}} 

[PATCH] D144976: [clangd] Add provider info on symbol hover.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks, left some comments on the unittest, I think we can make it simpler.




Comment at: clang-tools-extra/clangd/Hover.cpp:1107
+if (!Matches.empty()) {
+  // Pick the best-ranked included provider
+  Result = Matches[0]->quote();

The position of this comment is a bit confusing:

- `Headers` is sorted by the ranking
- `Matches` the result returned by  `ConvertedIncludes.match` is not sorted. 

The comment here reads like the `Matches` is sorted! I think it is better to 
move it to the outer for-loop. The first element of `Headers` which satisfies 
`!Matches.empty()` is the best-ranked provider.



Comment at: clang-tools-extra/clangd/Hover.cpp:1118
+
+  for (const auto  : Headers) {
+if (H.kind() == include_cleaner::Header::Physical &&

now the for-range loop doesn't seem necessary, we could always use the 
`Headers.front()`, right?



Comment at: clang-tools-extra/clangd/Hover.cpp:1219
 maybeAddCalleeArgInfo(N, *HI, PP);
+maybeAddSymbolProviders(AST, *HI, include_cleaner::Symbol{*DeclToUse});
   } else if (const Expr *E = N->ASTNode.get()) {

I wonder how well it works for the `NamespaceDecl`. NamespaceDecl is usually 
declared in many files, we will basically show a random provider in hover. 
We're not interested in `NamesapceDecl`, we probably need to ignore it. 

No action required here, but this is something we should bear in mind.



Comment at: clang-tools-extra/clangd/Hover.cpp:1138
+if (H.kind() == include_cleaner::Header::Physical &&
+H.physical() == SM.getFileEntryForID(SM.getMainFileID()))
+  continue;

VitaNuo wrote:
> hokein wrote:
> > MainFile provider is a special case (I don't recall the details).
> > 
> > IIUC, the model is:
> > 
> > 1) a symbol usage that is satisfied (e.g. any of its providers that are 
> > directly included in the main file), we show the one with highest rank of 
> > these included providers
> > 2) a symbol usage that is not satisfied (we choose the highest rank of all 
> > providers)
> > 3) If the provider is the main-file, we don't show it in the hover card. 
> > 
> > Based on 1), if the main-file provider is the highest, we will not show it 
> > in the hover based on 3). However, the current implementation doesn't match 
> > this behavior
> > -- on L1123 `ConvertedIncludes.match(H)` is always false  if H is a 
> > main-file, and we will choose a lower-rank provider if the main-file is the 
> > first element of `Headers`
> > -- the logic here doesn't seem to work, we should do a `break` on L1139 
> > rather than `continue`, which means we always use the `Headers[0]` element.
> > 
> > Not sure we have discussed 3), one alternative is to show the information 
> > for main-file provider as well, it seems fine to me that the hover shows 
> > `provided by the current file` text (not the full path).
> > we should do a break on L1139 rather than continue
> 
> Ok. I'm not sure if this is of great practical importance (what are the 
> chances that the main file is the first provider, and there are more 
> providers for the same symbol?),
> but you're right that we shouldn't show the lower-ranked provider for this 
> case.
> 
> > one alternative is to show the information for main-file provider as well
> 
> Yeah, this is possible ofc, but I'm not sure what the use would be. The 
> general intention of this feature (or feature set, even) is to help users 
> eliminate unnecessary dependencies, and they can hardly get rid of the main 
> file :)
> So of the two options, I'd rather opt for not showing anything at all.
>  I'm not sure if this is of great practical importance  
I think the code should match our mental model, otherwise it is hard to reason 
about whether the actual behavior is expected or a bug.

> (what are the chances that the main file is the first provider, and there are 
> more providers for the same symbol?),

I think the pattern is not rare (especially for headers), think of the case 
below.

```
#include "other.h" // other.h transitively includes the `foo.h`

class Foo;
const Foo& createFoo();
```

although the main-file provider is not the top1 given the current ranking 
implementation, we have a plan to address it, see the FIXME in 
https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/include-cleaner/lib/FindHeaders.cpp#L219.
 After addressing the FIXME, the main-file could be the top1.

> on L1123 ConvertedIncludes.match(H) is always false if H is a main-file, and 
> we will choose a lower-rank provider if the main-file is the first element of 
> Headers

This comment doesn't seem to be addressed, now it is L1105.

Given the following case, if the returned providers is [main-file, `foo.h`], 
the current code will show `foo.h` in hover.
However, based on our mental model:
 1) the main-file is one of the `Foo` providers, and it is the top1, 

[PATCH] D146164: Fix nomerge attribute not working with __builtin_trap().

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added inline comments.



Comment at: clang/test/CodeGen/attr-nomerge.cpp:44
+
+  [[clang::nomerge]] __builtin_trap();
 }

hans wrote:
> Maybe do __debugbreak() too since that's also mentioned on the debug.
The `__debugbreak()` is also emitted from `EmitTrapCall()`. So, just add a test 
case for `__debugbreak()`. 



Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6856
   return;
 }
 TargetLowering::ArgListTy Args;

hans wrote:
> Do we need to handle this "else" branch too?
> 
> Or would it make sense to do the nomerge check in the caller of 
> `visitIntrinsicCall` instead?
Setting CLI.NoMerge instead of setting DAG.getRoot(), because it later somehow 
replace the root with another instruction which causes the call instruction to 
lose the attribute. 

Added a test case for else branch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146164

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

[PATCH] D146164: Fix nomerge attribute not working with __builtin_trap().

[Zequan Wu via Phabricator via cfe-commits]

zequanwu updated this revision to Diff 505837.
zequanwu marked 3 inline comments as done.
zequanwu added a comment.

- Add test case for __debugbreak().
- Handle named trap instructions and add test case for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146164

Files:
  clang/lib/CodeGen/CGExpr.cpp
  clang/test/CodeGen/attr-nomerge.cpp
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
  llvm/test/CodeGen/X86/nomerge.ll

Index: llvm/test/CodeGen/X86/nomerge.ll
===
--- llvm/test/CodeGen/X86/nomerge.ll
+++ llvm/test/CodeGen/X86/nomerge.ll
@@ -22,7 +22,72 @@
 
 declare dso_local void @bar()
 
+define void @nomerge_trap(i32 %i) {
+entry:
+  switch i32 %i, label %if.end3 [
+i32 5, label %if.then
+i32 7, label %if.then2
+  ]
+
+if.then:
+  tail call void @llvm.trap() #0
+  unreachable
+
+if.then2:
+  tail call void @llvm.trap() #0
+  unreachable
+
+if.end3:
+  tail call void @llvm.trap() #0
+  unreachable
+}
+
+declare dso_local void @llvm.trap()
+
+define void @nomerge_debugtrap(i32 %i) {
+entry:
+  switch i32 %i, label %if.end3 [
+i32 5, label %if.then
+i32 7, label %if.then2
+  ]
+
+if.then:
+  tail call void @llvm.debugtrap() #0
+  unreachable
+
+if.then2:
+  tail call void @llvm.debugtrap() #0
+  unreachable
+
+if.end3:
+  tail call void @llvm.debugtrap() #0
+  unreachable
+}
+
+define void @nomerge_named_debugtrap(i32 %i) {
+entry:
+  switch i32 %i, label %if.end3 [
+i32 5, label %if.then
+i32 7, label %if.then2
+  ]
+
+if.then:
+  tail call void @llvm.debugtrap() #1
+  unreachable
+
+if.then2:
+  tail call void @llvm.debugtrap() #1
+  unreachable
+
+if.end3:
+  tail call void @llvm.debugtrap() #1
+  unreachable
+}
+
+declare dso_local void @llvm.debugtrap()
+
 attributes #0 = { nomerge }
+attributes #1 = { nomerge "trap-func-name"="trap_func" }
 
 ; CHECK-LABEL: foo:
 ; CHECK: # %bb.0: # %entry
@@ -34,3 +99,33 @@
 ; CHECK: callq bar
 ; CHECK: .LBB0_4: # %if.end3
 ; CHECK: jmp bar # TAILCALL
+
+; CHECK-LABEL: nomerge_trap:
+; CHECK:  # %bb.0: # %entry
+; CHECK:  # %bb.1: # %entry
+; CHECK:  # %bb.2: # %if.then
+; CHECK-NEXT: ud2
+; CHECK-NEXT: LBB1_3: # %if.then2
+; CHECK-NEXT: ud2
+; CHECK-NEXT: .LBB1_4: # %if.end3
+; CHECK-NEXT: ud2
+
+; CHECK-LABEL: nomerge_debugtrap:
+; CHECK:  # %bb.0: # %entry
+; CHECK:  # %bb.1: # %entry
+; CHECK:  # %bb.2: # %if.then
+; CHECK-NEXT: int3
+; CHECK-NEXT: LBB2_3: # %if.then2
+; CHECK-NEXT: int3
+; CHECK-NEXT: .LBB2_4: # %if.end3
+; CHECK-NEXT: int3
+
+; CHECK-LABEL: nomerge_named_debugtrap:
+; CHECK:  # %bb.0: # %entry
+; CHECK:  # %bb.1: # %entry
+; CHECK:  # %bb.2: # %if.then
+; CHECK-NEXT: callq trap_func@PLT
+; CHECK-NEXT: LBB3_3: # %if.then2
+; CHECK-NEXT: callq trap_func@PLT
+; CHECK-NEXT: .LBB3_4: # %if.end3
+; CHECK-NEXT: callq trap_func@PLT
Index: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
===
--- llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -6849,6 +6849,8 @@
 break;
   default: llvm_unreachable("unknown trap intrinsic");
   }
+  DAG.addNoMergeSiteInfo(DAG.getRoot().getNode(),
+ I.hasFnAttr(Attribute::NoMerge));
   return;
 }
 TargetLowering::ArgListTy Args;
@@ -6865,7 +6867,7 @@
 DAG.getExternalSymbol(TrapFuncName.data(),
   TLI.getPointerTy(DAG.getDataLayout())),
 std::move(Args));
-
+CLI.NoMerge = I.hasFnAttr(Attribute::NoMerge);
 std::pair Result = TLI.LowerCallTo(CLI);
 DAG.setRoot(Result.second);
 return;
Index: clang/test/CodeGen/attr-nomerge.cpp
===
--- clang/test/CodeGen/attr-nomerge.cpp
+++ clang/test/CodeGen/attr-nomerge.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -S -emit-llvm %s -triple x86_64-unknown-linux-gnu -o - | FileCheck %s
+// RUN: %clang_cc1 -S -emit-llvm %s -fms-extensions -o - | FileCheck %s
 
 class A {
 public:
@@ -40,6 +40,9 @@
 
   A *newA = new B();
   delete newA;
+
+  [[clang::nomerge]] __builtin_trap();
+  [[clang::nomerge]] __debugbreak();
 }
 
 int g(int i);
@@ -93,6 +96,8 @@
 // CHECK: load ptr, ptr
 // CHECK: %[[AG:.*]] = load ptr, ptr
 // CHECK-NEXT: call void %[[AG]](ptr {{.*}}) #[[ATTR1]]
+// CHECK: call void @llvm.trap() #[[ATTR0]]
+// CHECK: call void @llvm.debugtrap() #[[ATTR0]]
 // CHECK: call void  @_ZN1AD1Ev(ptr {{.*}}) #[[ATTR1]]
 
 // CHECK-DAG: attributes #[[ATTR0]] = {{{.*}}nomerge{{.*}}}
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -3623,7 +3623,8 @@
   CGM.getCodeGenOpts().TrapFuncName);
 TrapCall->addFnAttr(A);

[PATCH] D144878: __builtin_FILE_NAME()

[Ilya Karapsin via Phabricator via cfe-commits]

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

In D144878#4199297 , @aaron.ballman 
wrote:

> LGTM aside from a small formatting issue. Do you need someone to land this on 
> your behalf? If so, what name and email address would you like used for patch 
> attribution?

Yes, I need help with landing.
Name: Ilya Karapsin.
Email: ilya84...@gmail.com.




Comment at: clang/lib/Parse/ParseExpr.cpp:805-807
+/// [C++11] simple-type-specifier braced-init-list [C++11 5.2.3]
 /// [C++]   typename-specifier '(' expression-list[opt] ')' [C++ 5.2.3]
+/// [C++11] typename-specifier braced-init-list [C++11 5.2.3]

aaron.ballman wrote:
> Formatting here also got messed up a bit.
If these two lines are restored, "git-clang-format" will break the comments.


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

https://reviews.llvm.org/D144878

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

[PATCH] D145069: [analyzer][NFC] Split the no state change logic and bug report suppression into two visitors

[Gábor Spaits via Phabricator via cfe-commits]

spaits added a comment.

In D145069#4191046 , @xazax.hun wrote:

> Do you plan to selectively enable warnings coming from the STL to catch 
> misuses of certain STL types?

No. At first when we have found out that the Static Analyzer can reason about 
std::variant but it suppresses the diagnostics (D142354#4079643 
), we were suspecting a too strong 
heuristic somewhere, that invalidates even true positives. Since the Static 
Analyzer was able to reason about std::variant by itself we we did not want to 
write a checker to do the same thing. So the plan was to find the point where 
the suppression happen and change on the heuristics so it can let thru every 
kind of true positive about STL types/functions. But as it turns out, it is 
kind of impossible to do that without letting a lot of false positives to not 
be suppressed.

While it seems like it may be very difficult to unsuppress all the reports from 
std::variant, it still made sense to fine-tune some of these suppression.

In D145069#4191046 , @xazax.hun wrote:

> Also, those warning reports might be leaky in a sense the reported path might 
> contain implementations details from the STL that is hard to interpret.

We have tested the new heuristics and the new reports did not contain 
implementation details form STL.

In D145069#4191046 , @xazax.hun wrote:

> I am afraid, if we want to provide a good user experience, we might be doomed 
> to manually simulate the behavior of STL classes.

That might be the best approach to prevent the mentioned implementation 
dependency. Plus it would probably make it easier to write my BSc thesis if I 
have created a brand new checker.

On another note, it might be interesting to see how the checker approach and 
the force-inlining analyses compare (even if one of those approaches turn out 
to be a dead end).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145069

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

[PATCH] D146089: [Sema] Fix null pointer dereference handleAlwaysInlineAttr.

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.



Comment at: clang/test/Sema/attr-alwaysinline.cpp:36
+return x;
+}
+

erichkeane wrote:
> craig.topper wrote:
> > erichkeane wrote:
> > > craig.topper wrote:
> > > > erichkeane wrote:
> > > > > erichkeane wrote:
> > > > > > craig.topper wrote:
> > > > > > > erichkeane wrote:
> > > > > > > > craig.topper wrote:
> > > > > > > > > erichkeane wrote:
> > > > > > > > > > Can you add a test that shows that we warn on 
> > > > > > > > > > instantiation?  This shouldn't be a dependent declrefexpr 
> > > > > > > > > > when instantiated.
> > > > > > > > > > 
> > > > > > > > > > Additionally, this would make sure that we're properly 
> > > > > > > > > > propoagating `always_inline`.
> > > > > > > > > Should this warn
> > > > > > > > > 
> > > > > > > > > ```
> > > > > > > > > template [[gnu::noinline]] 
> > > > > > > > >
> > > > > > > > > int bar(int x) {  
> > > > > > > > >
> > > > > > > > > if constexpr (D > 1)  
> > > > > > > > >
> > > > > > > > > [[clang::always_inline]] return bar(x + 1);  
> > > > > > > > >
> > > > > > > > > else  
> > > > > > > > >
> > > > > > > > > return x; 
> > > > > > > > >
> > > > > > > > > } 
> > > > > > > > >
> > > > > > > > >   
> > > > > > > > >
> > > > > > > > > int baz(int x) {  
> > > > > > > > >
> > > > > > > > >   return bar<5>(x);   
> > > > > > > > >
> > > > > > > > > }  
> > > > > > > > > ```
> > > > > > > > Yes, I would expect that to warn.
> > > > > > > It looks like handleAlwaysInlineAttr only gets called once so it 
> > > > > > > doesn't get called after instantiation.
> > > > > > Hmm... thats unfortunate.  That means we're perhaps not 
> > > > > > instantiating it correctly.  I'll take some time to poke around as 
> > > > > > I get a chacne.
> > > > > Ok, it looks like SemaStmt.cpp's `BuildAttributedStmt` needs to 
> > > > > handle the attributes.  We should some day do that automatically for 
> > > > > a majority o f attributes, but for now, just adding the call there is 
> > > > > sufficient.
> > > > What I do need to add?
> > > You should be able to follow what is already there, but I suspect you 
> > > just need to call some sort of 'handle' function for those to do this.  
> > > There is one there already youc an crib off of.
> > Can I do that as a separate patch? Fixing the crash seems like a net 
> > improvement.
> I'd prefer they be done together: They are a situation where you cannot 
> properly test either patch without the other one being fixed, so I believe 
> they are intrinsically linked.
I thought not crashing on code that doesn't need to warn would still be an 
improvement.

The warning that's being lost doesn't seem all that serious. It's just telling 
that an alwaysinline statement attribute has priority over a noinline function 
attribute.

We don't warn for this

```
void foo();

void bar() {
  [[clang::always_inline]] foo();
}
```

If foo's definition is in another translation unit, the always_inline can't 
happen without LTO. So foo is effectively noinline without LTO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146089

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

[PATCH] D146168: [Sema] Stop stripping CV quals from *this captures in lambdas

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Can we also add the test from the bug report as a regression test, it looks 
like the existing test are basically covering the same thing but I have seen 
stranger bugs so it would be good to explicitly cover it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146168

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

[PATCH] D146168: [Sema] Stop stripping CV quals from *this captures in lambdas

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added subscribers: hubert.reinterpretcast, cor3ntin.
cor3ntin accepted this revision.
cor3ntin added a comment.
This revision is now accepted and ready to land.

LGTM. Triple checking with @hubert.reinterpretcast for conformance but I'm 
pretty sure that's correct.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146168

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

[PATCH] D146168: [Sema] Stop stripping CV quals from *this captures in lambdas

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

Can you add a summary, I realize folks can goto the release notes edit and see 
the bug report and then go look it up but at least a cursory summary is a 
little more reviewer friendly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146168

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

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added inline comments.



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:128
+  if (IsHostOffloadingAction) {
+for (size_t i = 1; i < Inputs.size(); ++i) {
+  if (Inputs[i].getType() != types::TY_Nothing)

agozillon wrote:
> awarzynski wrote:
> > What's the magic "1"? And given that the input count matters here - is 
> > there a test with multiple inputs?
> It aims to mimic the behavior of Clang: 
> https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L4561
>  where the main input is skipped (the input currently being compiled or 
> embedded into etc.), when adding to //-fembed-offload-object//. 
> 
> It does look different to Clang's as Clang has more cases and the logic is 
> spread across the constructJob invocation, but the first if case is what the 
> if statement inside of the loop and setting the loop index variable to 1 do. 
> The HostOffloadingInputs array is what is being generated here, except we're 
> skipping and directly applying it as arguments.
> 
> I tried to condense it a little in this case! Perhaps it loses readability 
> though, I had hoped the comment might have kept it clear
Thanks for the link - that code in Clang doesn't really clarify what makes 
`Inputs[0]` special 樂 . 

Let me rephrase my question - what's so special about the first input? 
(referred to in Clang as "main input") Is that something specific to OpenMP? 
For example, in this case:
```
flang-new  -fopenmp  file.f90
```
I assume that `inputs[0]` is "file.f90", so nothing will happen?

> I tried to condense it a little in this case! Perhaps it loses readability 
> though, I had hoped the comment might have kept it clear

Nah, I think that your implementation is fine. It's my ignorance with respect 
to OpenMP that's the problem here ;-)



Comment at: clang/test/Driver/flang/flang-omp.f90:6
+! Test regular -fopenmp with no offload
+! RUN: %clang --driver-mode=flang -### -fopenmp %s 2>&1 | FileCheck 
--check-prefixes=CHECK-OPENMP %s
+! CHECK-OPENMP: "{{[^"]*}}flang-new" "-fc1" {{.*}} "-fopenmp" {{.*}}.f90"

agozillon wrote:
> awarzynski wrote:
> > Can you remind me why isn't it possible to test this with `flang-new`? 
> I double checked, it seems possible to test these with flang-new directly, 
> the main reason I've tested it like this is as it's based on the other tests 
> in the same directory which use clang! 
> 
> However, I'm more than happy to move these tests to the 
> omp-frontend-forwarding.f90 test, remove them or keep these and add flang-new 
> variations into omp-frontend-forwarding.f90. 
I know that it's a bit confusing - Flang.cpp lives in Clang rather than Flang. 
But that's because `Flang.cpp` is part of `clangDriver` - the driver library. 
That library is shared between Clang and Flang and in principle should be taken 
out of Clang into a dedicated subproject - that's the plan :)

This is effectively a Flang patch and I would prefer this test to be added in 
Flang (with `clang` being replaced with `flang-new`). In general, I wouldn't 
add any test in Clang unless testing something Clang specific. This test, 
however, tests Flang.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145815

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

[PATCH] D146194: NOT report the warning that unnamed bitfields are uninitialized.

[Tang Wenyu via Phabricator via cfe-commits]

Tedlion updated this revision to Diff 505802.
Tedlion edited the summary of this revision.
Tedlion added a comment.

Provide the full diff context instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146194

Files:
  clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp


Index: clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
@@ -262,7 +262,9 @@
 if (T->getAsStructureType()) {
   if (Find(FR))
 return true;
-} else {
+} else if (!I->isUnnamedBitfield()){
+  // unnamed bitfields are skipped during aggregate initialization
+  // Since they are not supposed to be used, warnings should not be 
reported
   const SVal  = StoreMgr.getBinding(store, loc::MemRegionVal(FR));
   if (V.isUndef())
 return true;


Index: clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
@@ -262,7 +262,9 @@
 if (T->getAsStructureType()) {
   if (Find(FR))
 return true;
-} else {
+} else if (!I->isUnnamedBitfield()){
+  // unnamed bitfields are skipped during aggregate initialization
+  // Since they are not supposed to be used, warnings should not be reported
   const SVal  = StoreMgr.getBinding(store, loc::MemRegionVal(FR));
   if (V.isUndef())
 return true;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

[Alexander Shaposhnikov via Phabricator via cfe-commits]

alexander-shaposhnikov added a comment.

@erichkeane - ok, thanks a lot for the comments / help, I need a bit of time to 
play with the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146194: NOT report the warning that unnamed bitfields are uninitialized.

[Tang Wenyu via Phabricator via cfe-commits]

Tedlion accepted this revision.
Tedlion added a comment.

I argee that the different CSA behavior between C and C++ should be checked.

In D146194#4199257 , @steakhal wrote:

> As per your proposed change, I think we should do something like this:
>
>   for (const auto *I : RD->fields()) {
> if (I->isUnnamedBitfield())
>   continue;
>
> instead of embedding this into some condition.

Note in the begin and end of the for loop body, there is a push_back and 
pop_back action to FieldChain, that's why I have to emembed the new condition 
into the old. (OK now I know large diff context is necessary.)

  ...
for (const auto *I : RD->fields()) {
  const FieldRegion *FR = MrMgr.getFieldRegion(I, R);
  FieldChain.push_back(I);
  T = I->getType();
  if (T->getAsStructureType()) {
if (Find(FR))
  return true;
  } else {
const SVal  = StoreMgr.getBinding(store, loc::MemRegionVal(FR));
if (V.isUndef())
  return true;
  }
  FieldChain.pop_back();
}
  
  ...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146194

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D146178#4199263 , @erichkeane 
wrote:

> After a night of sleeping on it, the use of a AdjustConstraintDepth::Diff and 
> AdjustConstraintDepth::Value feels like a hacky solution to SOMETHING here, 
> though I'm not sure what yet. The depth of a template shouldn't change 
> between the equality and constraint checking, it is usually a property of the 
> specialization level.  I could definitely believe that 
> `getTemplateInstantiationArgs` needs some sort of change here to better 
> detect its own state, but this solution doesn't seem right to me.

I debugged a bit: It isn't correct I think that `FunctionTemplateDecl` doesn't 
pick up its template arguments in `getTemplateInstantiationArgs`.  
Additionally, instead of picking up its `DeclContext`, it probably needs its 
`LexicalDeclContext` as the next step, which I believe fixes the problem (plus 
your change in SemaOverload.cpp)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146141: [ARM] Use FPUKind enum instead of unsigned

[Michael Platings 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 rG60bbf271b568: [ARM][NFC] Use FPUKind enum instead of 
unsigned (authored by michaelplatings).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146141

Files:
  clang/lib/Basic/Targets/ARM.cpp
  clang/lib/Driver/ToolChains/Arch/ARM.cpp
  llvm/include/llvm/MC/MCStreamer.h
  llvm/include/llvm/TargetParser/ARMTargetParser.h
  llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
  llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp
  llvm/lib/Target/ARM/MCTargetDesc/ARMTargetStreamer.cpp
  llvm/lib/TargetParser/ARMTargetParser.cpp
  llvm/unittests/TargetParser/TargetParserTest.cpp

Index: llvm/unittests/TargetParser/TargetParserTest.cpp
===
--- llvm/unittests/TargetParser/TargetParserTest.cpp
+++ llvm/unittests/TargetParser/TargetParserTest.cpp
@@ -120,7 +120,7 @@
   ARM::ArchKind AK = ARM::parseCPUArch(params.CPUName);
   EXPECT_EQ(params.ExpectedArch, ARM::getArchName(AK));
 
-  unsigned FPUKind = ARM::getDefaultFPU(params.CPUName, AK);
+  ARM::FPUKind FPUKind = ARM::getDefaultFPU(params.CPUName, AK);
   EXPECT_EQ(params.ExpectedFPU, ARM::getFPUName(FPUKind));
 
   uint64_t default_extensions = ARM::getDefaultExtensions(params.CPUName, AK);
@@ -765,10 +765,10 @@
 testArchExtDependency(const char *ArchExt,
   const std::initializer_list ) {
   std::vector Features;
-  unsigned FPUID;
+  ARM::FPUKind FPUKind;
 
   if (!ARM::appendArchExtFeatures("", ARM::ArchKind::ARMV8_1MMainline, ArchExt,
-  Features, FPUID))
+  Features, FPUKind))
 return false;
 
   return llvm::all_of(Expected, [&](StringRef Ext) {
Index: llvm/lib/TargetParser/ARMTargetParser.cpp
===
--- llvm/lib/TargetParser/ARMTargetParser.cpp
+++ llvm/lib/TargetParser/ARMTargetParser.cpp
@@ -147,7 +147,8 @@
   return getProfileKind(parseArch(Arch));
 }
 
-bool ARM::getFPUFeatures(unsigned FPUKind, std::vector ) {
+bool ARM::getFPUFeatures(ARM::FPUKind FPUKind,
+ std::vector ) {
 
   if (FPUKind >= FK_LAST || FPUKind == FK_INVALID)
 return false;
@@ -211,7 +212,7 @@
   return true;
 }
 
-unsigned ARM::parseFPU(StringRef FPU) {
+ARM::FPUKind ARM::parseFPU(StringRef FPU) {
   StringRef Syn = getFPUSynonym(FPU);
   for (const auto  : FPUNames) {
 if (Syn == F.Name)
@@ -220,7 +221,7 @@
   return FK_INVALID;
 }
 
-ARM::NeonSupportLevel ARM::getFPUNeonSupportLevel(unsigned FPUKind) {
+ARM::NeonSupportLevel ARM::getFPUNeonSupportLevel(ARM::FPUKind FPUKind) {
   if (FPUKind >= FK_LAST)
 return NeonSupportLevel::None;
   return FPUNames[FPUKind].NeonSupport;
@@ -243,33 +244,33 @@
   .Default(FPU);
 }
 
-StringRef ARM::getFPUName(unsigned FPUKind) {
+StringRef ARM::getFPUName(ARM::FPUKind FPUKind) {
   if (FPUKind >= FK_LAST)
 return StringRef();
   return FPUNames[FPUKind].Name;
 }
 
-ARM::FPUVersion ARM::getFPUVersion(unsigned FPUKind) {
+ARM::FPUVersion ARM::getFPUVersion(ARM::FPUKind FPUKind) {
   if (FPUKind >= FK_LAST)
 return FPUVersion::NONE;
   return FPUNames[FPUKind].FPUVer;
 }
 
-ARM::FPURestriction ARM::getFPURestriction(unsigned FPUKind) {
+ARM::FPURestriction ARM::getFPURestriction(ARM::FPUKind FPUKind) {
   if (FPUKind >= FK_LAST)
 return FPURestriction::None;
   return FPUNames[FPUKind].Restriction;
 }
 
-unsigned ARM::getDefaultFPU(StringRef CPU, ARM::ArchKind AK) {
+ARM::FPUKind ARM::getDefaultFPU(StringRef CPU, ARM::ArchKind AK) {
   if (CPU == "generic")
 return ARM::ARMArchNames[static_cast(AK)].DefaultFPU;
 
-  return StringSwitch(CPU)
+  return StringSwitch(CPU)
 #define ARM_CPU_NAME(NAME, ID, DEFAULT_FPU, IS_DEFAULT, DEFAULT_EXT)   \
   .Case(NAME, DEFAULT_FPU)
 #include "llvm/TargetParser/ARMTargetParser.def"
-   .Default(ARM::FK_INVALID);
+  .Default(ARM::FK_INVALID);
 }
 
 uint64_t ARM::getDefaultExtensions(StringRef CPU, ARM::ArchKind AK) {
@@ -362,7 +363,7 @@
   return StringRef();
 }
 
-static unsigned findDoublePrecisionFPU(unsigned InputFPUKind) {
+static ARM::FPUKind findDoublePrecisionFPU(ARM::FPUKind InputFPUKind) {
   const ARM::FPUName  = ARM::FPUNames[InputFPUKind];
 
   // If the input FPU already supports double-precision, then there
@@ -394,7 +395,7 @@
 bool ARM::appendArchExtFeatures(StringRef CPU, ARM::ArchKind AK,
 StringRef ArchExt,
 std::vector ,
-unsigned ) {
+ARM::FPUKind ) {
 
   size_t StartingNumFeatures = Features.size();
   const bool Negated = stripNegationPrefix(ArchExt);
@@ -417,7 +418,7 @@
 CPU = "generic";
 
   if (ArchExt == "fp" || ArchExt == "fp.dp") {
-unsigned 

[clang] 60bbf27 - [ARM][NFC] Use FPUKind enum instead of unsigned

[Michael Platings via cfe-commits]

Author: Michael Platings
Date: 2023-03-16T13:38:10Z
New Revision: 60bbf271b568ab85ef90c92ff6014e06786217b9

URL: 
https://github.com/llvm/llvm-project/commit/60bbf271b568ab85ef90c92ff6014e06786217b9
DIFF: 
https://github.com/llvm/llvm-project/commit/60bbf271b568ab85ef90c92ff6014e06786217b9.diff

LOG: [ARM][NFC] Use FPUKind enum instead of unsigned

Also rename some FPUID variables to FPUKind now it's clear that's what
they are.

Differential Revision: https://reviews.llvm.org/D146141

Added: 


Modified: 
clang/lib/Basic/Targets/ARM.cpp
clang/lib/Driver/ToolChains/Arch/ARM.cpp
llvm/include/llvm/MC/MCStreamer.h
llvm/include/llvm/TargetParser/ARMTargetParser.h
llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp
llvm/lib/Target/ARM/MCTargetDesc/ARMTargetStreamer.cpp
llvm/lib/TargetParser/ARMTargetParser.cpp
llvm/unittests/TargetParser/TargetParserTest.cpp

Removed: 




diff  --git a/clang/lib/Basic/Targets/ARM.cpp b/clang/lib/Basic/Targets/ARM.cpp
index b85d5dc2d3478..e01379ec82fb4 100644
--- a/clang/lib/Basic/Targets/ARM.cpp
+++ b/clang/lib/Basic/Targets/ARM.cpp
@@ -452,7 +452,7 @@ bool ARMTargetInfo::initFeatureMap(
   }
 
   // get default FPU features
-  unsigned FPUKind = llvm::ARM::getDefaultFPU(CPU, Arch);
+  llvm::ARM::FPUKind FPUKind = llvm::ARM::getDefaultFPU(CPU, Arch);
   llvm::ARM::getFPUFeatures(FPUKind, TargetFeatures);
 
   // get default Extension features

diff  --git a/clang/lib/Driver/ToolChains/Arch/ARM.cpp 
b/clang/lib/Driver/ToolChains/Arch/ARM.cpp
index e75f8a474410c..7843031a4c22f 100644
--- a/clang/lib/Driver/ToolChains/Arch/ARM.cpp
+++ b/clang/lib/Driver/ToolChains/Arch/ARM.cpp
@@ -72,25 +72,25 @@ static void getARMHWDivFeatures(const Driver , const Arg 
*A,
 }
 
 // Handle -mfpu=.
-static unsigned getARMFPUFeatures(const Driver , const Arg *A,
-  const ArgList , StringRef FPU,
-  std::vector ) {
-  unsigned FPUID = llvm::ARM::parseFPU(FPU);
-  if (!llvm::ARM::getFPUFeatures(FPUID, Features))
+static llvm::ARM::FPUKind getARMFPUFeatures(const Driver , const Arg *A,
+const ArgList , StringRef FPU,
+std::vector ) {
+  llvm::ARM::FPUKind FPUKind = llvm::ARM::parseFPU(FPU);
+  if (!llvm::ARM::getFPUFeatures(FPUKind, Features))
 D.Diag(clang::diag::err_drv_clang_unsupported) << A->getAsString(Args);
-  return FPUID;
+  return FPUKind;
 }
 
 // Decode ARM features from string like +[no]featureA+[no]featureB+...
 static bool DecodeARMFeatures(const Driver , StringRef text, StringRef CPU,
   llvm::ARM::ArchKind ArchKind,
   std::vector ,
-  unsigned ) {
+  llvm::ARM::FPUKind ) {
   SmallVector Split;
   text.split(Split, StringRef("+"), -1, false);
 
   for (StringRef Feature : Split) {
-if (!appendArchExtFeatures(CPU, ArchKind, Feature, Features, ArgFPUID))
+if (!appendArchExtFeatures(CPU, ArchKind, Feature, Features, ArgFPUKind))
   return false;
   }
   return true;
@@ -112,14 +112,16 @@ static void DecodeARMFeaturesFromCPU(const Driver , 
StringRef CPU,
 static void checkARMArchName(const Driver , const Arg *A, const ArgList 
,
  llvm::StringRef ArchName, llvm::StringRef CPUName,
  std::vector ,
- const llvm::Triple , unsigned ) {
+ const llvm::Triple ,
+ llvm::ARM::FPUKind ) {
   std::pair Split = ArchName.split("+");
 
   std::string MArch = arm::getARMArch(ArchName, Triple);
   llvm::ARM::ArchKind ArchKind = llvm::ARM::parseArch(MArch);
   if (ArchKind == llvm::ARM::ArchKind::INVALID ||
-  (Split.second.size() && !DecodeARMFeatures(D, Split.second, CPUName,
- ArchKind, Features, 
ArgFPUID)))
+  (Split.second.size() &&
+   !DecodeARMFeatures(D, Split.second, CPUName, ArchKind, Features,
+  ArgFPUKind)))
 D.Diag(clang::diag::err_drv_unsupported_option_argument)
 << A->getSpelling() << A->getValue();
 }
@@ -128,15 +130,16 @@ static void checkARMArchName(const Driver , const Arg 
*A, const ArgList ,
 static void checkARMCPUName(const Driver , const Arg *A, const ArgList ,
 llvm::StringRef CPUName, llvm::StringRef ArchName,
 std::vector ,
-const llvm::Triple , unsigned ) {
+const llvm::Triple ,
+llvm::ARM::FPUKind ) {
   std::pair Split = CPUName.split("+");
 
   std::string CPU = arm::getARMTargetCPU(CPUName, ArchName, Triple);
   llvm::ARM::ArchKind ArchKind =
 

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

[Alexander Shaposhnikov via Phabricator via cfe-commits]

alexander-shaposhnikov added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:779
+// The depths calculated for the declarations can be equal but they still
+// may differ from the depths of types inside OldConstr and NewConstr.
+OldConstr =

erichkeane wrote:
> I don't believe they are supposed to, at least based on what you've said 
> here.  Can you clarify what you mean?
i think I need to better understand what's going on here, because this is the 
culprit:
the calculated Depth1 and Depth2 are equal to 1 on our reproducer, thus no 
adjustments happen (in the old code), yet in the OldConst and NewConstr the 
depths are different.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

[Alexander Shaposhnikov via Phabricator via cfe-commits]

alexander-shaposhnikov added a comment.

@erichkeane , ok, let me think a bit a more about it / investigate further. 
It's unclear why we use AdjustConstraintDepth here in the first place, i.e. why 
the depth is incorrect in the first place. Regarding Diff/Value - TemplateDepth 
(despite its name) currently (without this diff) is used as an addend (i.e. 
AdjustConstraintDepth doesn't set depths to a given value but replaces  Depth 
with Depth +  TemplateDepth)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D144878: __builtin_FILE_NAME()

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM aside from a small formatting issue. Do you need someone to land this on 
your behalf? If so, what name and email address would you like used for patch 
attribution?




Comment at: clang/lib/Parse/ParseExpr.cpp:805-807
+/// [C++11] simple-type-specifier braced-init-list [C++11 5.2.3]
 /// [C++]   typename-specifier '(' expression-list[opt] ')' [C++ 5.2.3]
+/// [C++11] typename-specifier braced-init-list [C++11 5.2.3]

Formatting here also got messed up a bit.


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

https://reviews.llvm.org/D144878

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

[PATCH] D146164: Fix nomerge attribute not working with __builtin_trap().

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

Since we're touching SelectionDAG, does GlobalISel also need updating?




Comment at: clang/test/CodeGen/attr-nomerge.cpp:44
+
+  [[clang::nomerge]] __builtin_trap();
 }

Maybe do __debugbreak() too since that's also mentioned on the debug.



Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6853
   }
+  DAG.addNoMergeSiteInfo(Node.getNode(), I.hasFnAttr(Attribute::NoMerge));
+  DAG.setRoot(Node);

(I guess you could have used `DAG.getRoot()` instead of creating the `Node` 
variable.)



Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6856
   return;
 }
 TargetLowering::ArgListTy Args;

Do we need to handle this "else" branch too?

Or would it make sense to do the nomerge check in the caller of 
`visitIntrinsicCall` instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146164

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

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

[Andrew Gozillon via Phabricator via cfe-commits]

agozillon added inline comments.



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:128
+  if (IsHostOffloadingAction) {
+for (size_t i = 1; i < Inputs.size(); ++i) {
+  if (Inputs[i].getType() != types::TY_Nothing)

awarzynski wrote:
> What's the magic "1"? And given that the input count matters here - is there 
> a test with multiple inputs?
It aims to mimic the behavior of Clang: 
https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L4561
 where the main input is skipped (the input currently being compiled or 
embedded into etc.), when adding to //-fembed-offload-object//. 

It does look different to Clang's as Clang has more cases and the logic is 
spread across the constructJob invocation, but the first if case is what the if 
statement inside of the loop and setting the loop index variable to 1 do. The 
HostOffloadingInputs array is what is being generated here, except we're 
skipping and directly applying it as arguments.

I tried to condense it a little in this case! Perhaps it loses readability 
though, I had hoped the comment might have kept it clear



Comment at: clang/test/Driver/flang/flang-omp.f90:6
+! Test regular -fopenmp with no offload
+! RUN: %clang --driver-mode=flang -### -fopenmp %s 2>&1 | FileCheck 
--check-prefixes=CHECK-OPENMP %s
+! CHECK-OPENMP: "{{[^"]*}}flang-new" "-fc1" {{.*}} "-fopenmp" {{.*}}.f90"

awarzynski wrote:
> Can you remind me why isn't it possible to test this with `flang-new`? 
I double checked, it seems possible to test these with flang-new directly, the 
main reason I've tested it like this is as it's based on the other tests in the 
same directory which use clang! 

However, I'm more than happy to move these tests to the 
omp-frontend-forwarding.f90 test, remove them or keep these and add flang-new 
variations into omp-frontend-forwarding.f90. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145815

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

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

These changes will also need a release note at some point.




Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:662-665
+def err_type_definition_cannot_be_modified : Error<
+  "%0 type definition cannot be modified inside a scope containing "
+  "'#pragma clang fp eval_method(%1)' when a command line "
+  "option 'ffp-eval-method=%2' is used">;

I think we should add some documentation as to why this error happens. Naively, 
users expect the pragma to override the command line (the command line is the 
"default" and the pragma overrides that in the cases the default is wrong), so 
I'd imagine a user getting this error would ask, "But why?"



Comment at: clang/lib/Parse/ParseStmt.cpp:1122-1123
+return "source";
+  default:
+llvm_unreachable("unexpected eval method value");
+  }

Does something assert that we never try to call this with `FEM_Indeterminable`?



Comment at: clang/lib/Parse/ParseStmt.cpp:1198-1210
+if (Tok.is(tok::identifier)) {
+  if (Tok.getIdentifierInfo()->getName().str() == "float_t" ||
+  Tok.getIdentifierInfo()->getName().str() == "double_t") {
+if (getLangOpts().getFPEvalMethod() !=
+LangOptions::FPEvalMethodKind::FEM_UnsetOnCommandLine &&
+PP.getLastFPEvalPragmaLocation().isValid() &&
+PP.getCurrentFPEvalMethod() != getLangOpts().getFPEvalMethod())

This is not correct -- it's only going to catch the case where the user 
literally writes `float_t` as the first token in a statement, so it's going to 
miss `const float_t t` and `typedef float_t frobble; frobble t;` etc. Also, 
this seems like it will impact compile time overhead because that's going to be 
performing a lot of useless string comparisons.

This cannot be handled at the level of the parser, it needs to be handled 
within Sema. I think you'll want it to live somewhere around 
`GetFullTypeForDeclarator()` in SemaType.cpp.

There are other situations that should be tested:
```
char buffer[sizeof(float_t)]; // This seems like something we'd want to prevent?
typedef float_t foo; // This seems like something that's harmless to accept?
using quux = float_t; // Same here as above
foo bar; // This, however, is a problem

extern float_t blah(); // This also seems bad

_Generic(1, float_t : 0); // Maybe a bad thing?

auto lam = [](float_t f) { return f; }; // Bad?

float f = (float_t)12; // What about this? Note, the float_t is only used in 
the cast...
```




Comment at: clang/test/CodeGen/abi-check-1.c:1-2
+// RUN: %clang_cc1  -isystem %S/Inputs -triple x86_64-linux-gnu \
+// RUN: -emit-llvm -o - %s | FileCheck %s
+

Why are these codegen tests when the diagnostic (should be) in Sema?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146148

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

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

After a night of sleeping on it, the use of a AdjustConstraintDepth::Diff and 
AdjustConstraintDepth::Value feels like a hacky solution to SOMETHING here, 
though I'm not sure what yet. The depth of a template shouldn't change between 
the equality and constraint checking, it is usually a property of the 
specialization level.  I could definitely believe that 
`getTemplateInstantiationArgs` needs some sort of change here to better detect 
its own state, but this solution doesn't seem right to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146178

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

[PATCH] D146194: NOT report the warning that unnamed bitfields are uninitialized.

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

BTW we request full context for git patches. That way the reviewer can scroll 
and see the context around your hunks.
You can request git to emit the full context by passing the `-U99` 
commandline option. See 
.

As per your proposed change, I think we should do something like this:

  for (const auto *I : RD->fields()) {
if (I->isUnnamedBitfield())
  continue;

instead of embedding this into some condition.

But I'm curious to see why does C behave differently.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146194

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

[PATCH] D146194: NOT report the warning that unnamed bitfields are uninitialized.

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

Okay, thanks for clarifying.

So, it seems like it depends on the language flavor, but why does CSA behave 
differently in C and C++?
This is necessary to understand if we need to fix that instead of patching this 
particular checker.

I'd still add a test to the `clang/test/Analysis/unnamedBitfields.c`, something 
like this:

  // RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s -x c
  // RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s -x c++
  
  typedef struct {
unsigned x : 3;
unsigned : 29; 
unsigned y;
  } A;
  void take_by_value(A a);
  
  void initlistExpr(void) {
A a = {0}; // zero-initializes all fields
take_by_value(a); // no-warning
  }
  
  void defaultConstruct(void) {
A a; // uninitialized
a.x = 0;
take_by_value(a); // expected-warning{{Passed-by-value struct argument 
contains uninitialized data (e.g., field: 'y')}}
  }



---

FYI you can add syntax highlighting for your snippets here by:

  ```lang=C++


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146194

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

[PATCH] D146089: [Sema] Fix null pointer dereference handleAlwaysInlineAttr.

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: clang/test/Sema/attr-alwaysinline.cpp:36
+return x;
+}
+

craig.topper wrote:
> erichkeane wrote:
> > craig.topper wrote:
> > > erichkeane wrote:
> > > > erichkeane wrote:
> > > > > craig.topper wrote:
> > > > > > erichkeane wrote:
> > > > > > > craig.topper wrote:
> > > > > > > > erichkeane wrote:
> > > > > > > > > Can you add a test that shows that we warn on instantiation?  
> > > > > > > > > This shouldn't be a dependent declrefexpr when instantiated.
> > > > > > > > > 
> > > > > > > > > Additionally, this would make sure that we're properly 
> > > > > > > > > propoagating `always_inline`.
> > > > > > > > Should this warn
> > > > > > > > 
> > > > > > > > ```
> > > > > > > > template [[gnu::noinline]]   
> > > > > > > >  
> > > > > > > > int bar(int x) {
> > > > > > > >  
> > > > > > > > if constexpr (D > 1)
> > > > > > > >  
> > > > > > > > [[clang::always_inline]] return bar(x + 1);
> > > > > > > >  
> > > > > > > > else
> > > > > > > >  
> > > > > > > > return x;   
> > > > > > > >  
> > > > > > > > }   
> > > > > > > >  
> > > > > > > > 
> > > > > > > >  
> > > > > > > > int baz(int x) {
> > > > > > > >  
> > > > > > > >   return bar<5>(x); 
> > > > > > > >  
> > > > > > > > }  
> > > > > > > > ```
> > > > > > > Yes, I would expect that to warn.
> > > > > > It looks like handleAlwaysInlineAttr only gets called once so it 
> > > > > > doesn't get called after instantiation.
> > > > > Hmm... thats unfortunate.  That means we're perhaps not instantiating 
> > > > > it correctly.  I'll take some time to poke around as I get a chacne.
> > > > Ok, it looks like SemaStmt.cpp's `BuildAttributedStmt` needs to handle 
> > > > the attributes.  We should some day do that automatically for a 
> > > > majority o f attributes, but for now, just adding the call there is 
> > > > sufficient.
> > > What I do need to add?
> > You should be able to follow what is already there, but I suspect you just 
> > need to call some sort of 'handle' function for those to do this.  There is 
> > one there already youc an crib off of.
> Can I do that as a separate patch? Fixing the crash seems like a net 
> improvement.
I'd prefer they be done together: They are a situation where you cannot 
properly test either patch without the other one being fixed, so I believe they 
are intrinsically linked.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146089

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

[PATCH] D145088: [RISCV] Add attribute(riscv_rvv_vector_bits(N)) based on AArch64 arm_sve_vector_bits.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/AST/ItaniumMangle.cpp:3897-3899
+  } else if (T->getVectorKind() == VectorType::RVVFixedLengthDataVector) {
+mangleRISCVFixedRVVVectorType(T);
+return;

craig.topper wrote:
> aaron.ballman wrote:
> > Should there be corresponding changes to the Microsoft mangler as well?
> Good question. I don't see the equivalent SVE handling in the Microsoft 
> mangler.
I'm fine if you want to address that issue in a follow-up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145088

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

[PATCH] D131307: [Clang] Allow downgrading to a warning the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D131307#4197789 , @erichkeane 
wrote:

> In D131307#4197767 , @rsmith wrote:
>
>>> Based on feedback we will provide users to the ability to downgrade this 
>>> this diagnostic to a waring to allow for a transition period. We expect to 
>>> turn this diagnostic to an error in the next release.
>>
>> Can we revert this now?
>
> Seems appropriate.  Aaron, WDYT?

I think it's fine -- if we get significant user feedback after 16 goes out to 
the public which suggests we need to retain this for longer, it's easy enough 
to bring back from version control.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131307

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

[PATCH] D146187: [docs] Update the status for coroutines

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Should we also be updating InitPreprocessor.cpp at the same time, for 
non-Windows targets? I see we define:

  // TS features.
  if (LangOpts.Coroutines)
Builder.defineMacro("__cpp_coroutines", "201703L");

but we don't define `__cpp_­impl_­coroutine`: 
http://eel.is/c++draft/tab:cpp.predefined.ft

And this should definitely come with a release note so users know about the 
change in status.




Comment at: clang/www/cxx_status.html:1225
 Partial
-  The optimizer does not yet handle TLS with
-  __attribute__((const)) attribute correctly. There can be issues 
where the
-  coroutine may resume on a different thread. This feature requires 
further
-  analysis of the C++ Standard to determine what work is necessary for 
conformance.
+  It's supported fully everywhere but on Windows targets.
 




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146187

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

[PATCH] D146202: [clang] Fix a UsingTemplate regression after 3e78fa860235431323aaf08c8fa922d75a7cfffa

[Haojian Wu 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 rG1f5fdc22a26c: [clang] Fix a UsingTemplate regression after… 
(authored by hokein).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146202

Files:
  clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
  clang/lib/AST/TemplateName.cpp
  clang/test/AST/ast-dump-using-template.cpp


Index: clang/test/AST/ast-dump-using-template.cpp
===
--- clang/test/AST/ast-dump-using-template.cpp
+++ clang/test/AST/ast-dump-using-template.cpp
@@ -9,8 +9,11 @@
  public:
S(T);
 };
+template struct S2 { S2(T); };
+template  S2(T t) -> S2;
 }
 using ns::S;
+using ns::S2;
 
 // TemplateName in TemplateSpecializationType.
 template
@@ -36,3 +39,10 @@
 // CHECK-NEXT:  |-DeclRefExpr {{.*}}
 // CHECK-NEXT:  `-ElaboratedType {{.*}} 'S' sugar
 // CHECK-NEXT:`-DeducedTemplateSpecializationType {{.*}} 'ns::S' 
sugar using
+
+S2 DeducedTemplateSpecializationT2(123);
+using D = decltype(DeducedTemplateSpecializationT2);
+// CHECK:  DecltypeType {{.*}}
+// CHECK-NEXT:  |-DeclRefExpr {{.*}}
+// CHECK-NEXT:  `-ElaboratedType {{.*}} 'S2' sugar
+// CHECK-NEXT:`-DeducedTemplateSpecializationType {{.*}} 'S2' sugar 
using
Index: clang/lib/AST/TemplateName.cpp
===
--- clang/lib/AST/TemplateName.cpp
+++ clang/lib/AST/TemplateName.cpp
@@ -282,7 +282,9 @@
 }
 
 void TemplateName::Profile(llvm::FoldingSetNodeID ) {
-  if (auto *TD = getAsTemplateDecl())
+  if (const auto* USD = getAsUsingShadowDecl())
+ID.AddPointer(USD->getCanonicalDecl());
+  else if (const auto *TD = getAsTemplateDecl())
 ID.AddPointer(TD->getCanonicalDecl());
   else
 ID.AddPointer(Storage.getOpaqueValue());
Index: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
===
--- clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
+++ clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
@@ -171,6 +171,13 @@
   namespace ns {template  struct S {}; }
   using ns::$explicit^S;)cpp",
"^S x;");
+  testWalk(R"cpp(
+  namespace ns {
+template  struct S { S(T);};
+template  S(T t) -> S;
+  }
+  using ns::$explicit^S;)cpp",
+  "^S x(123);");
   testWalk("template struct $explicit^S {};",
R"cpp(
   template  typename> struct X {};


Index: clang/test/AST/ast-dump-using-template.cpp
===
--- clang/test/AST/ast-dump-using-template.cpp
+++ clang/test/AST/ast-dump-using-template.cpp
@@ -9,8 +9,11 @@
  public:
S(T);
 };
+template struct S2 { S2(T); };
+template  S2(T t) -> S2;
 }
 using ns::S;
+using ns::S2;
 
 // TemplateName in TemplateSpecializationType.
 template
@@ -36,3 +39,10 @@
 // CHECK-NEXT:  |-DeclRefExpr {{.*}}
 // CHECK-NEXT:  `-ElaboratedType {{.*}} 'S' sugar
 // CHECK-NEXT:`-DeducedTemplateSpecializationType {{.*}} 'ns::S' sugar using
+
+S2 DeducedTemplateSpecializationT2(123);
+using D = decltype(DeducedTemplateSpecializationT2);
+// CHECK:  DecltypeType {{.*}}
+// CHECK-NEXT:  |-DeclRefExpr {{.*}}
+// CHECK-NEXT:  `-ElaboratedType {{.*}} 'S2' sugar
+// CHECK-NEXT:`-DeducedTemplateSpecializationType {{.*}} 'S2' sugar using
Index: clang/lib/AST/TemplateName.cpp
===
--- clang/lib/AST/TemplateName.cpp
+++ clang/lib/AST/TemplateName.cpp
@@ -282,7 +282,9 @@
 }
 
 void TemplateName::Profile(llvm::FoldingSetNodeID ) {
-  if (auto *TD = getAsTemplateDecl())
+  if (const auto* USD = getAsUsingShadowDecl())
+ID.AddPointer(USD->getCanonicalDecl());
+  else if (const auto *TD = getAsTemplateDecl())
 ID.AddPointer(TD->getCanonicalDecl());
   else
 ID.AddPointer(Storage.getOpaqueValue());
Index: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
===
--- clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
+++ clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
@@ -171,6 +171,13 @@
   namespace ns {template  struct S {}; }
   using ns::$explicit^S;)cpp",
"^S x;");
+  testWalk(R"cpp(
+  namespace ns {
+template  struct S { S(T);};
+template  S(T t) -> S;
+  }
+  using ns::$explicit^S;)cpp",
+  "^S x(123);");
   testWalk("template struct $explicit^S {};",
R"cpp(
   template  typename> struct X {};
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] 1f5fdc2 - [clang] Fix a UsingTemplate regression after 3e78fa860235431323aaf08c8fa922d75a7cfffa

[Haojian Wu via cfe-commits]

Author: Haojian Wu
Date: 2023-03-16T12:59:26+01:00
New Revision: 1f5fdc22a26c46a11ce406b745291b3c03bc67e8

URL: 
https://github.com/llvm/llvm-project/commit/1f5fdc22a26c46a11ce406b745291b3c03bc67e8
DIFF: 
https://github.com/llvm/llvm-project/commit/1f5fdc22a26c46a11ce406b745291b3c03bc67e8.diff

LOG: [clang] Fix a UsingTemplate regression after 
3e78fa860235431323aaf08c8fa922d75a7cfffa

TemplateName::getAsTemplateDecl() returns the underlying TemplateDecl
for a UsingTemplate kind template name. We should respect that in the
Profile method otherwise we might desugar the template name unexpectedly
(e.g. for template argument deduction with deduciton guides).

Differential Revision: https://reviews.llvm.org/D146202

Added: 


Modified: 
clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
clang/lib/AST/TemplateName.cpp
clang/test/AST/ast-dump-using-template.cpp

Removed: 




diff  --git a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp 
b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
index 7dcbf475e9869..68b6b217a2e01 100644
--- a/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
+++ b/clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
@@ -171,6 +171,13 @@ TEST(WalkAST, TemplateNames) {
   namespace ns {template  struct S {}; }
   using ns::$explicit^S;)cpp",
"^S x;");
+  testWalk(R"cpp(
+  namespace ns {
+template  struct S { S(T);};
+template  S(T t) -> S;
+  }
+  using ns::$explicit^S;)cpp",
+  "^S x(123);");
   testWalk("template struct $explicit^S {};",
R"cpp(
   template  typename> struct X {};

diff  --git a/clang/lib/AST/TemplateName.cpp b/clang/lib/AST/TemplateName.cpp
index 76f507ba26fe4..2f0e4181e9408 100644
--- a/clang/lib/AST/TemplateName.cpp
+++ b/clang/lib/AST/TemplateName.cpp
@@ -282,7 +282,9 @@ bool TemplateName::containsUnexpandedParameterPack() const {
 }
 
 void TemplateName::Profile(llvm::FoldingSetNodeID ) {
-  if (auto *TD = getAsTemplateDecl())
+  if (const auto* USD = getAsUsingShadowDecl())
+ID.AddPointer(USD->getCanonicalDecl());
+  else if (const auto *TD = getAsTemplateDecl())
 ID.AddPointer(TD->getCanonicalDecl());
   else
 ID.AddPointer(Storage.getOpaqueValue());

diff  --git a/clang/test/AST/ast-dump-using-template.cpp 
b/clang/test/AST/ast-dump-using-template.cpp
index da18f0499f54d..de3ce277fd24f 100644
--- a/clang/test/AST/ast-dump-using-template.cpp
+++ b/clang/test/AST/ast-dump-using-template.cpp
@@ -9,8 +9,11 @@ template class S {
  public:
S(T);
 };
+template struct S2 { S2(T); };
+template  S2(T t) -> S2;
 }
 using ns::S;
+using ns::S2;
 
 // TemplateName in TemplateSpecializationType.
 template
@@ -36,3 +39,10 @@ using C = decltype(DeducedTemplateSpecializationT);
 // CHECK-NEXT:  |-DeclRefExpr {{.*}}
 // CHECK-NEXT:  `-ElaboratedType {{.*}} 'S' sugar
 // CHECK-NEXT:`-DeducedTemplateSpecializationType {{.*}} 'ns::S' 
sugar using
+
+S2 DeducedTemplateSpecializationT2(123);
+using D = decltype(DeducedTemplateSpecializationT2);
+// CHECK:  DecltypeType {{.*}}
+// CHECK-NEXT:  |-DeclRefExpr {{.*}}
+// CHECK-NEXT:  `-ElaboratedType {{.*}} 'S2' sugar
+// CHECK-NEXT:`-DeducedTemplateSpecializationType {{.*}} 'S2' sugar 
using



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

[PATCH] D146202: [clang] Fix a UsingTemplate regression after 3e78fa860235431323aaf08c8fa922d75a7cfffa

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for the review.

In D146202#4198904 , @ChuanqiXu wrote:

> Yeah, this one should be correct. Modules techniques use `*::Profile` method 
> to judge if two entities are the same extensively. So it would be best to add 
> a test case for modules like we did in 
> https://reviews.llvm.org/rG3e78fa860235431323aaf08c8fa922d75a7cfffa. And it 
> doesn't matter if you are not interested, I'll take it later then.

I'm landing the patch as-is now. It would be nice for you to add a related test 
for modules afterwards (since I don't have much knowledge about clang module 
stuff).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146202

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