[PATCH] D77982: [Windows SEH] Fix the frame-ptr of a nested-filter within a _finally

[Ten Tzen via Phabricator via cfe-commits]

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



Comment at: clang/lib/CodeGen/CGException.cpp:1798
+
+// if the parent is a _finally, need to retrive Establisher's FP,
+//  2nd paramenter, saved & named frame_pointer in parent's frame

efriedma wrote:
> Maybe worth expanding this comment a little, to explain that a "finally" 
> should have at most one localescape'ed variable.  (At least, that's my 
> understanding.)
ok, will add more explanation.



Comment at: clang/lib/CodeGen/CGException.cpp:1800
+//  2nd paramenter, saved & named frame_pointer in parent's frame
+if (ParentCGF.ParentCGF != NULL) {
+  // Locate and escape Parent's frame_pointer.addr alloca

efriedma wrote:
> nullptr
> 
> Do you actually use ParentCGF.ParentCGF anywhere?  If not, do you really need 
> to save it?
Yes, I used it in other patches.  I found it's very handy to have a way to 
access parent CGF & Function.



Comment at: clang/lib/CodeGen/CGException.cpp:1805
+llvm::AllocaInst *II = dyn_cast(&I);
+if (II && II->getName().startswith("frame_pointer")) {
+  FramePtrAddrAlloca = II;

efriedma wrote:
> Using the name isn't reliable. You should be using data stored somewhere in 
> the CodeGenFunction or something like that.
good sense.  will update it.



Comment at: clang/lib/CodeGen/CGException.cpp:1822
+  llvm::Function *FrameRecoverFn = llvm::Intrinsic::getDeclaration(
+  &CGM.getModule(), llvm::Intrinsic::localrecover);
+  llvm::Constant *ParentI8Fn =

efriedma wrote:
> Is there some reason you can't reuse recoverAddrOfEscapedLocal here?
because recoverAddrOfEscapedLocal() is used to get escaped locals of outermost 
frame.  The escaped frame-pointer we are looking for here is in _finally frame, 
not outermost frame.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77982



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

[PATCH] D77984: Make IRBuilder automatically set alignment on load/store/alloca.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Thanks, this is indeed much better and the direction again in-line with what 
i've raised in some attributor patch.
LG to me in general, but please wait for someone else to take a look, too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77984



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

[PATCH] D75034: [clang-format] use spaces for alignment with UT_ForContinuationAndIndentation

[Maximilian Fickert via Phabricator via cfe-commits]

fickert updated this revision to Diff 256938.
fickert added a comment.

rebased, fixed two unit tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75034

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/BreakableToken.cpp
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/ContinuationIndenter.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/lib/Format/WhitespaceManager.cpp
  clang/lib/Format/WhitespaceManager.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -10133,17 +10133,18 @@
" \t \tcomment */",
Tab));
   EXPECT_EQ("{\n"
-"  /*\n"
-"   * Comment\n"
-"   */\n"
-"  int i;\n"
+"/*\n"
+" * Comment\n"
+" */\n"
+"int i;\n"
 "}",
 format("{\n"
"\t/*\n"
"\t * Comment\n"
"\t */\n"
"\t int i;\n"
-   "}"));
+   "}",
+   Tab));
 
   Tab.UseTab = FormatStyle::UT_ForContinuationAndIndentation;
   Tab.TabWidth = 8;
@@ -10314,15 +10315,245 @@
"\t*/\n"
"}",
Tab));
+  EXPECT_EQ("/* some\n"
+"   comment */",
+format(" \t \t /* some\n"
+   " \t \tcomment */",
+   Tab));
+  EXPECT_EQ("int a; /* some\n"
+"   comment */",
+format(" \t \t int a; /* some\n"
+   " \t \tcomment */",
+   Tab));
+  EXPECT_EQ("int a; /* some\n"
+"comment */",
+format(" \t \t int\ta; /* some\n"
+   " \t \tcomment */",
+   Tab));
+  EXPECT_EQ("f(\"\t\t\"); /* some\n"
+"comment */",
+format(" \t \t f(\"\t\t\"); /* some\n"
+   " \t \tcomment */",
+   Tab));
+  EXPECT_EQ("{\n"
+"\t/*\n"
+"\t * Comment\n"
+"\t */\n"
+"\tint i;\n"
+"}",
+format("{\n"
+   "\t/*\n"
+   "\t * Comment\n"
+   "\t */\n"
+   "\t int i;\n"
+   "}",
+   Tab));
+  Tab.TabWidth = 2;
+  Tab.IndentWidth = 2;
+  EXPECT_EQ("{\n"
+"\t/* \n"
+"\t\t  */\n"
+"}",
+format("{\n"
+   "/* \n"
+   "\t  */\n"
+   "}",
+   Tab));
+  EXPECT_EQ("{\n"
+"\t/*\n"
+"\t\taa\n"
+"\t\tb\n"
+"\t*/\n"
+"}",
+format("{\n"
+   "/*\n"
+   "\taa b\n"
+   "*/\n"
+   "}",
+   Tab));
+  Tab.AlignConsecutiveAssignments = true;
+  Tab.AlignConsecutiveDeclarations = true;
+  Tab.TabWidth = 4;
+  Tab.IndentWidth = 4;
+  verifyFormat("class Assign {\n"
+   "\tvoid f() {\n"
+   "\t\tint x  = 123;\n"
+   "\t\tint random = 4;\n"
+   "\t\tstd::string alphabet =\n"
+   "\t\t\t\"abcdefghijklmnopqrstuvwxyz\";\n"
+   "\t}\n"
+   "};",
+   Tab);
+
+  Tab.UseTab = FormatStyle::UT_AlignWithSpaces;
+  Tab.TabWidth = 8;
+  Tab.IndentWidth = 8;
+  EXPECT_EQ("if ( && // q\n"
+"bb) // w\n"
+"\t;",
+format("if ( &&// q\n"
+   "bb)// w\n"
+   ";",
+   Tab));
+  EXPECT_EQ("if (aaa && bbb) // w\n"
+"\t;",
+format("if(aaa&&bbb)// w\n"
+   ";",
+   Tab));
+  verifyFormat("class X {\n"
+   "\tvoid f() {\n"
+   "\t\tsomeFunction(parameter1,\n"
+   "\t\t parameter2);\n"
+   "\t}\n"
+   "};",
+   Tab);
+  verifyFormat("#define A\\\n"
+   "\tvoid f() {   \\\n"
+   "\t\tsomeFunction(\\\n"
+   "\t\tparameter1,  \\\n"
+   "\t\tparameter2); \\\n"
+   "\t}",
+   Tab);
+  Tab.TabWidth = 4;
+  Tab.IndentWidth = 8;
+  verifyFormat("class TabWidth4Indent8 {\n"
+   "\t\tvoid f() {\n"
+   "\t\t\t\tsomeFunction(parameter1,\n"
+   "\t\t\t\t

[PATCH] D77983: clang-tidy doc: add a note for every checker with an autofix

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru updated this revision to Diff 256943.
sylvestre.ledru added a comment.

2 spaces instead of 4


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77983

Files:
  clang-tools-extra/docs/clang-tidy/checks/abseil-duration-addition.rst
  clang-tools-extra/docs/clang-tidy/checks/abseil-duration-comparison.rst
  clang-tools-extra/docs/clang-tidy/checks/abseil-duration-conversion-cast.rst
  clang-tools-extra/docs/clang-tidy/checks/abseil-duration-division.rst
  clang-tools-extra/docs/clang-tidy/checks/abseil-duration-factory-float.rst
  clang-tools-extra/docs/clang-tidy/checks/abseil-duration-factory-scale.rst
  clang-tools-extra/docs/clang-tidy/checks/abseil-duration-subtraction.rst
  
clang-tools-extra/docs/clang-tidy/checks/abseil-duration-unnecessary-conversion.rst
  clang-tools-extra/docs/clang-tidy/checks/abseil-faster-strsplit-delimiter.rst
  clang-tools-extra/docs/clang-tidy/checks/abseil-redundant-strcat-calls.rst
  clang-tools-extra/docs/clang-tidy/checks/abseil-str-cat-append.rst
  clang-tools-extra/docs/clang-tidy/checks/abseil-string-find-startswith.rst
  clang-tools-extra/docs/clang-tidy/checks/abseil-time-comparison.rst
  clang-tools-extra/docs/clang-tidy/checks/abseil-time-subtraction.rst
  
clang-tools-extra/docs/clang-tidy/checks/abseil-upgrade-duration-conversions.rst
  clang-tools-extra/docs/clang-tidy/checks/android-cloexec-accept.rst
  clang-tools-extra/docs/clang-tidy/checks/android-cloexec-creat.rst
  clang-tools-extra/docs/clang-tidy/checks/android-cloexec-dup.rst
  clang-tools-extra/docs/clang-tidy/checks/android-cloexec-pipe.rst
  clang-tools-extra/docs/clang-tidy/checks/boost-use-to-string.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-argument-comment.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-bool-pointer-implicit-conversion.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-copy-constructor-init.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-inaccurate-erase.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-macro-parentheses.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-misplaced-operator-in-strlen-in-alloc.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-misplaced-pointer-arithmetic-in-alloc.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-move-forwarding-reference.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-parent-virtual-call.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-posix-return.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-reserved-identifier.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-string-constructor.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-string-integer-assignment.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-memset-usage.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-semicolon.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-suspicious-string-compare.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-swapped-arguments.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-terminating-continue.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-unused-raii.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-virtual-near-miss.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-dcl03-c.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-dcl16-c.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-dcl37-c.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-dcl51-cpp.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-oop11-cpp.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-explicit-virtual-functions.rst
  clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-init-variables.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-pro-bounds-constant-array-index.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-pro-type-cstyle-cast.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-pro-type-member-init.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-pro-type-static-cast-downcast.rst
  clang-tools-extra/docs/clang-tidy/checks/darwin-dispatch-once-nonstatic.rst
  
clang-tools-extra/docs/clang-tidy/checks/fuchsia-default-arguments-declarations.rst
  clang-tools-extra/docs/clang-tidy/checks/google-explicit-constructor.rst
  
clang-tools-extra/docs/clang-tidy/checks/google-readability-braces-around-statements.rst
  clang-tools-extra/docs/clang-tidy/checks/google-upgrade-googletest-case.rst
  clang-tools-extra/docs/clang-tidy/checks/hicpp-braces-around-statements.rst
  clang-tools-extra/docs/clang-tidy/checks/hicpp-deprecated-headers.rst
  clang-tools-extra/docs/clang-tidy/checks/hicpp-explicit-conversions.rst
  clang-tools-extra/docs/clang-tidy/checks/hicpp-member-init.rst
  clang-tools-extra/docs/clang-tidy/checks/hicpp-move-const

[PATCH] D77982: [Windows SEH] Fix the frame-ptr of a nested-filter within a _finally

[Ten Tzen via Phabricator via cfe-commits]

tentzen updated this revision to Diff 256942.
tentzen added a comment.

Remove hard-code name "frame-pointer". 
get the name from 2nd Arg of the _finally().


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77982

Files:
  clang/lib/CodeGen/CGException.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/test/CodeGen/windows-seh-filter-inFinally.c

Index: clang/test/CodeGen/windows-seh-filter-inFinally.c
===
--- /dev/null
+++ clang/test/CodeGen/windows-seh-filter-inFinally.c
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 -triple x86_64-windows -fms-extensions -Wno-implicit-function-declaration -S -emit-llvm %s -o - | FileCheck %s
+
+// CHECK: %[[dst:[0-9-]+]] = call i8* @llvm.eh.recoverfp(i8* bitcast (void (i8, i8*)* @"?fin$0@0@main@@" to i8*), i8* %frame_pointer)
+// CHECK-NEXT: %[[dst1:[0-9-]+]] = call i8* @llvm.localrecover(i8* bitcast (void (i8, i8*)* @"?fin$0@0@main@@" to i8*), i8* %[[dst]], i32 0)
+// CHECK-NEXT: %[[dst2:[0-9-]+]] = bitcast i8* %[[dst1]] to i8**
+// CHECK-NEXT: = load i8*, i8** %[[dst2]], align 8
+
+int
+main(int argc, char *argv[])
+{
+int Counter = 0;
+//
+// Try/except within the finally clause of a try/finally.
+//
+__try {
+  Counter -= 1;
+}
+__finally {
+  __try {
+Counter += 2;
+// RtlRaiseStatus(STATUS_INTEGER_OVERFLOW);
+  } __except(Counter) {
+__try {
+  Counter += 3;
+}
+__finally {
+  if (abnormal_termination() == 1) {
+Counter += 5;
+  }
+}
+  }
+}
+// expect Counter == 9
+return 1;
+}
+
Index: clang/lib/CodeGen/CodeGenFunction.h
===
--- clang/lib/CodeGen/CodeGenFunction.h
+++ clang/lib/CodeGen/CodeGenFunction.h
@@ -367,6 +367,9 @@
   CodeGenModule &CGM;  // Per-module state.
   const TargetInfo &Target;
 
+  // For EH/SEH outlined funclets, this field points to parent's CGF
+  CodeGenFunction *ParentCGF = nullptr;
+
   typedef std::pair ComplexPairTy;
   LoopInfoStack LoopStack;
   CGBuilderTy Builder;
Index: clang/lib/CodeGen/CGException.cpp
===
--- clang/lib/CodeGen/CGException.cpp
+++ clang/lib/CodeGen/CGException.cpp
@@ -1794,6 +1794,46 @@
 llvm::Constant *ParentI8Fn =
 llvm::ConstantExpr::getBitCast(ParentCGF.CurFn, Int8PtrTy);
 ParentFP = Builder.CreateCall(RecoverFPIntrin, {ParentI8Fn, EntryFP});
+
+// if the parent is a _finally, the passed-in ParentFP is the FP
+// of parent _finally, not Establisher's FP (FP of outermost function).
+// Establkisher FP is 2nd paramenter passed into parent _finally.
+// Fortunately, it's always saved in parent's frame. The following
+// code retrieves it, and escapes it so that spill instruction won't be
+// optimized away.
+if (ParentCGF.ParentCGF != nullptr) {
+  // Locate and escape Parent's frame_pointer.addr alloca
+  llvm::Function *Fn = ParentCGF.CurFn;
+  StringRef Name = Fn->getArg(1)->getName();
+  llvm::AllocaInst *FramePtrAddrAlloca = nullptr;
+  for (llvm::Instruction &I : ParentCGF.CurFn->getEntryBlock()) {
+llvm::AllocaInst *II = dyn_cast(&I);
+if (II && II->getName().startswith(Name)) {
+  FramePtrAddrAlloca = II;
+  break;
+}
+  }
+  assert(FramePtrAddrAlloca);
+  auto InsertPair = ParentCGF.EscapedLocals.insert(
+  std::make_pair(FramePtrAddrAlloca, ParentCGF.EscapedLocals.size()));
+  int FrameEscapeIdx = InsertPair.first->second;
+
+  // an example of a filter's prolog::
+  // %0 = call i8* @llvm.eh.recoverfp(bitcast(@"?fin$0@0@main@@"),..)
+  // %1 = call i8* @llvm.localrecover(bitcast(@"?fin$0@0@main@@"),..)
+  // %2 = bitcast i8* %1 to i8**
+  // %3 = load i8*, i8* *%2, align 8
+  //   ==> %3 is the frame-pointer of outermost host function
+  llvm::Function *FrameRecoverFn = llvm::Intrinsic::getDeclaration(
+  &CGM.getModule(), llvm::Intrinsic::localrecover);
+  llvm::Constant *ParentI8Fn =
+  llvm::ConstantExpr::getBitCast(ParentCGF.CurFn, Int8PtrTy);
+  ParentFP = Builder.CreateCall(
+  FrameRecoverFn, {ParentI8Fn, ParentFP,
+   llvm::ConstantInt::get(Int32Ty, FrameEscapeIdx)});
+  ParentFP = Builder.CreateBitCast(ParentFP, CGM.VoidPtrPtrTy);
+  ParentFP = Builder.CreateLoad(Address(ParentFP, getPointerAlign()));
+}
   }
 
   // Create llvm.localrecover calls for all captures.
@@ -1992,6 +2032,7 @@
 
 void CodeGenFunction::EnterSEHTryStmt(const SEHTryStmt &S) {
   CodeGenFunction HelperCGF(CGM, /*suppressNewContext=*/true);
+  HelperCGF.ParentCGF = this;
   if (const SEHFinallyStmt *Finally = S.getFinallyHandler()) {
 // Outline the finally block.
 llvm::Function *FinallyFunc =
__

[PATCH] D77461: [WIP][clang-tidy] Remove false positive in AvoidNonConstGlobalVariables

[Kim Viggedal via Phabricator via cfe-commits]

vingeldal added a comment.

In D77461#1963166 , @lebedev.ri wrote:

> https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Ri-global
>  is in "Interfaces" section, it only covers inter-procedural stuff.
>  Diagnosing function-local static non-const variable is a plain 
> false-positive diagnostic.


I don't follow your train of thought. Could you please elaborate on why you 
think this must be a false positive?

My reason for hesitating to  call this a false positive is that this pattern 
does cause a hidden dependency between users of the function, hence it clearly 
goes against the short and simple rationale given for this rule:
"Non-const global variables hide dependencies and make the dependencies subject 
to unpredictable changes."

It is arguably unconventional in C++ to make a free function statefull, if one 
wants to make a function stateful there is the obvious alternative of making it 
a member function of a class -which would allow you to achieve the same thing 
but with more explicitly expressed statefullness in the interface and stronger 
encapsulation of the  state.

Not sure if it makes any difference but note that this check also covers rule 
R.6 which is the exact same rule but in a different context.
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rr-global


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77461



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

[PATCH] D77632: [TLI] Per-function fveclib for math library used for vectorization

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

I gave D77952  a try (on top of this one), but 
didn't see a significant improvement from that change.

Looking at the callgrind output for compilation of a **small** file, I see 52M 
total instructions, 4 calls to TLII initialization, where addition of the 
vector functions takes up the majority of the time, at 0.7M. Most of the cost 
is in the sorting. 2 of the initialization calls are default-constructed TLII 
without target triple, which seems suspect to me (are we not adding TLI early 
enough, and something pulls it in via analysis dependency?)

So for small files, just registering the vector functions does make up a 
non-trivial fraction of time, and lazy initialization might make sense. This 
isn't the whole truth though: While the largest regressions are indeed on small 
files, there are also quite a few > 1% regressions on very large files.

For a mid-size file with ~6000M instructions retried, the main difference I see 
is `TargetLibraryAnalysis::run()` going up from 82M to 126M, with the cost 
coming from the extra `getFnAttribute("veclib")` call in the TargetLibraryInfo 
constructor. Fetching attributes is surprisingly expensive, as it performs an 
iteration over all attributes internally. As this code is iterating over all 
attributes anyway in order to handle `no-builtin-*`, it might make sense to 
move the checks for `"veclib"` and `"no-builtins"` into that loop as well, 
which should make them essentially free.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77632



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

[clang-tools-extra] 48d851a - [clangd] Update TUStatus test to handle async PreambleThread

[Kadir Cetinkaya via cfe-commits]

Author: Kadir Cetinkaya
Date: 2020-04-13T12:27:50+02:00
New Revision: 48d851a92e98ca9c425d4d9bc43def3693895498

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

LOG: [clangd] Update TUStatus test to handle async PreambleThread

Summary:
Currently it doesn't matter because we run PreambleThread in sync mode.
Once we start running it in async, test order will become non-deterministic.

Reviewers: sammccall

Subscribers: ilya-biryukov, javed.absar, MaskRay, jkorous, mgrang, arphaman, 
usaxena95, cfe-commits

Tags: #clang

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

Added: 


Modified: 
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp

Removed: 




diff  --git a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp 
b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
index 2a0e6b001c00..bacd1f6b5d98 100644
--- a/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -8,6 +8,7 @@
 
 #include "Annotations.h"
 #include "Cancellation.h"
+#include "ClangdServer.h"
 #include "Context.h"
 #include "Diagnostics.h"
 #include "Matchers.h"
@@ -829,18 +830,35 @@ TEST_F(TUSchedulerTests, TUStatus) {
   class CaptureTUStatus : public ClangdServer::Callbacks {
   public:
 void onFileUpdated(PathRef File, const TUStatus &Status) override {
+  auto ASTAction = Status.ASTActivity.K;
+  auto PreambleAction = Status.PreambleActivity;
   std::lock_guard Lock(Mutex);
-  AllStatus.push_back(Status);
+  // Only push the action if it has changed. Since TUStatus can be 
published
+  // from either Preamble or AST thread and when one changes the other 
stays
+  // the same.
+  // Note that this can result in missing some updates when something other
+  // than action kind changes, e.g. when AST is built/reused the action 
kind
+  // stays as Building.
+  if (ASTActions.empty() || ASTActions.back() != ASTAction)
+ASTActions.push_back(ASTAction);
+  if (PreambleActions.empty() || PreambleActions.back() != PreambleAction)
+PreambleActions.push_back(PreambleAction);
 }
 
-std::vector allStatus() {
+std::vector preambleStatuses() {
   std::lock_guard Lock(Mutex);
-  return AllStatus;
+  return PreambleActions;
+}
+
+std::vector astStatuses() {
+  std::lock_guard Lock(Mutex);
+  return ASTActions;
 }
 
   private:
 std::mutex Mutex;
-std::vector AllStatus;
+std::vector ASTActions;
+std::vector PreambleActions;
   } CaptureTUStatus;
   MockFSProvider FS;
   MockCompilationDatabase CDB;
@@ -850,35 +868,39 @@ TEST_F(TUSchedulerTests, TUStatus) {
   // We schedule the following tasks in the queue:
   //   [Update] [GoToDefinition]
   Server.addDocument(testPath("foo.cpp"), Code.code(), "1",
- WantDiagnostics::Yes);
+ WantDiagnostics::Auto);
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
   Server.locateSymbolAt(testPath("foo.cpp"), Code.point(),
 [](Expected> Result) {
   ASSERT_TRUE((bool)Result);
 });
-
   ASSERT_TRUE(Server.blockUntilIdleForTest());
 
-  EXPECT_THAT(CaptureTUStatus.allStatus(),
+  EXPECT_THAT(CaptureTUStatus.preambleStatuses(),
+  ElementsAre(
+  // PreambleThread starts idle, as the update is first handled
+  // by ASTWorker.
+  PreambleAction::Idle,
+  // Then it starts building first preamble and releases that 
to
+  // ASTWorker.
+  PreambleAction::Building,
+  // Then goes idle and stays that way as we don't receive any
+  // more update requests.
+  PreambleAction::Idle));
+  EXPECT_THAT(CaptureTUStatus.astStatuses(),
   ElementsAre(
-  // Everything starts with ASTWorker starting to execute an
-  // update
-  TUState(PreambleAction::Idle, ASTAction::RunningAction),
-  // We build the preamble
-  TUState(PreambleAction::Building, ASTAction::RunningAction),
-  // We built the preamble, and issued ast built on ASTWorker
-  // thread. Preambleworker goes idle afterwards.
-  TUState(PreambleAction::Idle, ASTAction::RunningAction),
-  // Start task for building the ast, as a result of building
-  // preamble, on astworker thread.
-  TUState(PreambleAction::Idle, ASTAction::RunningAction),
-  // AST build starts.
-  TUState(PreambleAction::Idle, ASTAction::Building),
-  // AST built fi

[PATCH] D75184: [clang-tidy] Optional inheritance of file configs from parent directories

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin added a comment.

@alexfh friend ping, please take a look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75184



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

[PATCH] D77669: [clangd] Update TUStatus test to handle async PreambleThread

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG48d851a92e98: [clangd] Update TUStatus test to handle async 
PreambleThread (authored by kadircet).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77669

Files:
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp

Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -8,6 +8,7 @@
 
 #include "Annotations.h"
 #include "Cancellation.h"
+#include "ClangdServer.h"
 #include "Context.h"
 #include "Diagnostics.h"
 #include "Matchers.h"
@@ -829,18 +830,35 @@
   class CaptureTUStatus : public ClangdServer::Callbacks {
   public:
 void onFileUpdated(PathRef File, const TUStatus &Status) override {
+  auto ASTAction = Status.ASTActivity.K;
+  auto PreambleAction = Status.PreambleActivity;
   std::lock_guard Lock(Mutex);
-  AllStatus.push_back(Status);
+  // Only push the action if it has changed. Since TUStatus can be published
+  // from either Preamble or AST thread and when one changes the other stays
+  // the same.
+  // Note that this can result in missing some updates when something other
+  // than action kind changes, e.g. when AST is built/reused the action kind
+  // stays as Building.
+  if (ASTActions.empty() || ASTActions.back() != ASTAction)
+ASTActions.push_back(ASTAction);
+  if (PreambleActions.empty() || PreambleActions.back() != PreambleAction)
+PreambleActions.push_back(PreambleAction);
 }
 
-std::vector allStatus() {
+std::vector preambleStatuses() {
   std::lock_guard Lock(Mutex);
-  return AllStatus;
+  return PreambleActions;
+}
+
+std::vector astStatuses() {
+  std::lock_guard Lock(Mutex);
+  return ASTActions;
 }
 
   private:
 std::mutex Mutex;
-std::vector AllStatus;
+std::vector ASTActions;
+std::vector PreambleActions;
   } CaptureTUStatus;
   MockFSProvider FS;
   MockCompilationDatabase CDB;
@@ -850,35 +868,39 @@
   // We schedule the following tasks in the queue:
   //   [Update] [GoToDefinition]
   Server.addDocument(testPath("foo.cpp"), Code.code(), "1",
- WantDiagnostics::Yes);
+ WantDiagnostics::Auto);
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
   Server.locateSymbolAt(testPath("foo.cpp"), Code.point(),
 [](Expected> Result) {
   ASSERT_TRUE((bool)Result);
 });
-
   ASSERT_TRUE(Server.blockUntilIdleForTest());
 
-  EXPECT_THAT(CaptureTUStatus.allStatus(),
+  EXPECT_THAT(CaptureTUStatus.preambleStatuses(),
+  ElementsAre(
+  // PreambleThread starts idle, as the update is first handled
+  // by ASTWorker.
+  PreambleAction::Idle,
+  // Then it starts building first preamble and releases that to
+  // ASTWorker.
+  PreambleAction::Building,
+  // Then goes idle and stays that way as we don't receive any
+  // more update requests.
+  PreambleAction::Idle));
+  EXPECT_THAT(CaptureTUStatus.astStatuses(),
   ElementsAre(
-  // Everything starts with ASTWorker starting to execute an
-  // update
-  TUState(PreambleAction::Idle, ASTAction::RunningAction),
-  // We build the preamble
-  TUState(PreambleAction::Building, ASTAction::RunningAction),
-  // We built the preamble, and issued ast built on ASTWorker
-  // thread. Preambleworker goes idle afterwards.
-  TUState(PreambleAction::Idle, ASTAction::RunningAction),
-  // Start task for building the ast, as a result of building
-  // preamble, on astworker thread.
-  TUState(PreambleAction::Idle, ASTAction::RunningAction),
-  // AST build starts.
-  TUState(PreambleAction::Idle, ASTAction::Building),
-  // AST built finished successfully
-  TUState(PreambleAction::Idle, ASTAction::Building),
-  // Running go to def
-  TUState(PreambleAction::Idle, ASTAction::RunningAction),
-  // ASTWorker goes idle.
-  TUState(PreambleAction::Idle, ASTAction::Idle)));
+  // Starts handling the update action and blocks until the
+  // first preamble is built.
+  ASTAction::RunningAction,
+  // Afterwqards it builds an AST for that preamble to publish
+  // diagnostics.
+  ASTAct

[PATCH] D77062: [analyzer] Added check for unacceptable equality operation between Loc and NonLoc types

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@Szelethus, @NoQ
I've investigated graph.dot of the sample. F11723129: t37503.dot 

Here is a simplification:

1. SA thinks that `ptr` is a pointer with a structure 
`MemRegion->MemRegion->MemRegion->Element`
2. Then `*(unsigned char **)ptr = (unsigned char *)(func());` occures. Symbolic 
substitution happens to `ptr`.
3. After that SA thinks that `ptr` holds a symbolic value 
`MemRegion->MemRegion->Element` because of casts.
4. `**ptr` should lead us to `MemRegion->MemRegion->MemRegion` from C++ point 
of view, but dereferencing applies to substituted symbolic value from SA point 
of view and we finally get `MemRegion->MemRegion->Element`

As I see, this is not //treating the symptom//. This is exactly handling this 
particular case which is legal and may take place.

Another solution could be to check the first argument of `strcpy` for being 
actially a `char*` and show a warning otherwise.

Please, explain, what I could miss in my suggestions, because I'm less 
expertise than you, guys.


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

https://reviews.llvm.org/D77062



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

[PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

[Raphael Isemann via Phabricator via cfe-commits]

teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

From what I understand the whole idea here is to just ask the external AST 
source to complete the record before we import them? If yes, then this seems 
like the right idea to me.

Also this seems to be testable via a Clang unit test, so I think this patch 
should have one.

But otherwise this LGTM. Thanks for figuring this out!




Comment at: clang/lib/AST/ASTImporter.cpp:8173
+  // If a RecordDecl has base classes they won't be imported and we will
+  // be missing anything that we inherit from those bases.
+  if (FromRecord->hasExternalLexicalStorage() &&

I'm not sure how it can be that ASTImporter::CompleteDecl starts but never 
finishes the decl as it does both things at once unconditionally?
```
lang=c++
  TD->startDefinition();
  TD->setCompleteDefinition(true);
```

FWIW, this whole comment could just be `Ask the external source if this is 
actually a complete record that just hasn't been completed yet` or something 
like this. I think if we reach this point then it makes a complete sense to ask 
the external source for more info. The explanation about the base classes seems 
to be just a smaller detail of the whole picture here.



Comment at: clang/lib/AST/ASTImporter.cpp:8180
+  if (Error Err = ASTNodeImporter(*this).ImportDefinition(
+  FromRecord, ToRecord, ASTNodeImporter::IDK_Basic))
+return std::move(Err);

I assume we should check here that FromRecord is now a complete definition 
before trying to import it's definition?



Comment at: 
lldb/test/API/commands/expression/import_base_class_when_class_has_derived_member/main.cpp:25
+return true; //%self.expect_expr("DD->dump()", result_type="int",
+ // result_value="42")
+  }

You need to make this a "//%" as otherwise this test fails (which it does right 
now).


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

https://reviews.llvm.org/D78000



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

[PATCH] D77461: [WIP][clang-tidy] Remove false positive in AvoidNonConstGlobalVariables

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D77461#1977503 , @vingeldal wrote:

> In D77461#1963166 , @lebedev.ri 
> wrote:
>
> > https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Ri-global
> >  is in "Interfaces" section, it only covers inter-procedural stuff.
> >  Diagnosing function-local static non-const variable is a plain 
> > false-positive diagnostic.
>
>
> I don't follow your train of thought. Could you please elaborate on why you 
> think this must be a false positive?


I think it must be a false positive because the rule is about global variables 
where "global" refers to the scope of the variable. This is a locally scoped 
variable. Also, the rule's enforcement section is pretty clear that this does 
not apply to local statics.

> My reason for hesitating to  call this a false positive is that this pattern 
> does cause a hidden dependency between users of the function, hence it 
> clearly goes against the short and simple rationale given for this rule:
>  "Non-const global variables hide dependencies and make the dependencies 
> subject to unpredictable changes."

I don't see it hiding a dependency between users of the function. The local 
static could be swapped out for something else (an actual global, linker magic, 
etc) and the caller would be unaware. The same is not true for a globally 
scoped object where the identifier is exposed because someone else could be 
trying to link to that symbol (and they would break if the symbol changed).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77461



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

[PATCH] D75184: [clang-tidy] Optional inheritance of file configs from parent directories

[Alexander Kornienko via Phabricator via cfe-commits]

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

Apologies for the delay! It's sort of a crazy time now =\

The code looks mostly good now modulo a few comments inline.




Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.cpp:52
+  (IterGlobal == CheckOptions.end() ||
+   IterGlobal->second.Priority < IterLocal->second.Priority))
+return IterLocal->second.Value;

supernit: Let's swap the compared values to make the code slightly more 
intuitive ("if local is higher priority, use local" vs "if global is lower 
priority, use local").



Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.cpp:73
+for (const auto &KeyValue : OptionMap)
+  Options.push_back(std::make_pair(KeyValue.first, KeyValue.second.Value));
+  }

nit: would emplace_back work?



Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.h:101-104
+ClangTidyValue(const char *Value) : Value(Value), Priority(0) {}
+ClangTidyValue(const std::string &Value) : Value(Value), Priority(0) {}
+ClangTidyValue(const std::string &Value, unsigned Priority)
+: Value(Value), Priority(Priority) {}

Maybe just `ClangTidyValue(StringRef Value, unsigned Priority = 0)`?



Comment at: clang-tools-extra/clang-tidy/ClangTidyOptions.h:107
+std::string Value;
+unsigned Priority;
+  };

Could you describe how the priority is used?



Comment at: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp:41
+  in the parent directory (if any exists) will be taken and current config file
+  will be applied on top of the parent one. If any configuration options have
+  a corresponding command-line option, command-line option takes precedence.

Does the new logic related to local and global options deserve a separate 
mention here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75184



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

[clang] ebd5290 - Address sphinx warnings

[Benjamin Kramer via cfe-commits]

Author: Benjamin Kramer
Date: 2020-04-13T14:41:55+02:00
New Revision: ebd5290ff2b34c6863ee50cd2bcadff19d260812

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

LOG: Address sphinx warnings

LanguageExtensions.rst:2191: WARNING: Title underline too short.
llvm-symbolizer.rst:157: Error in "code-block" directive: maximum 1 argument(s) 
allowed, 30 supplied.

Added: 


Modified: 
clang/docs/LanguageExtensions.rst
llvm/docs/CommandGuide/llvm-symbolizer.rst

Removed: 




diff  --git a/clang/docs/LanguageExtensions.rst 
b/clang/docs/LanguageExtensions.rst
index f83d7cff9c87..929cd1c67e73 100644
--- a/clang/docs/LanguageExtensions.rst
+++ b/clang/docs/LanguageExtensions.rst
@@ -2188,7 +2188,7 @@ argument.
   __builtin_preserve_access_index(v->j);
 
 ``__builtin_unique_stable_name``
-
+
 
 ``__builtin_unique_stable_name()`` is a builtin that takes a type or 
expression and
 produces a string literal containing a unique name for the type (or type of the

diff  --git a/llvm/docs/CommandGuide/llvm-symbolizer.rst 
b/llvm/docs/CommandGuide/llvm-symbolizer.rst
index 820b15061715..5c8465af04a7 100644
--- a/llvm/docs/CommandGuide/llvm-symbolizer.rst
+++ b/llvm/docs/CommandGuide/llvm-symbolizer.rst
@@ -155,10 +155,11 @@ shows the default absolute path, the second --basenames, 
and the third
 shows --relativenames.
 
 .. code-block:: console
+
   $ pwd
   /tmp
   $ clang -g foo/test.cpp -o test.elf
-  $ llvm-symbolizer --obj=test.elf 0x4004a0 
+  $ llvm-symbolizer --obj=test.elf 0x4004a0
   main
   /tmp/foo/test.cpp:15:0
   $ llvm-symbolizer --obj=test.elf 0x4004a0 --basenames
@@ -167,7 +168,7 @@ shows --relativenames.
   $ llvm-symbolizer --obj=test.elf 0x4004a0 --relativenames
   main
   foo/test.cpp:15:0
-   
+
 OPTIONS
 ---
 



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

[PATCH] D77983: clang-tidy doc: add a note for every checker with an autofix

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/abseil-duration-conversion-cast.rst:29-35
 Note: In the second example, the suggested fix could yield a different result,
 as the conversion to integer could truncate.  In practice, this is very rare,
 and you should use ``absl::Trunc`` to perform this operation explicitly 
instead.
+
+.. note::
+
+  This checker provides an automatic fix callable with ``--fix``.

A few problems with this approach:
1. Some checkers' docs already have more specific wording about the fix. Adding 
a generic phase doesn't make the documentation look neat.
2. The "callable with --fix" part isn't relevant to all clang-tidy frontends. 
Even for the CLI a reference to the documentation would be more helpful than a 
mention of this command-line option (there are other relevant options and ways 
to apply a fix - --fix-errors, -export-fixes + the clang-apply-fixes tool, 
etc.).

My proposal is to add a mandatory `Automatic fixes` or `Suggested fixes` 
section to all checker documentation files, require the authors to describe the 
nature of the fix more thoroughly, and use consistent wording when there is no 
fix. E.g.

```
Automatic Fixes
^

The check doesn't provide an automatic fix.
```

or

```
Automatic Fixes
^

The check provides suggested fixes. See the documentation above for the details.
```

or

```
Automatic Fixes
^

The check provides suggested fixes in the following cases:
1.
2.
3.
```

WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77983



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

[PATCH] D77461: [WIP][clang-tidy] Remove false positive in AvoidNonConstGlobalVariables

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

FWIW i've posted https://github.com/isocpp/CppCoreGuidelines/issues/1599 asking 
for clarification, but no replies so far..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77461



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

[PATCH] D77233: [NFC] Refactoring PropertyAttributeKind for ObjCPropertyDecl and ObjCDeclSpec.

[Erik Pilkington via Phabricator via cfe-commits]

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

LGTM (after fixing those tests). Thanks for cleaning this up!




Comment at: clang/tools/c-index-test/c-index-test.c:1107
 PRINT_PROP_ATTR(unsafe_unretained);
-PRINT_PROP_ATTR(class);
+PRINT_PROP_ATTR(classattr);
 printf("]");

Hm, looks like this change is breaking some CHECK lines in the indexer tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77233



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

[PATCH] D77461: [WIP][clang-tidy] Remove false positive in AvoidNonConstGlobalVariables

[Kim Viggedal via Phabricator via cfe-commits]

vingeldal added a comment.

In D77461#1977639 , @aaron.ballman 
wrote:

> In D77461#1977503 , @vingeldal wrote:
>
> > In D77461#1963166 , @lebedev.ri 
> > wrote:
> >
> > > https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Ri-global
> > >  is in "Interfaces" section, it only covers inter-procedural stuff.
> > >  Diagnosing function-local static non-const variable is a plain 
> > > false-positive diagnostic.
> >
> >
> > I don't follow your train of thought. Could you please elaborate on why you 
> > think this must be a false positive?
>
>
> I think it must be a false positive because the rule is about global 
> variables where "global" refers to the scope of the variable. This is a 
> locally scoped variable. Also, the rule's enforcement section is pretty clear 
> that this does not apply to local statics.
>
> > My reason for hesitating to  call this a false positive is that this 
> > pattern does cause a hidden dependency between users of the function, hence 
> > it clearly goes against the short and simple rationale given for this rule:
> >  "Non-const global variables hide dependencies and make the dependencies 
> > subject to unpredictable changes."
>
> I don't see it hiding a dependency between users of the function. The local 
> static could be swapped out for something else (an actual global, linker 
> magic, etc) and the caller would be unaware. The same is not true for a 
> globally scoped object where the identifier is exposed because someone else 
> could be trying to link to that symbol (and they would break if the symbol 
> changed).


A caller of this function will clearly be affected by other callers if there 
are any, so there clearly is a dependency added between callers of this 
function. At the same time the function doesn't provide any hints toward the 
fact that it is statefull or any way for the caller achieve observability of 
this state, hence the state causing the dependency is hidden and the dependency 
is hidden.

I must agree that the enforcement section goes against my interpretation but I 
would prefer to try to simply change the enforcement section.

This function with a static variable can almost always be replaced with a 
solution where the free function is instead made a member function of a class 
keeping the state as a non-static, private member variable -which would more 
clearly convey that the function may have state and would provide stronger 
encapsulation.
The only exception would be if we are talking about a C-interface, in which 
case one could still use a class, as I just described, instead of a static 
variable; you would just have to call the member function via a free function.
Can anyone explain to me why one would ever *have* to rely on functions with 
static variables? Because before I can see a legitimate use case for this 
potential false positive I think it would be a bad idea to consider it a false 
positive since there are obvious issues with hidden dependencies in the 
discussed example code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77461



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

[PATCH] D77545: Represent FP options in AST by special Expression node

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added a comment.

In D77545#1974509 , @rjmccall wrote:

> For example, in this code:
>
>   const float global = 2.22 + 3.33;
>  
>   #pragma STDV FENV_ROUND 
>   float getGlobal() { return global; }
>
>
> The code-generation of `getGlobal` will attempt to constant-evaluate the 
> initializer of `global` so that it can just return a constant instead of 
> emitting a load from a global.  It is important for correctness that this 
> constant-evaluation occur in the default FP environment, not the active FP 
> environment at the point of use.  Presumably you are not proposing that 
> `global`'s initializer is a `PragmaExpr`, since it's in the default 
> environment.  Perhaps we can recognize by the *absence* of a `PragmaExpr` 
> that it's meant to use the default environment?  It's not clear at what level 
> we would trigger this check, however.  Is it now semantically important that 
> we call a method like `VarDecl::evaluateValue()` rather than 
> `Expr::EvaluateAsFloat(...)`?  What happens with variables in local scope?  A 
> local variable could be from a scope outside the current pragma, so 
> presumably we need to introduce `PragmaExpr`s on the initializers of all 
> variables within pragmas.


As you correctly noted, putting `PragmaExpr` around initializer expression even 
in default environment solves all these problems.  This solution may be 
improved to put `PragmaExpr`  only  when the expression may be dependent on FP 
environment.

> This sort of problem is why having the pragma state be an argument rather 
> than global state is so much more attractive.  And making it an argument is 
> much more disruptive for clients, who must now do their own tracking, and 
> recreate the state appropriately when calling outside the context.
> 
> Storing the pragma state in the expressions avoids these problems much more 
> cleanly.

I see at least two issues with such solution:

1. There are cases when part of AST should be interpreted in other context than 
it was created. The example was presented earlier, this is constexpr function 
evaluation:

  #pragma STDC FENV_ACCESS ON
constexpr float func(float x, float y) { return x + y;  }

#pragma STDC FENV_ROUND FE_DOWNWARD
float V1 = func(1.11, 2.22);

#pragma STDC FENV_ROUND FE_UPWARD
float V2 = func(1.11, 2.22);

If FP environment is encoded in AST nodes, then the body of `func` should use 
`FE_DYNAMIC` as rounding mode. During evaluation such nodes this value should 
be replaced by the rounding mode taken from current context. Constant evaluator 
anyway should use global state and `RoundingMode::Dynamic` must be interpreted 
specifically.

2. This solution requires huge changes  to AST node implementation, as 
demonstrated by D76384 . And what will we do 
with CallExpr and CastExpr, which already have TrailingObjects? How this 
solution can be extended? For instance, we may want to make ExprWithCleanups 
sensitive to FP environment so that it can restore previous state. We have to 
start with redesign of ExprWithCleanups so that it contains FPOptions. As this 
node already contains TrailingObjects it is not a trivial task. Any node that 
designates an operation that can depend on FP state should be reworked first.

In contrast, using special nodes to represent global state make this task 
transparently. No changes are required to existing AST nodes, only AST 
consumers that are aware of FP state need to be updated. This way looks more 
robust due to orthogonal and less invasive implementation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77545



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

[PATCH] D77461: [WIP][clang-tidy] Remove false positive in AvoidNonConstGlobalVariables

[Kim Viggedal via Phabricator via cfe-commits]

vingeldal added a comment.

I'm also adding to this discussion the exception-part of the rule:
"A global object is often better than a singleton."

I would argue that the example given above, while not strictly follwong the 
definition of a singleton, is behaviorally consistent with a singleton.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77461



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

[PATCH] D76083: [clang-tidy] Expand the list of functions in bugprone-unused-return-value

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp:98
+   "::access;"
+   "::bind;"
+   "::connect;"

aaron.ballman wrote:
> sammccall wrote:
> > aaron.ballman wrote:
> > > sammccall wrote:
> > > > aaron.ballman wrote:
> > > > > jranieri-grammatech wrote:
> > > > > > alexfh wrote:
> > > > > > > bind has a side effect and returns a success status. Thus, the 
> > > > > > > result being unused isn't necessarily a bug. Same for `connect`. 
> > > > > > > And probably for `setjmp` as well.
> > > > > > In terms of bind, connect, and setjmp: while I personally would say 
> > > > > > that code not using the return value is bugprone, the data suggests 
> > > > > > that the vast majority of developers are using these functions in 
> > > > > > the intended manner and the false-positive rate should be low.
> > > > > I think we have sufficient statistical data to suggest that these 
> > > > > APIs should be on the list because the majority of programmers *do 
> > > > > not* use them solely for side effects without using the return value, 
> > > > > so my preference is to keep them in the list.
> > > > I stumbled upon this review as we're considering turning this check on 
> > > > by default in clangd.
> > > > 
> > > > There's a significant difference between unused std::async() 
> > > > (programmer misunderstood contract) and unused ::connect() (ignoring 
> > > > error conditions). The former is ~never noise, and the latter may be 
> > > > (e.g. in experimental or incomplete code).
> > > > 
> > > > So there's some value in separating these two lists out either as an 
> > > > option or a separate named check (bugprone-unhandled-error?) I think we 
> > > > probably wouldn't enable this check by default if it includes the 
> > > > error-code functions.
> > > > 
> > > > > the majority of programmers *do not* use them solely for side effects
> > > > ...in popular, distributed software :-)
> > > > So there's some value in separating these two lists out either as an 
> > > > option or a separate named check (bugprone-unhandled-error?) I think we 
> > > > probably wouldn't enable this check by default if it includes the 
> > > > error-code functions.
> > > 
> > > I think that adds complexity to this check when the complexity isn't 
> > > necessary. clang-tidy has traditionally been a place for checks that are 
> > > chattier than what the compiler should provide, and this check has a 
> > > trivial, well-understood mechanism to silence the diagnostics (cast to 
> > > void) which also expresses intent properly to the toolchain.
> > > 
> > > >>the majority of programmers *do not* use them solely for side effects
> > > > ...in popular, distributed software :-)
> > > 
> > > I have not seen anyone provide data to suggest that the functions in 
> > > question appear in any statistically significant amount in practice 
> > > without checking the return value, just worries that they *could*. I 
> > > don't think that's compelling in the face of data. Remember, this is for 
> > > bugprone patterns, not bugknown patterns.
> > I think we're talking past each other here. I'm not saying clang-tidy 
> > shouldn't have the check, or that it's not a bugprone pattern, or that the 
> > clang-tidy default should be different.
> > 
> > But there are scenarios where you want one but not the other. Concretely, 
> > warnings shown in an IDE **as you type and by default**. If you're misusing 
> > an API rendering it completely useless, you should see that ASAP. If you 
> > fail to check an error code, some users won't want to be warned about that 
> > until later.
> > 
> > By bundling them into a single check without options (other than 
> > duplicating the whole list), it's hard to create that useful but 
> > inoffensive default setup. That's OK, clangd can remove the check from the 
> > whitelist, but I think we'd get a lot of value out of it.
> > 
> > > I have not seen anyone provide data to suggest that the functions in 
> > > question appear in any statistically significant amount in practice
> > Right, we don't have data either way on incomplete code. Based on 
> > experience of writing code and watching others write code, I believe people 
> > write sloppy code they'd never check in, and appreciate being told early 
> > when it doesn't do what they intend, but some don't appreciate being told 
> > to be less sloppy. Is your intuition different? Do you think the data 
> > provided addresses this question?
> > But there are scenarios where you want one but not the other. Concretely, 
> > warnings shown in an IDE as you type and by default. If you're misusing an 
> > API rendering it completely useless, you should see that ASAP. If you fail 
> > to check an error code, some users won't want to be warned about that until 
> > later.
> 
> You're right, we wer

[PATCH] D77461: [WIP][clang-tidy] Remove false positive in AvoidNonConstGlobalVariables

[Kim Viggedal via Phabricator via cfe-commits]

vingeldal added a comment.

In D77461#1977703 , @aaron.ballman 
wrote:

> In D77461#1977687 , @vingeldal wrote:
>
> > In D77461#1977639 , @aaron.ballman 
> > wrote:
> >
> > > In D77461#1977503 , @vingeldal 
> > > wrote:
> > >
> > > > In D77461#1963166 , 
> > > > @lebedev.ri wrote:
> > > >
> > > > > https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Ri-global
> > > > >  is in "Interfaces" section, it only covers inter-procedural stuff.
> > > > >  Diagnosing function-local static non-const variable is a plain 
> > > > > false-positive diagnostic.
> > > >
> > > >
> > > > I don't follow your train of thought. Could you please elaborate on why 
> > > > you think this must be a false positive?
> > >
> > >
> > > I think it must be a false positive because the rule is about global 
> > > variables where "global" refers to the scope of the variable. This is a 
> > > locally scoped variable. Also, the rule's enforcement section is pretty 
> > > clear that this does not apply to local statics.
> > >
> > > > My reason for hesitating to  call this a false positive is that this 
> > > > pattern does cause a hidden dependency between users of the function, 
> > > > hence it clearly goes against the short and simple rationale given for 
> > > > this rule:
> > > >  "Non-const global variables hide dependencies and make the 
> > > > dependencies subject to unpredictable changes."
> > >
> > > I don't see it hiding a dependency between users of the function. The 
> > > local static could be swapped out for something else (an actual global, 
> > > linker magic, etc) and the caller would be unaware. The same is not true 
> > > for a globally scoped object where the identifier is exposed because 
> > > someone else could be trying to link to that symbol (and they would break 
> > > if the symbol changed).
> >
> >
> > A caller of this function will clearly be affected by other callers if 
> > there are any, so there clearly is a dependency added between callers of 
> > this function. At the same time the function doesn't provide any hints 
> > toward the fact that it is statefull or any way for the caller achieve 
> > observability of this state, hence the state causing the dependency is 
> > hidden and the dependency is hidden.
> >
> > I must agree that the enforcement section goes against my interpretation 
> > but I would prefer to try to simply change the enforcement section.
> >
> > This function with a static variable can almost always be replaced with a 
> > solution where the free function is instead made a member function of a 
> > class keeping the state as a non-static, private member variable -which 
> > would more clearly convey that the function may have state and would 
> > provide stronger encapsulation.
> >  The only exception would be if we are talking about a C-interface, in 
> > which case one could still use a class, as I just described, instead of a 
> > static variable; you would just have to call the member function via a free 
> > function.
> >  Can anyone explain to me why one would ever *have* to rely on functions 
> > with static variables?
>
>
> To avoid the static initialization order fiasco, statistics or timing 
> counters, etc -- look through the LLVM code base for function local statics, 
> they're not uncommon.
>
> > Because before I can see a legitimate use case for this potential false 
> > positive I think it would be a bad idea to consider it a false positive 
> > since there are obvious issues with hidden dependencies in the discussed 
> > example code.
>
> If we don't hear back from the C++ Core Guidelines authors quickly on their 
> thoughts or adjustments, there are at least two reviewers who consider it a 
> false positive based on the current rule text, which is sufficient 
> post-review feedback to warrant a change IMO.


You can also avoid static initialization order fiasco by not using static as 
much, but fine, I'll just do it so I don't have to think about this discussion 
again.
Before I do that I'm just adding to the record that if there was an 
implementation for rule F.8 this example would definitely been reported by that 
check instead so it's clearly something to avoid according to the C++ Core 
Guidelines.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77461



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

[PATCH] D77461: [WIP][clang-tidy] Remove false positive in AvoidNonConstGlobalVariables

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D77461#1977687 , @vingeldal wrote:

> In D77461#1977639 , @aaron.ballman 
> wrote:
>
> > In D77461#1977503 , @vingeldal 
> > wrote:
> >
> > > In D77461#1963166 , @lebedev.ri 
> > > wrote:
> > >
> > > > https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Ri-global
> > > >  is in "Interfaces" section, it only covers inter-procedural stuff.
> > > >  Diagnosing function-local static non-const variable is a plain 
> > > > false-positive diagnostic.
> > >
> > >
> > > I don't follow your train of thought. Could you please elaborate on why 
> > > you think this must be a false positive?
> >
> >
> > I think it must be a false positive because the rule is about global 
> > variables where "global" refers to the scope of the variable. This is a 
> > locally scoped variable. Also, the rule's enforcement section is pretty 
> > clear that this does not apply to local statics.
> >
> > > My reason for hesitating to  call this a false positive is that this 
> > > pattern does cause a hidden dependency between users of the function, 
> > > hence it clearly goes against the short and simple rationale given for 
> > > this rule:
> > >  "Non-const global variables hide dependencies and make the dependencies 
> > > subject to unpredictable changes."
> >
> > I don't see it hiding a dependency between users of the function. The local 
> > static could be swapped out for something else (an actual global, linker 
> > magic, etc) and the caller would be unaware. The same is not true for a 
> > globally scoped object where the identifier is exposed because someone else 
> > could be trying to link to that symbol (and they would break if the symbol 
> > changed).
>
>
> A caller of this function will clearly be affected by other callers if there 
> are any, so there clearly is a dependency added between callers of this 
> function. At the same time the function doesn't provide any hints toward the 
> fact that it is statefull or any way for the caller achieve observability of 
> this state, hence the state causing the dependency is hidden and the 
> dependency is hidden.
>
> I must agree that the enforcement section goes against my interpretation but 
> I would prefer to try to simply change the enforcement section.
>
> This function with a static variable can almost always be replaced with a 
> solution where the free function is instead made a member function of a class 
> keeping the state as a non-static, private member variable -which would more 
> clearly convey that the function may have state and would provide stronger 
> encapsulation.
>  The only exception would be if we are talking about a C-interface, in which 
> case one could still use a class, as I just described, instead of a static 
> variable; you would just have to call the member function via a free function.
>  Can anyone explain to me why one would ever *have* to rely on functions with 
> static variables?


To avoid the static initialization order fiasco, statistics or timing counters, 
etc -- look through the LLVM code base for function local statics, they're not 
uncommon.

> Because before I can see a legitimate use case for this potential false 
> positive I think it would be a bad idea to consider it a false positive since 
> there are obvious issues with hidden dependencies in the discussed example 
> code.

If we don't hear back from the C++ Core Guidelines authors quickly on their 
thoughts or adjustments, there are at least two reviewers who consider it a 
false positive based on the current rule text, which is sufficient post-review 
feedback to warrant a change IMO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77461



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

[clang] 072ae7c - [clang-format] Always break line after enum opening brace

[via cfe-commits]

Author: mydeveloperday
Date: 2020-04-13T15:03:36+01:00
New Revision: 072ae7c1e64f8dd1b5e9db17838c93b150f8b487

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

LOG: [clang-format] Always break line after enum opening brace

Summary:
clang-format currently puts the first enumerator on the same line as the
enum keyword and opening brace if it fits (for example, for anonymous
enums if IndentWidth is 8):

  $ echo "enum { RED, GREEN, BLUE };" | clang-format -style="{BasedOnStyle: 
llvm, ColumnLimit: 15, IndentWidth: 8}"
  enum { RED,
 GREEN,
 BLUE };

This doesn't seem to be intentional, as I can't find any style guide that
suggests wrapping enums this way. Always force the enumerator to be on a new
line, which gets us the desired result:

  $ echo "enum { RED, GREEN, BLUE };" | ./bin/clang-format 
-style="{BasedOnStyle: llvm, ColumnLimit: 15, IndentWidth: 8}"
  enum {
  RED,
  GREEN,
  BLUE
  };

Test Plan:

New test added. Confirmed test failed without change and passed with change by
running:

  $ ninja FormatTests && ./tools/clang/unittests/Format/FormatTests

Reviewed By: MyDeveloperDay

Patch By: osandov

Tags: #clang-format, #clang

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

Added: 


Modified: 
clang/lib/Format/ContinuationIndenter.cpp
clang/unittests/Format/FormatTest.cpp

Removed: 




diff  --git a/clang/lib/Format/ContinuationIndenter.cpp 
b/clang/lib/Format/ContinuationIndenter.cpp
index 03e79a22954e..2dda5d89a3ac 100644
--- a/clang/lib/Format/ContinuationIndenter.cpp
+++ b/clang/lib/Format/ContinuationIndenter.cpp
@@ -423,7 +423,7 @@ bool ContinuationIndenter::mustBreak(const LineState 
&State) {
   State.Stack.back().BreakBeforeParameter && Current.CanBreakBefore)
 return true;
 
-  if (State.Column <= NewLineColumn)
+  if (!State.Line->First->is(tok::kw_enum) && State.Column <= NewLineColumn)
 return false;
 
   if (Style.AlwaysBreakBeforeMultilineStrings &&

diff  --git a/clang/unittests/Format/FormatTest.cpp 
b/clang/unittests/Format/FormatTest.cpp
index fe16eeaba53c..ece7c61a999c 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -1929,6 +1929,24 @@ TEST_F(FormatTest, FormatsEnum) {
"  TWO\n"
"};\n"
"int i;");
+
+  FormatStyle EightIndent = getLLVMStyle();
+  EightIndent.IndentWidth = 8;
+  verifyFormat("enum {\n"
+   "VOID,\n"
+   "CHAR,\n"
+   "SHORT,\n"
+   "INT,\n"
+   "LONG,\n"
+   "SIGNED,\n"
+   "UNSIGNED,\n"
+   "BOOL,\n"
+   "FLOAT,\n"
+   "DOUBLE,\n"
+   "COMPLEX\n"
+   "};",
+   EightIndent);
+
   // Not enums.
   verifyFormat("enum X f() {\n"
"  a();\n"



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

[clang] 3b37924 - [clang-format] A Minor change to clang-format-diff.py

[via cfe-commits]

Author: mydeveloperday
Date: 2020-04-13T15:08:07+01:00
New Revision: 3b379246c365721425d7e29772c08d8373b4a606

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

LOG: [clang-format] A Minor change to clang-format-diff.py

Summary: Testing for None should use the 'is' operator.

Reviewed By: MyDeveloperDay

Patch By:  eagleoflqj

Tags: #clang-format

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

Added: 


Modified: 
clang/tools/clang-format/clang-format-diff.py

Removed: 




diff  --git a/clang/tools/clang-format/clang-format-
diff .py b/clang/tools/clang-format/clang-format-
diff .py
index 122db49ff73c..24c6f4ae456c 100755
--- a/clang/tools/clang-format/clang-format-
diff .py
+++ b/clang/tools/clang-format/clang-format-
diff .py
@@ -65,7 +65,7 @@ def main():
 match = re.search(r'^\+\+\+\ (.*?/){%s}(\S*)' % args.p, line)
 if match:
   filename = match.group(2)
-if filename == None:
+if filename is None:
   continue
 
 if args.regex is not None:



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

[clang] e811150 - [clang-format] use spaces for alignment with UT_ForContinuationAndIndentation

[via cfe-commits]

Author: mydeveloperday
Date: 2020-04-13T15:14:26+01:00
New Revision: e8111502d8696896241132865c7a44c6f84f93c1

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

LOG: [clang-format] use spaces for alignment with 
UT_ForContinuationAndIndentation

Summary:
Use spaces instead of tabs for alignment with UT_ForContinuationAndIndentation 
to make the code aligned for any tab/indent width.

Fixes https://bugs.llvm.org/show_bug.cgi?id=38381

Reviewed By: MyDeveloperDay

Patch By: fickert

Tags: #clang-format

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

Added: 


Modified: 
clang/docs/ClangFormatStyleOptions.rst
clang/include/clang/Format/Format.h
clang/lib/Format/BreakableToken.cpp
clang/lib/Format/ContinuationIndenter.cpp
clang/lib/Format/ContinuationIndenter.h
clang/lib/Format/Format.cpp
clang/lib/Format/UnwrappedLineFormatter.cpp
clang/lib/Format/WhitespaceManager.cpp
clang/lib/Format/WhitespaceManager.h
clang/unittests/Format/FormatTest.cpp

Removed: 




diff  --git a/clang/docs/ClangFormatStyleOptions.rst 
b/clang/docs/ClangFormatStyleOptions.rst
index 1c1d0142930f..6d486224e3c2 100644
--- a/clang/docs/ClangFormatStyleOptions.rst
+++ b/clang/docs/ClangFormatStyleOptions.rst
@@ -2534,7 +2534,11 @@ the configuration (without a prefix: ``Auto``).
 Use tabs only for indentation.
 
   * ``UT_ForContinuationAndIndentation`` (in configuration: 
``ForContinuationAndIndentation``)
-Use tabs only for line continuation and indentation.
+Fill all leading whitespace with tabs, and use spaces for alignment that
+appears within a line (e.g. consecutive assignments and declarations).
+
+  * ``UT_AlignWithSpaces`` (in configuration: ``AlignWithSpaces``)
+Use tabs for line continuation and indentation, and spaces for alignment.
 
   * ``UT_Always`` (in configuration: ``Always``)
 Use tabs whenever we need to fill whitespace that spans at least from

diff  --git a/clang/include/clang/Format/Format.h 
b/clang/include/clang/Format/Format.h
index d4f76c87c14e..2b2edc4adc11 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -2129,8 +2129,12 @@ struct FormatStyle {
 UT_Never,
 /// Use tabs only for indentation.
 UT_ForIndentation,
-/// Use tabs only for line continuation and indentation.
+/// Fill all leading whitespace with tabs, and use spaces for alignment 
that
+/// appears within a line (e.g. consecutive assignments and declarations).
 UT_ForContinuationAndIndentation,
+/// Use tabs for line continuation and indentation, and spaces for
+/// alignemnt.
+UT_AlignWithSpaces,
 /// Use tabs whenever we need to fill whitespace that spans at least from
 /// one tab stop to the next one.
 UT_Always

diff  --git a/clang/lib/Format/BreakableToken.cpp 
b/clang/lib/Format/BreakableToken.cpp
index 4294c2a653fa..15fbe3b6515d 100644
--- a/clang/lib/Format/BreakableToken.cpp
+++ b/clang/lib/Format/BreakableToken.cpp
@@ -863,7 +863,8 @@ void BreakableLineCommentSection::reflow(unsigned LineIndex,
 // tokens by the empty string.
 Whitespaces.replaceWhitespace(
 *Tokens[LineIndex], /*Newlines=*/0, /*Spaces=*/0,
-/*StartOfTokenColumn=*/StartColumn, /*InPPDirective=*/false);
+/*StartOfTokenColumn=*/StartColumn, /*IsAligned=*/true,
+/*InPPDirective=*/false);
   } else if (LineIndex > 0) {
 // In case we're reflowing after the '\' in:
 //
@@ -931,6 +932,7 @@ void BreakableLineCommentSection::adaptStartOfLine(
   /*Newlines=*/1,
   /*Spaces=*/LineColumn,
   /*StartOfTokenColumn=*/LineColumn,
+  /*IsAligned=*/true,
   /*InPPDirective=*/false);
   }
   if (OriginalPrefix[LineIndex] != Prefix[LineIndex]) {

diff  --git a/clang/lib/Format/ContinuationIndenter.cpp 
b/clang/lib/Format/ContinuationIndenter.cpp
index 2dda5d89a3ac..e70ae7efb0c3 100644
--- a/clang/lib/Format/ContinuationIndenter.cpp
+++ b/clang/lib/Format/ContinuationIndenter.cpp
@@ -642,8 +642,10 @@ void ContinuationIndenter::addTokenOnCurrentLine(LineState 
&State, bool DryRun,
   if (Style.AlignAfterOpenBracket != FormatStyle::BAS_DontAlign &&
   !State.Stack.back().IsCSharpGenericTypeConstraint &&
   Previous.opensScope() && Previous.isNot(TT_ObjCMethodExpr) &&
-  (Current.isNot(TT_LineComment) || Previous.BlockKind == BK_BracedInit))
+  (Current.isNot(TT_LineComment) || Previous.BlockKind == BK_BracedInit)) {
 State.Stack.back().Indent = State.Column + Spaces;
+State.Stack.back().IsAligned = true;
+  }
   if (State.Stack.back().AvoidBinPacking && startsNextParameter(Cur

[PATCH] D77954: [CUDA][HIP] Fix host/device based overload resolution

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 6 inline comments as done.
yaxunl added inline comments.



Comment at: clang/lib/Sema/SemaOverload.cpp:9481
+  // emitted, Cand1 is not better than Cand2. This rule should have precedence
+  // over other rules.
+  //

rjmccall wrote:
> Please add `[CUDA]` or something similar to the top of this comment so that 
> readers can immediately know that it's dialect-specific.
> 
> At a high level, this part of the rule is essentially saying that CUDA 
> non-emittability is a kind of non-viability.  Should we just make 
> non-emittable functions get flagged as non-viable (which will avoid a lot of 
> relatively expensive conversion checking), or is it important to be able to 
> select non-emittable candidates over candidates that are non-viable for other 
> reasons?
There are two situations for "bad" callees:

1. the callee should never be called. It is not just invalid call in codegen, 
but also invalid call in AST. e.g. a host function call a device function. In 
CUDA call preference, it is termed "never". And clang already removed such 
callees from overload candidates.

2. the callee should not be called in codegen, but may be called in AST. This 
happens with `__host__ __device__` functions when calling a "wrong sided" 
function. e.g. in device compilation, a `__host__ __device__` function calls a 
`__host__` function. This is valid in AST since the `__host__ __device__` 
function may be an inline function which is only called by a `__host__` 
function. There is a deferred diagnostic for the wrong-sided call, which is 
triggered only if the caller is emitted. However in overloading resolution, if 
no better candidates are available, wrong-sided candidates are still viable.



Comment at: clang/lib/Sema/SemaOverload.cpp:9749
+  if (isBetterMultiversionCandidate(Cand1, Cand2))
+return true;
+

rjmccall wrote:
> If we move anything below this check, it needs to figure out a tri-state so 
> that it can return false if `Cand2` is a better candidate than `Cand1`.  Now, 
> that only matters if multiversion functions are supported under CUDA, but if 
> you're relying on them not being supported, that should at least be commented 
> on.
multiversion host functions is orthogonal to CUDA therefore should be 
supported. multiversion in device, host device, and global functions are not 
supported. However this change does not make things worse, and should continue 
to work if they are supported.

host/device based overloading resolution is mostly for determining viability of 
a function. If two functions are both viable, other factors should take 
precedence in preference. This general rule has been taken for cases other than 
multiversion, I think it should also apply to multiversion.

I will make isBetterMultiversionCandidate three states.



Comment at: clang/lib/Sema/SemaOverload.cpp:9752
+  // If other rules cannot determine which is better, CUDA preference is used
+  // to determine which is better.
+  if (S.getLangOpts().CUDA && Cand1.Function && Cand2.Function) {

rjmccall wrote:
> Okay, let's think about the right place to put this check in the ordering; we 
> don't want different extensions to get into a who-comes-last competition.
> 
> - Certainly this should have lower priority than the standard-defined 
> preferences like argument conversion ranks or `enable_if` partial-ordering.
> - The preference for pass-object-size parameters is probably most similar to 
> a type-based-overloading decision and so should take priority.
> - I would say that this should take priority over function multi-versioning.  
> Function multi-versioning is all about making specialized versions of the 
> "same function", whereas I think host/device overloading is meant to be 
> semantically broader than that.
> 
> What do you think?
> 
> Regardless, the rationale for the order should be explained in comments.
I will add comments for the rationale of preference.

I commented the preference between multiversion and host/device in another 
comment.


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

https://reviews.llvm.org/D77954



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

[PATCH] D76360: [PPC][AIX] Emit correct Vaarg for 32BIT-AIX in clang

[Sean Fertile via Phabricator via cfe-commits]

sfertile accepted this revision.
sfertile 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/D76360/new/

https://reviews.llvm.org/D76360



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

[PATCH] D77983: clang-tidy doc: add a note for every checker with an autofix

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added a comment.

Makes sense.
I will give it a try :)
thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77983



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

[PATCH] D77974: A Minor change to clang-format-diff.py

[MyDeveloperDay via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG3b379246c365: [clang-format] A Minor change to 
clang-format-diff.py (authored by MyDeveloperDay, committed by paulhoad 
).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77974

Files:
  clang/tools/clang-format/clang-format-diff.py


Index: clang/tools/clang-format/clang-format-diff.py
===
--- clang/tools/clang-format/clang-format-diff.py
+++ clang/tools/clang-format/clang-format-diff.py
@@ -65,7 +65,7 @@
 match = re.search(r'^\+\+\+\ (.*?/){%s}(\S*)' % args.p, line)
 if match:
   filename = match.group(2)
-if filename == None:
+if filename is None:
   continue
 
 if args.regex is not None:


Index: clang/tools/clang-format/clang-format-diff.py
===
--- clang/tools/clang-format/clang-format-diff.py
+++ clang/tools/clang-format/clang-format-diff.py
@@ -65,7 +65,7 @@
 match = re.search(r'^\+\+\+\ (.*?/){%s}(\S*)' % args.p, line)
 if match:
   filename = match.group(2)
-if filename == None:
+if filename is None:
   continue
 
 if args.regex is not None:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75034: [clang-format] use spaces for alignment with UT_ForContinuationAndIndentation

[MyDeveloperDay via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe8111502d869: [clang-format] use spaces for alignment with 
UT_ForContinuationAndIndentation (authored by MyDeveloperDay).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75034

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/BreakableToken.cpp
  clang/lib/Format/ContinuationIndenter.cpp
  clang/lib/Format/ContinuationIndenter.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/UnwrappedLineFormatter.cpp
  clang/lib/Format/WhitespaceManager.cpp
  clang/lib/Format/WhitespaceManager.h
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -10151,17 +10151,18 @@
" \t \tcomment */",
Tab));
   EXPECT_EQ("{\n"
-"  /*\n"
-"   * Comment\n"
-"   */\n"
-"  int i;\n"
+"/*\n"
+" * Comment\n"
+" */\n"
+"int i;\n"
 "}",
 format("{\n"
"\t/*\n"
"\t * Comment\n"
"\t */\n"
"\t int i;\n"
-   "}"));
+   "}",
+   Tab));
 
   Tab.UseTab = FormatStyle::UT_ForContinuationAndIndentation;
   Tab.TabWidth = 8;
@@ -10332,15 +10333,245 @@
"\t*/\n"
"}",
Tab));
+  EXPECT_EQ("/* some\n"
+"   comment */",
+format(" \t \t /* some\n"
+   " \t \tcomment */",
+   Tab));
+  EXPECT_EQ("int a; /* some\n"
+"   comment */",
+format(" \t \t int a; /* some\n"
+   " \t \tcomment */",
+   Tab));
+  EXPECT_EQ("int a; /* some\n"
+"comment */",
+format(" \t \t int\ta; /* some\n"
+   " \t \tcomment */",
+   Tab));
+  EXPECT_EQ("f(\"\t\t\"); /* some\n"
+"comment */",
+format(" \t \t f(\"\t\t\"); /* some\n"
+   " \t \tcomment */",
+   Tab));
+  EXPECT_EQ("{\n"
+"\t/*\n"
+"\t * Comment\n"
+"\t */\n"
+"\tint i;\n"
+"}",
+format("{\n"
+   "\t/*\n"
+   "\t * Comment\n"
+   "\t */\n"
+   "\t int i;\n"
+   "}",
+   Tab));
+  Tab.TabWidth = 2;
+  Tab.IndentWidth = 2;
+  EXPECT_EQ("{\n"
+"\t/* \n"
+"\t\t  */\n"
+"}",
+format("{\n"
+   "/* \n"
+   "\t  */\n"
+   "}",
+   Tab));
+  EXPECT_EQ("{\n"
+"\t/*\n"
+"\t\taa\n"
+"\t\tb\n"
+"\t*/\n"
+"}",
+format("{\n"
+   "/*\n"
+   "\taa b\n"
+   "*/\n"
+   "}",
+   Tab));
+  Tab.AlignConsecutiveAssignments = true;
+  Tab.AlignConsecutiveDeclarations = true;
+  Tab.TabWidth = 4;
+  Tab.IndentWidth = 4;
+  verifyFormat("class Assign {\n"
+   "\tvoid f() {\n"
+   "\t\tint x  = 123;\n"
+   "\t\tint random = 4;\n"
+   "\t\tstd::string alphabet =\n"
+   "\t\t\t\"abcdefghijklmnopqrstuvwxyz\";\n"
+   "\t}\n"
+   "};",
+   Tab);
+
+  Tab.UseTab = FormatStyle::UT_AlignWithSpaces;
+  Tab.TabWidth = 8;
+  Tab.IndentWidth = 8;
+  EXPECT_EQ("if ( && // q\n"
+"bb) // w\n"
+"\t;",
+format("if ( &&// q\n"
+   "bb)// w\n"
+   ";",
+   Tab));
+  EXPECT_EQ("if (aaa && bbb) // w\n"
+"\t;",
+format("if(aaa&&bbb)// w\n"
+   ";",
+   Tab));
+  verifyFormat("class X {\n"
+   "\tvoid f() {\n"
+   "\t\tsomeFunction(parameter1,\n"
+   "\t\t parameter2);\n"
+   "\t}\n"
+   "};",
+   Tab);
+  verifyFormat("#define A\\\n"
+   "\tvoid f() {   \\\n"
+   "\t\tsomeFunction(\\\n"
+   "\t\tparameter1,  \\\n"
+   "\t\tparameter2); \\\n"
+   "\t}",
+   Tab);
+  Tab.TabWidth = 4;
+  Tab.IndentWidth = 8;
+  verifyFormat("class TabWidth4Indent8 {\n"

[PATCH] D77682: [clang-format] Always break line after enum opening brace

[MyDeveloperDay via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG072ae7c1e64f: [clang-format] Always break line after enum 
opening brace (authored by MyDeveloperDay, committed by paulhoad 
).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77682

Files:
  clang/lib/Format/ContinuationIndenter.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -1929,6 +1929,24 @@
"  TWO\n"
"};\n"
"int i;");
+
+  FormatStyle EightIndent = getLLVMStyle();
+  EightIndent.IndentWidth = 8;
+  verifyFormat("enum {\n"
+   "VOID,\n"
+   "CHAR,\n"
+   "SHORT,\n"
+   "INT,\n"
+   "LONG,\n"
+   "SIGNED,\n"
+   "UNSIGNED,\n"
+   "BOOL,\n"
+   "FLOAT,\n"
+   "DOUBLE,\n"
+   "COMPLEX\n"
+   "};",
+   EightIndent);
+
   // Not enums.
   verifyFormat("enum X f() {\n"
"  a();\n"
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -423,7 +423,7 @@
   State.Stack.back().BreakBeforeParameter && Current.CanBreakBefore)
 return true;
 
-  if (State.Column <= NewLineColumn)
+  if (!State.Line->First->is(tok::kw_enum) && State.Column <= NewLineColumn)
 return false;
 
   if (Style.AlwaysBreakBeforeMultilineStrings &&


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -1929,6 +1929,24 @@
"  TWO\n"
"};\n"
"int i;");
+
+  FormatStyle EightIndent = getLLVMStyle();
+  EightIndent.IndentWidth = 8;
+  verifyFormat("enum {\n"
+   "VOID,\n"
+   "CHAR,\n"
+   "SHORT,\n"
+   "INT,\n"
+   "LONG,\n"
+   "SIGNED,\n"
+   "UNSIGNED,\n"
+   "BOOL,\n"
+   "FLOAT,\n"
+   "DOUBLE,\n"
+   "COMPLEX\n"
+   "};",
+   EightIndent);
+
   // Not enums.
   verifyFormat("enum X f() {\n"
"  a();\n"
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -423,7 +423,7 @@
   State.Stack.back().BreakBeforeParameter && Current.CanBreakBefore)
 return true;
 
-  if (State.Column <= NewLineColumn)
+  if (!State.Line->First->is(tok::kw_enum) && State.Column <= NewLineColumn)
 return false;
 
   if (Style.AlwaysBreakBeforeMultilineStrings &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78019: HIP: Fix handling of denormal mode

[Matt Arsenault via Phabricator via cfe-commits]

arsenm created this revision.
arsenm added reviewers: yaxunl, tra.
Herald added subscribers: kerbowa, nhaehnle, wdng, jvesely.
arsenm added a child revision: D78020: clang/AMDGPU: Assume denormals are 
enabled for the default target..

I didn't realize HIP was a distinct offloading kind, so the subtarget
was looking for -march, which isn't correct for HIP. We also have the
possibility of different denormal defaults in the case of multiple
offload targets, so we need to thread the JobAction through the target
hook.


https://reviews.llvm.org/D78019

Files:
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/ToolChains/AMDGPU.cpp
  clang/lib/Driver/ToolChains/AMDGPU.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/Cuda.cpp
  clang/lib/Driver/ToolChains/Cuda.h
  clang/lib/Driver/ToolChains/Linux.cpp
  clang/lib/Driver/ToolChains/Linux.h
  clang/lib/Driver/ToolChains/PS4CPU.h
  clang/test/Driver/cuda-flush-denormals-to-zero.cu

Index: clang/test/Driver/cuda-flush-denormals-to-zero.cu
===
--- clang/test/Driver/cuda-flush-denormals-to-zero.cu
+++ clang/test/Driver/cuda-flush-denormals-to-zero.cu
@@ -7,16 +7,28 @@
 // RUN: %clang -no-canonical-prefixes -### -target x86_64-linux-gnu -c -march=haswell --cuda-gpu-arch=sm_70 -fcuda-flush-denormals-to-zero -nocudainc -nocudalib %s 2>&1 | FileCheck -check-prefix=FTZ %s
 // RUN: %clang -no-canonical-prefixes -### -target x86_64-linux-gnu -c -march=haswell --cuda-gpu-arch=sm_70 -fno-cuda-flush-denormals-to-zero -nocudainc -nocudalib %s 2>&1 | FileCheck -check-prefix=NOFTZ %s
 
-// Test explicit argument.
+// Test explicit argument, with CUDA offload kind
 // RUN: %clang -no-canonical-prefixes -### -target x86_64-linux-gnu -c -march=haswell --cuda-gpu-arch=gfx803 -fcuda-flush-denormals-to-zero -nocudainc -nogpulib %s 2>&1 | FileCheck -check-prefix=FTZ %s
 // RUN: %clang -no-canonical-prefixes -### -target x86_64-linux-gnu -c -march=haswell --cuda-gpu-arch=gfx803 -fno-cuda-flush-denormals-to-zero -nocudainc -nogpulib %s 2>&1 | FileCheck -check-prefix=NOFTZ %s
+
+// Test explicit argument, with HIP offload kind
+// RUN: %clang -x hip -no-canonical-prefixes -### -target x86_64-linux-gnu -c -march=haswell --cuda-gpu-arch=gfx803 -fcuda-flush-denormals-to-zero -nocudainc -nogpulib %s 2>&1 | FileCheck -check-prefix=FTZ %s
+// RUN: %clang -x hip -no-canonical-prefixes -### -target x86_64-linux-gnu -c -march=haswell --cuda-gpu-arch=gfx803 -fno-cuda-flush-denormals-to-zero -nocudainc -nogpulib %s 2>&1 | FileCheck -check-prefix=NOFTZ %s
+
 // RUN: %clang -x hip -no-canonical-prefixes -### -target x86_64-linux-gnu -c -march=haswell --cuda-gpu-arch=gfx900 -fcuda-flush-denormals-to-zero -nocudainc -nogpulib %s 2>&1 | FileCheck -check-prefix=FTZ %s
 // RUN: %clang -x hip -no-canonical-prefixes -### -target x86_64-linux-gnu -c -march=haswell --cuda-gpu-arch=gfx900 -fno-cuda-flush-denormals-to-zero -nocudainc -nogpulib %s 2>&1 | FileCheck -check-prefix=NOFTZ %s
 
-// Test the default changing with no argument based on the subtarget.
+// Test the default changing with no argument based on the subtarget in HIP mode
 // RUN: %clang -x hip -no-canonical-prefixes -### -target x86_64-linux-gnu -c -march=haswell --cuda-gpu-arch=gfx803 -nocudainc -nogpulib %s 2>&1 | FileCheck -check-prefix=FTZ %s
 // RUN: %clang -x hip -no-canonical-prefixes -### -target x86_64-linux-gnu -c -march=haswell --cuda-gpu-arch=gfx900 -nocudainc -nogpulib %s 2>&1 | FileCheck -check-prefix=NOFTZ %s
 
+
+// Test multiple offload archs with different defaults.
+// RUN: %clang -x hip -no-canonical-prefixes -### -target x86_64-linux-gnu -c -march=haswell --cuda-gpu-arch=gfx803 --cuda-gpu-arch=gfx900 -nocudainc -nogpulib %s 2>&1 | FileCheck -check-prefix=MIXED-DEFAULT-MODE %s
+// RUN: %clang -x hip -no-canonical-prefixes -### -target x86_64-linux-gnu -c -march=haswell -fcuda-flush-denormals-to-zero --cuda-gpu-arch=gfx803 --cuda-gpu-arch=gfx900 -nocudainc -nogpulib %s 2>&1 | FileCheck -check-prefix=FTZX2 %s
+// RUN: %clang -x hip -no-canonical-prefixes -### -target x86_64-linux-gnu -c -march=haswell -fno-cuda-flush-denormals-to-zero --cuda-gpu-arch=gfx803 --cuda-gpu-arch=gfx900 -nocudainc -nogpulib %s 2>&1 | FileCheck -check-prefix=NOFTZ %s
+
+
 // CPUFTZ-NOT: -fdenormal-fp-math
 
 // FTZ-NOT: -fdenormal-fp-math-f32=
@@ -25,3 +37,13 @@
 // The default of ieee is omitted
 // NOFTZ-NOT: "-fdenormal-fp-math"
 // NOFTZ-NOT: "-fdenormal-fp-math-f32"
+
+// MIXED-DEFAULT-MODE-NOT: -denormal-fp-math
+// MIXED-DEFAULT-MODE: "-fdenormal-fp-math-f32=preserve-sign,preserve-sign"
+// MIXED-DEFAULT-MODE-SAME: "-target-cpu" "gfx803"
+// MIXED-DEFAULT-MODE-NOT: -denormal-fp-math
+
+// FTZX2: "-fdenormal-fp-math-f32=preserve-sign,preserve-sign"
+// FTZX2-SAME: "-target-cpu" "gfx803"
+// FTZX2: "-fdenormal-fp-math-f32=preserve-sign,preserve-sign"
+// FTZX2-SAME: "-target-cpu" "gfx900"
Index: clang/lib/Driver/ToolChains/PS4CPU.h
===

[PATCH] D76083: [clang-tidy] Expand the list of functions in bugprone-unused-return-value

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp:98
+   "::access;"
+   "::bind;"
+   "::connect;"

aaron.ballman wrote:
> aaron.ballman wrote:
> > sammccall wrote:
> > > aaron.ballman wrote:
> > > > sammccall wrote:
> > > > > aaron.ballman wrote:
> > > > > > jranieri-grammatech wrote:
> > > > > > > alexfh wrote:
> > > > > > > > bind has a side effect and returns a success status. Thus, the 
> > > > > > > > result being unused isn't necessarily a bug. Same for 
> > > > > > > > `connect`. And probably for `setjmp` as well.
> > > > > > > In terms of bind, connect, and setjmp: while I personally would 
> > > > > > > say that code not using the return value is bugprone, the data 
> > > > > > > suggests that the vast majority of developers are using these 
> > > > > > > functions in the intended manner and the false-positive rate 
> > > > > > > should be low.
> > > > > > I think we have sufficient statistical data to suggest that these 
> > > > > > APIs should be on the list because the majority of programmers *do 
> > > > > > not* use them solely for side effects without using the return 
> > > > > > value, so my preference is to keep them in the list.
> > > > > I stumbled upon this review as we're considering turning this check 
> > > > > on by default in clangd.
> > > > > 
> > > > > There's a significant difference between unused std::async() 
> > > > > (programmer misunderstood contract) and unused ::connect() (ignoring 
> > > > > error conditions). The former is ~never noise, and the latter may be 
> > > > > (e.g. in experimental or incomplete code).
> > > > > 
> > > > > So there's some value in separating these two lists out either as an 
> > > > > option or a separate named check (bugprone-unhandled-error?) I think 
> > > > > we probably wouldn't enable this check by default if it includes the 
> > > > > error-code functions.
> > > > > 
> > > > > > the majority of programmers *do not* use them solely for side 
> > > > > > effects
> > > > > ...in popular, distributed software :-)
> > > > > So there's some value in separating these two lists out either as an 
> > > > > option or a separate named check (bugprone-unhandled-error?) I think 
> > > > > we probably wouldn't enable this check by default if it includes the 
> > > > > error-code functions.
> > > > 
> > > > I think that adds complexity to this check when the complexity isn't 
> > > > necessary. clang-tidy has traditionally been a place for checks that 
> > > > are chattier than what the compiler should provide, and this check has 
> > > > a trivial, well-understood mechanism to silence the diagnostics (cast 
> > > > to void) which also expresses intent properly to the toolchain.
> > > > 
> > > > >>the majority of programmers *do not* use them solely for side effects
> > > > > ...in popular, distributed software :-)
> > > > 
> > > > I have not seen anyone provide data to suggest that the functions in 
> > > > question appear in any statistically significant amount in practice 
> > > > without checking the return value, just worries that they *could*. I 
> > > > don't think that's compelling in the face of data. Remember, this is 
> > > > for bugprone patterns, not bugknown patterns.
> > > I think we're talking past each other here. I'm not saying clang-tidy 
> > > shouldn't have the check, or that it's not a bugprone pattern, or that 
> > > the clang-tidy default should be different.
> > > 
> > > But there are scenarios where you want one but not the other. Concretely, 
> > > warnings shown in an IDE **as you type and by default**. If you're 
> > > misusing an API rendering it completely useless, you should see that 
> > > ASAP. If you fail to check an error code, some users won't want to be 
> > > warned about that until later.
> > > 
> > > By bundling them into a single check without options (other than 
> > > duplicating the whole list), it's hard to create that useful but 
> > > inoffensive default setup. That's OK, clangd can remove the check from 
> > > the whitelist, but I think we'd get a lot of value out of it.
> > > 
> > > > I have not seen anyone provide data to suggest that the functions in 
> > > > question appear in any statistically significant amount in practice
> > > Right, we don't have data either way on incomplete code. Based on 
> > > experience of writing code and watching others write code, I believe 
> > > people write sloppy code they'd never check in, and appreciate being told 
> > > early when it doesn't do what they intend, but some don't appreciate 
> > > being told to be less sloppy. Is your intuition different? Do you think 
> > > the data provided addresses this question?
> > > But there are scenarios where you want one but not the other. Concretely, 
> > > warnings shown in an IDE as you type and by default. If you're misusing 
>

[PATCH] D77954: [CUDA][HIP] Fix host/device based overload resolution

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 256973.
yaxunl marked 3 inline comments as done.
yaxunl added a comment.

fix preference for multiversion. add comments. add more tests for wrong-sided 
function.


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

https://reviews.llvm.org/D77954

Files:
  clang/lib/Sema/SemaOverload.cpp
  clang/test/SemaCUDA/function-overload.cu

Index: clang/test/SemaCUDA/function-overload.cu
===
--- clang/test/SemaCUDA/function-overload.cu
+++ clang/test/SemaCUDA/function-overload.cu
@@ -331,9 +331,6 @@
 // If we have a mix of HD and H-only or D-only candidates in the overload set,
 // normal C++ overload resolution rules apply first.
 template  TemplateReturnTy template_vs_hd_function(T arg)
-#ifdef __CUDA_ARCH__
-//expected-note@-2 {{declared here}}
-#endif
 {
   return TemplateReturnTy();
 }
@@ -342,11 +339,13 @@
 }
 
 __host__ __device__ void test_host_device_calls_hd_template() {
-  HostDeviceReturnTy ret1 = template_vs_hd_function(1.0f);
-  TemplateReturnTy ret2 = template_vs_hd_function(1);
 #ifdef __CUDA_ARCH__
-  // expected-error@-2 {{reference to __host__ function 'template_vs_hd_function' in __host__ __device__ function}}
+  typedef HostDeviceReturnTy ExpectedReturnTy;
+#else
+  typedef TemplateReturnTy ExpectedReturnTy;
 #endif
+  HostDeviceReturnTy ret1 = template_vs_hd_function(1.0f);
+  ExpectedReturnTy ret2 = template_vs_hd_function(1);
 }
 
 __host__ void test_host_calls_hd_template() {
@@ -367,14 +366,14 @@
 __device__ DeviceReturnTy device_only_function(int arg) { return DeviceReturnTy(); }
 __device__ DeviceReturnTy2 device_only_function(float arg) { return DeviceReturnTy2(); }
 #ifndef __CUDA_ARCH__
-  // expected-note@-3 {{'device_only_function' declared here}}
-  // expected-note@-3 {{'device_only_function' declared here}}
+  // expected-note@-3 2{{'device_only_function' declared here}}
+  // expected-note@-3 2{{'device_only_function' declared here}}
 #endif
 __host__ HostReturnTy host_only_function(int arg) { return HostReturnTy(); }
 __host__ HostReturnTy2 host_only_function(float arg) { return HostReturnTy2(); }
 #ifdef __CUDA_ARCH__
-  // expected-note@-3 {{'host_only_function' declared here}}
-  // expected-note@-3 {{'host_only_function' declared here}}
+  // expected-note@-3 2{{'host_only_function' declared here}}
+  // expected-note@-3 2{{'host_only_function' declared here}}
 #endif
 
 __host__ __device__ void test_host_device_single_side_overloading() {
@@ -392,6 +391,37 @@
 #endif
 }
 
+// wrong-sided overloading should not cause diagnostic unless it is emitted.
+// This inline function is not emitted.
+inline __host__ __device__ void test_host_device_wrong_side_overloading_inline_no_diag() {
+  DeviceReturnTy ret1 = device_only_function(1);
+  DeviceReturnTy2 ret2 = device_only_function(1.0f);
+  HostReturnTy ret3 = host_only_function(1);
+  HostReturnTy2 ret4 = host_only_function(1.0f);
+}
+
+// wrong-sided overloading should cause diagnostic if it is emitted.
+// This inline function is emitted since it is called by an emitted function.
+inline __host__ __device__ void test_host_device_wrong_side_overloading_inline_diag() {
+  DeviceReturnTy ret1 = device_only_function(1);
+  DeviceReturnTy2 ret2 = device_only_function(1.0f);
+#ifndef __CUDA_ARCH__
+  // expected-error@-3 {{reference to __device__ function 'device_only_function' in __host__ __device__ function}}
+  // expected-error@-3 {{reference to __device__ function 'device_only_function' in __host__ __device__ function}}
+#endif
+  HostReturnTy ret3 = host_only_function(1);
+  HostReturnTy2 ret4 = host_only_function(1.0f);
+#ifdef __CUDA_ARCH__
+  // expected-error@-3 {{reference to __host__ function 'host_only_function' in __host__ __device__ function}}
+  // expected-error@-3 {{reference to __host__ function 'host_only_function' in __host__ __device__ function}}
+#endif
+}
+
+__host__ __device__ void test_host_device_wrong_side_overloading_inline_diag_caller() {
+  test_host_device_wrong_side_overloading_inline_diag();
+  // expected-note@-1 {{called by 'test_host_device_wrong_side_overloading_inline_diag_caller'}}
+}
+
 // Verify that we allow overloading function templates.
 template  __host__ T template_overload(const T &a) { return a; };
 template  __device__ T template_overload(const T &a) { return a; };
Index: clang/lib/Sema/SemaOverload.cpp
===
--- clang/lib/Sema/SemaOverload.cpp
+++ clang/lib/Sema/SemaOverload.cpp
@@ -9374,16 +9374,19 @@
   return Comparison::Equal;
 }
 
-static bool isBetterMultiversionCandidate(const OverloadCandidate &Cand1,
-  const OverloadCandidate &Cand2) {
+static Comparison
+isBetterMultiversionCandidate(const OverloadCandidate &Cand1,
+  const OverloadCandidate &Cand2) {
   if (!Cand1.Function || !Cand1.Function->isMultiVersion() || !Cand2.Function 

[PATCH] D77233: [NFC] Refactoring PropertyAttributeKind for ObjCPropertyDecl and ObjCDeclSpec.

[Puyan Lotfi via Phabricator via cfe-commits]

plotfi added inline comments.



Comment at: clang/include/clang-c/Index.h:4503
   CXObjCPropertyAttr_unsafe_unretained = 0x800,
-  CXObjCPropertyAttr_class = 0x1000
+  CXObjCPropertyAttr_classattr = 0x1000
 } CXObjCPropertyAttrKind;

@erik.pilkington  Do you think we should be adding the C-API analogs of 
nullability / null_resettable (and direct) to clang/include/clang-c/Index.h? 

I noticed those are missing.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77233



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

[PATCH] D78020: clang/AMDGPU: Assume denormals are enabled for the default target.

[Matt Arsenault via Phabricator via cfe-commits]

arsenm created this revision.
arsenm added reviewers: yaxunl, rampitec, b-sumner, msearles.
Herald added subscribers: kerbowa, t-tye, tpr, dstuttard, nhaehnle, wdng, 
jvesely, kzhuravl.
arsenm added a parent revision: D78019: HIP: Fix handling of denormal mode.

Since the default logic was based on having fast denormal/fma
features, and the default target has no features, we assumed flushing by
default. This fixes incorrectly assuming flushing in builds for
"generic" IR libraries.

  

The handling for no specified --cuda-gpu-arch in HIP is kind of
broken. Somewhere else forces a default target of gfx803, which does
not enable denormal handling by default. We don't see this default
switching here, so you'll end up with a different denormal mode
depending on whether you explicitly requested gfx803, or used it by
default.


https://reviews.llvm.org/D78020

Files:
  clang/lib/Driver/ToolChains/AMDGPU.cpp
  clang/test/Driver/cl-denorms-are-zero.cl
  clang/test/Driver/cuda-flush-denormals-to-zero.cu


Index: clang/test/Driver/cuda-flush-denormals-to-zero.cu
===
--- clang/test/Driver/cuda-flush-denormals-to-zero.cu
+++ clang/test/Driver/cuda-flush-denormals-to-zero.cu
@@ -22,6 +22,10 @@
 // RUN: %clang -x hip -no-canonical-prefixes -### -target x86_64-linux-gnu -c 
-march=haswell --cuda-gpu-arch=gfx803 -nocudainc -nogpulib %s 2>&1 | FileCheck 
-check-prefix=FTZ %s
 // RUN: %clang -x hip -no-canonical-prefixes -### -target x86_64-linux-gnu -c 
-march=haswell --cuda-gpu-arch=gfx900 -nocudainc -nogpulib %s 2>&1 | FileCheck 
-check-prefix=NOFTZ %s
 
+// Test no subtarget
+// FIXME: This defaults to gfx803, but gets a different denormal mode than 
explicitly specifying it.
+// RUN: %clang -x hip -no-canonical-prefixes -### -target x86_64-linux-gnu -c 
-march=haswell -nocudainc -nogpulib %s 2>&1 | FileCheck -check-prefix=NOFTZ %s
+
 
 // Test multiple offload archs with different defaults.
 // RUN: %clang -x hip -no-canonical-prefixes -### -target x86_64-linux-gnu -c 
-march=haswell --cuda-gpu-arch=gfx803 --cuda-gpu-arch=gfx900 -nocudainc 
-nogpulib %s 2>&1 | FileCheck -check-prefix=MIXED-DEFAULT-MODE %s
Index: clang/test/Driver/cl-denorms-are-zero.cl
===
--- clang/test/Driver/cl-denorms-are-zero.cl
+++ clang/test/Driver/cl-denorms-are-zero.cl
@@ -1,20 +1,24 @@
 // Slow FMAF and slow f32 denormals
-// RUN: %clang -### -target amdgcn--amdhsa -c -mcpu=pitcairn %s 2>&1 | 
FileCheck -check-prefixes=AMDGCN,AMDGCN-FLUSH %s
+// RUN: %clang -### -target amdgcn--amdhsa -nogpulib -c -mcpu=pitcairn %s 2>&1 
| FileCheck -check-prefixes=AMDGCN,AMDGCN-FLUSH %s
 // RUN: %clang -### -cl-denorms-are-zero -o - -target amdgcn--amdhsa -c 
-mcpu=pitcairn %s 2>&1 | FileCheck -check-prefixes=AMDGCN,AMDGCN-FLUSH %s
 
 // Fast FMAF, but slow f32 denormals
-// RUN: %clang -### -target amdgcn--amdhsa -c -mcpu=tahiti %s 2>&1 | FileCheck 
-check-prefixes=AMDGCN,AMDGCN-FLUSH %s
+// RUN: %clang -### -target amdgcn--amdhsa -nogpulib -c -mcpu=tahiti %s 2>&1 | 
FileCheck -check-prefixes=AMDGCN,AMDGCN-FLUSH %s
 // RUN: %clang -### -cl-denorms-are-zero -o - -target amdgcn--amdhsa -c 
-mcpu=tahiti %s 2>&1 | FileCheck -check-prefixes=AMDGCN,AMDGCN-FLUSH %s
 
 // Fast F32 denormals, but slow FMAF
-// RUN: %clang -### -target amdgcn--amdhsa -c -mcpu=fiji %s 2>&1 | FileCheck 
-check-prefixes=AMDGCN,AMDGCN-FLUSH %s
+// RUN: %clang -### -target amdgcn--amdhsa -nogpulib -c -mcpu=fiji %s 2>&1 | 
FileCheck -check-prefixes=AMDGCN,AMDGCN-FLUSH %s
 // RUN: %clang -### -cl-denorms-are-zero -o - -target amdgcn--amdhsa -c 
-mcpu=fiji %s 2>&1 | FileCheck -check-prefixes=AMDGCN,AMDGCN-FLUSH %s
 
 // Fast F32 denormals and fast FMAF
-// RUN: %clang -### -target amdgcn--amdhsa -c -mcpu=gfx900 %s 2>&1 | FileCheck 
-check-prefixes=AMDGCN,AMDGCN-DENORM %s
-// RUN: %clang -### -cl-denorms-are-zero -o - -target amdgcn--amdhsa -c 
-mcpu=gfx900 %s 2>&1 | FileCheck -check-prefixes=AMDGCN,AMDGCN-FLUSH %s
+// RUN: %clang -### -target amdgcn--amdhsa -nogpulib -c -mcpu=gfx900 %s 2>&1 | 
FileCheck -check-prefixes=AMDGCN,AMDGCN-DENORM %s
+// RUN: %clang -### -cl-denorms-are-zero -o - -target amdgcn--amdhsa -nogpulib 
-c -mcpu=gfx900 %s 2>&1 | FileCheck -check-prefixes=AMDGCN,AMDGCN-FLUSH %s
+
+// Default target is artificial, but should assume a conservative default.
+// RUN: %clang -### -target amdgcn--amdhsa -nogpulib -c %s 2>&1 | FileCheck 
-check-prefixes=AMDGCN,AMDGCN-DENORM %s
+// RUN: %clang -### -cl-denorms-are-zero -o - -target amdgcn--amdhsa -nogpulib 
-c %s 2>&1 | FileCheck -check-prefixes=AMDGCN,AMDGCN-FLUSH %s
 
 // AMDGCN-FLUSH: "-fdenormal-fp-math-f32=preserve-sign,preserve-sign"
 
 // This should be omitted and default to ieee
-// AMDGCN-DENORM-NOT: "-fdenormal-fp-math-f32"
+// AMDGCN-DENORM-NOT: denormal-fp-math
Index: clang/lib/Driver/ToolChains/AMDGPU.cpp
===
--- clang/lib/Dr

[PATCH] D76182: [AVR] Support aliases in non-zero address space

[Ayke via Phabricator via cfe-commits]

aykevl updated this revision to Diff 256977.
aykevl added a comment.

- added test
- using `getPointerAddressSpace` instead of a cast

This is now ready for review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76182

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGen/alias-avr.c


Index: clang/test/CodeGen/alias-avr.c
===
--- /dev/null
+++ clang/test/CodeGen/alias-avr.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -triple avr-unknown-unknown -emit-llvm -o - %s | FileCheck 
%s
+
+int mul(int a, int b) {
+   return a * b;
+}
+
+// CHECK: @multiply = alias i16 (i16, i16), i16 (i16, i16) addrspace(1)* @mul
+int multiply(int x, int y) __attribute__((alias("mul")));
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -4547,8 +4547,9 @@
   }
 
   // Create the new alias itself, but don't set a name yet.
+  unsigned AS = Aliasee->getType()->getPointerAddressSpace();
   auto *GA =
-  llvm::GlobalAlias::create(DeclTy, 0, LT, "", Aliasee, &getModule());
+  llvm::GlobalAlias::create(DeclTy, AS, LT, "", Aliasee, &getModule());
 
   if (Entry) {
 if (GA->getAliasee() == Entry) {


Index: clang/test/CodeGen/alias-avr.c
===
--- /dev/null
+++ clang/test/CodeGen/alias-avr.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -triple avr-unknown-unknown -emit-llvm -o - %s | FileCheck %s
+
+int mul(int a, int b) {
+	return a * b;
+}
+
+// CHECK: @multiply = alias i16 (i16, i16), i16 (i16, i16) addrspace(1)* @mul
+int multiply(int x, int y) __attribute__((alias("mul")));
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -4547,8 +4547,9 @@
   }
 
   // Create the new alias itself, but don't set a name yet.
+  unsigned AS = Aliasee->getType()->getPointerAddressSpace();
   auto *GA =
-  llvm::GlobalAlias::create(DeclTy, 0, LT, "", Aliasee, &getModule());
+  llvm::GlobalAlias::create(DeclTy, AS, LT, "", Aliasee, &getModule());
 
   if (Entry) {
 if (GA->getAliasee() == Entry) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77923: OpenCL: Fix some missing predefined macros

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

In D77923#1976497 , @jvesely wrote:

> OPENCL_VERSION is something that should be really set by the opencl driver 
> rather than the compiler.
>  coarse-grained SVM can be done without FEATURE_FLAT_ADDRESS_SPACE, so those 
> gpus can get over 1.2, while the compiler can be used in an implementation 
> that doesn't go above 1.2 even for gfx700+


The offline compiler needs to be perfectly usable, with no driver/runtime 
dependencies. Any information needed from the driver should be captured by the 
target triple/OS type. We would invent a new OS type if there was such a need


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

https://reviews.llvm.org/D77923



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

[PATCH] D77233: [NFC] Refactoring PropertyAttributeKind for ObjCPropertyDecl and ObjCDeclSpec.

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington requested changes to this revision.
erik.pilkington added inline comments.
This revision now requires changes to proceed.



Comment at: clang/include/clang-c/Index.h:4503
   CXObjCPropertyAttr_unsafe_unretained = 0x800,
-  CXObjCPropertyAttr_class = 0x1000
+  CXObjCPropertyAttr_classattr = 0x1000
 } CXObjCPropertyAttrKind;

plotfi wrote:
> @erik.pilkington  Do you think we should be adding the C-API analogs of 
> nullability / null_resettable (and direct) to clang/include/clang-c/Index.h? 
> 
> I noticed those are missing.
> 
> 
Oh, sorry, I missed this. The C API is supposed to be stable, so I don't think 
we should change the name of this enumerator (@arphaman can you confirm?). 
Adding the missing attributes seem fine though. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77233



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

[PATCH] D78019: HIP: Fix handling of denormal mode

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added inline comments.



Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:286
+
+// FIXME: Should this use the default mode based on the target? How do we
+// deal with multiple --cuda-gpu-arch?

If there are multiple --cuda-gpu-arch, driver will create separate JobAction 
for launching separate `clang -cc1` command for each arch. This function is 
called for each JobAction and  getOffloadingArch contains the single arch. 
Therefore there is no issue for multiple --cuda-gpu-arch and this comment can 
be removed.



Comment at: clang/test/Driver/cuda-flush-denormals-to-zero.cu:27
+// Test multiple offload archs with different defaults.
+// RUN: %clang -x hip -no-canonical-prefixes -### -target x86_64-linux-gnu -c 
-march=haswell --cuda-gpu-arch=gfx803 --cuda-gpu-arch=gfx900 -nocudainc 
-nogpulib %s 2>&1 | FileCheck -check-prefix=MIXED-DEFAULT-MODE %s
+// RUN: %clang -x hip -no-canonical-prefixes -### -target x86_64-linux-gnu -c 
-march=haswell -fcuda-flush-denormals-to-zero --cuda-gpu-arch=gfx803 
--cuda-gpu-arch=gfx900 -nocudainc -nogpulib %s 2>&1 | FileCheck 
-check-prefix=FTZX2 %s

this will result in multiple clang -cc1 commands, each one corresponding to an 
arch. You need to check each arch.


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

https://reviews.llvm.org/D78019



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

[PATCH] D76360: [PPC][AIX] Emit correct Vaarg for 32BIT-AIX in clang

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.



Comment at: clang/test/CodeGen/ppc32-struct-return.c:53
+
+// AIX-SVR4: fatal error: error in backend: -msvr4-struct-return not supported 
on AIX
+ 

If certain front end option is not supported on certain target, I think it 
makes more sense to have a standard diagnostic in the driver component, instead 
of "crash" in the backend. 
i.e. What if we specify this option on a Windows machine? Maybe we should 
pursue the same behavior. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76360



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

[PATCH] D69171: [clang-fuzzer] Add new fuzzer target for Objective-C

[Matt Morehouse via Phabricator via cfe-commits]

morehouse added inline comments.



Comment at: clang/tools/clang-fuzzer/ClangObjectiveCFuzzer.cpp:19
 
-extern "C" int LLVMFuzzerInitialize(int *argc, char ***argv) { return 0; }
-

RKSimon wrote:
> @dgoldman @morehouse Removing this has been causing link failures ever since 
> it was committed on a number of targets including msvc (PR44414) and I've 
> just noticed it on a linux release+asserts build. If it is superfluous why 
> did you leave it in ClangFuzzer.cpp ?
It is superfluous on Linux at least since libFuzzer defines its own weak 
definition.  I'm not sure how it works for Windows.

I'm fine if you want to add it back in here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69171



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

[clang] a59ba33 - Fix an indent.

[Nico Weber via cfe-commits]

Author: Nico Weber
Date: 2020-04-13T11:54:00-04:00
New Revision: a59ba3384e9d5e1cf89ddae1d14601a613b8d7b7

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

LOG: Fix an indent.

Added: 


Modified: 
clang/test/lit.cfg.py

Removed: 




diff  --git a/clang/test/lit.cfg.py b/clang/test/lit.cfg.py
index 18b5a2991f7e..0358a9d8a959 100644
--- a/clang/test/lit.cfg.py
+++ b/clang/test/lit.cfg.py
@@ -171,7 +171,7 @@ def calculate_arch_features(arch_string):
 llvm_config.feature_config(
 [('--assertion-mode', {'ON': 'asserts'}),
  ('--cxxflags', {r'-D_GLIBCXX_DEBUG\b': 'libstdcxx-safe-mode'}),
-('--targets-built', calculate_arch_features)
+ ('--targets-built', calculate_arch_features),
  ])
 
 if lit.util.which('xmllint'):



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

[PATCH] D77984: Make IRBuilder automatically set alignment on load/store/alloca.

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

I think this is fine. Two comments to consider below.




Comment at: llvm/include/llvm/IR/IRBuilder.h:1600
+return CreateAlignedLoad(Ty, Ptr, DL.getABITypeAlign(Ty), isVolatile, 
Name);
   }
 

Can't we just pawn of the alignment stuff to `CreateAlignedLoad`? We cal lit 
with 0 here and we determine an alignment there anyway?
Nit: `BB->getModule()` should work.




Comment at: llvm/include/llvm/IR/IRBuilder.h:1622
+return CreateAlignedStore(Val, Ptr, StoreAlign, isVolatile);
   }
 

Same as for the load.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77984



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

[PATCH] D77632: [TLI] Per-function fveclib for math library used for vectorization

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In D77632#1976308 , @mehdi_amini wrote:

> In D77632#1976231 , @wenlei wrote:
>
> > And agree with @tejohnson, if the openness is a feature, it should be 
> > covered in tests, otherwise it can feel somewhat like a loophole and prone 
> > to breakage
>
>
> The thing is that LLVM does not have much C++ unittests in general, so most 
> of the "features" like this one that LLVM offers as a library are just an 
> artifact of being "designed as a library" and being mindful about the 
> layering.
>  From this point of view this patch is changing the design of a component 
> that was modular/pluggable into a closed system.


The interfaces being relied on were in the underlying Impl class, I think if 
that is expected to be pluggable and stable it really needs unit testing to 
reflect that usage.

> I'm perfectly fine with trying to add a unit-test, I just don't know yet 
> where it would fit in the LLVM testing though.

Presumably the testing should be in 
llvm/unittests/Analysis/TargetLibraryInfoTest.cpp, which already exists but 
only tests the LibFuncs (builtins) interfaces.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77632



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

[PATCH] D78027: [TimeProfiler] Emit real process ID and thread names

[Sergej Jaskiewicz via Phabricator via cfe-commits]

broadwaylamb created this revision.
broadwaylamb added reviewers: anton-afanasyev, russell.gallop.
Herald added subscribers: cfe-commits, MaskRay, hiraditya, emaste.
Herald added a reviewer: espindola.
Herald added a project: clang.
broadwaylamb added a parent revision: D78022: Introduce 
llvm::sys::Process::getProcessId() and adopt it.
broadwaylamb updated this revision to Diff 256993.
broadwaylamb added a comment.

Remove unrelated changes


https://reviews.llvm.org/D78027

Files:
  clang/test/Driver/check-time-trace.cpp
  lld/test/ELF/time-trace.s
  llvm/lib/Support/TimeProfiler.cpp

Index: llvm/lib/Support/TimeProfiler.cpp
===
--- llvm/lib/Support/TimeProfiler.cpp
+++ llvm/lib/Support/TimeProfiler.cpp
@@ -15,6 +15,7 @@
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/Process.h"
 #include "llvm/Support/Threading.h"
 #include 
 #include 
@@ -73,10 +74,18 @@
   }
 };
 
+static std::string getThreadName() {
+  SmallString<64> Name;
+  llvm::get_thread_name(Name);
+  return std::string(std::move(Name));
+}
+
 struct TimeTraceProfiler {
   TimeTraceProfiler(unsigned TimeTraceGranularity = 0, StringRef ProcName = "")
   : StartTime(steady_clock::now()), ProcName(ProcName),
-Tid(llvm::get_threadid()), TimeTraceGranularity(TimeTraceGranularity) {}
+Pid(sys::Process::getProcessId()), ThreadName(getThreadName()),
+Tid(llvm::get_threadid()),
+TimeTraceGranularity(TimeTraceGranularity) {}
 
   void begin(std::string Name, llvm::function_ref Detail) {
 Stack.emplace_back(steady_clock::now(), TimePointType(), std::move(Name),
@@ -141,7 +150,7 @@
   auto DurUs = E.getFlameGraphDurUs();
 
   J.object([&]{
-J.attribute("pid", 1);
+J.attribute("pid", Pid);
 J.attribute("tid", int64_t(Tid));
 J.attribute("ph", "X");
 J.attribute("ts", StartUs);
@@ -205,7 +214,7 @@
   auto Count = AllCountAndTotalPerName[Total.first].first;
 
   J.object([&]{
-J.attribute("pid", 1);
+J.attribute("pid", Pid);
 J.attribute("tid", int64_t(TotalTid));
 J.attribute("ph", "X");
 J.attribute("ts", 0);
@@ -220,16 +229,25 @@
   ++TotalTid;
 }
 
-// Emit metadata event with process name.
-J.object([&] {
-  J.attribute("cat", "");
-  J.attribute("pid", 1);
-  J.attribute("tid", 0);
-  J.attribute("ts", 0);
-  J.attribute("ph", "M");
-  J.attribute("name", "process_name");
-  J.attributeObject("args", [&] { J.attribute("name", ProcName); });
-});
+auto writeMetadataEvent = [&](const char* Name, uint64_t Tid, auto arg) {
+  J.object([&] {
+J.attribute("cat", "");
+J.attribute("pid", Pid);
+J.attribute("tid", int64_t(Tid));
+J.attribute("ts", 0);
+J.attribute("ph", "M");
+J.attribute("name", Name);
+J.attributeObject("args", [&] { J.attribute("name", arg); });
+  });
+};
+
+writeMetadataEvent("process_name", Tid, ProcName);
+
+writeMetadataEvent("thread_name", Tid, ThreadName);
+
+for (const auto &TTP : ThreadTimeTraceProfilerInstances) {
+  writeMetadataEvent("thread_name", TTP->Tid, TTP->ThreadName);
+}
 
 J.arrayEnd();
 J.attributeEnd();
@@ -241,6 +259,8 @@
   StringMap CountAndTotalPerName;
   const TimePointType StartTime;
   const std::string ProcName;
+  const sys::Process::Pid Pid;
+  const std::string ThreadName;
   const uint64_t Tid;
 
   // Minimum time granularity (in microseconds)
Index: lld/test/ELF/time-trace.s
===
--- lld/test/ELF/time-trace.s
+++ lld/test/ELF/time-trace.s
@@ -34,6 +34,7 @@
 # Check process_name entry field
 # CHECK: "name": "ld.lld{{(.exe)?}}"
 # CHECK: "name": "process_name"
+# CHECK: "name": "thread_name"
 
 .globl _start
 _start:
Index: clang/test/Driver/check-time-trace.cpp
===
--- clang/test/Driver/check-time-trace.cpp
+++ clang/test/Driver/check-time-trace.cpp
@@ -14,6 +14,7 @@
 // CHECK-NEXT: "ts":
 // CHECK: "name": "clang{{.*}}"
 // CHECK: "name": "process_name"
+// CHECK: "name": "thread_name"
 
 template 
 struct Struct {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78027: [TimeProfiler] Emit real process ID and thread names

[Sergej Jaskiewicz via Phabricator via cfe-commits]

broadwaylamb updated this revision to Diff 256993.
broadwaylamb added a comment.

Remove unrelated changes


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

https://reviews.llvm.org/D78027

Files:
  clang/test/Driver/check-time-trace.cpp
  lld/test/ELF/time-trace.s
  llvm/lib/Support/TimeProfiler.cpp

Index: llvm/lib/Support/TimeProfiler.cpp
===
--- llvm/lib/Support/TimeProfiler.cpp
+++ llvm/lib/Support/TimeProfiler.cpp
@@ -15,6 +15,7 @@
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/Process.h"
 #include "llvm/Support/Threading.h"
 #include 
 #include 
@@ -73,10 +74,18 @@
   }
 };
 
+static std::string getThreadName() {
+  SmallString<64> Name;
+  llvm::get_thread_name(Name);
+  return std::string(std::move(Name));
+}
+
 struct TimeTraceProfiler {
   TimeTraceProfiler(unsigned TimeTraceGranularity = 0, StringRef ProcName = "")
   : StartTime(steady_clock::now()), ProcName(ProcName),
-Tid(llvm::get_threadid()), TimeTraceGranularity(TimeTraceGranularity) {}
+Pid(sys::Process::getProcessId()), ThreadName(getThreadName()),
+Tid(llvm::get_threadid()),
+TimeTraceGranularity(TimeTraceGranularity) {}
 
   void begin(std::string Name, llvm::function_ref Detail) {
 Stack.emplace_back(steady_clock::now(), TimePointType(), std::move(Name),
@@ -141,7 +150,7 @@
   auto DurUs = E.getFlameGraphDurUs();
 
   J.object([&]{
-J.attribute("pid", 1);
+J.attribute("pid", Pid);
 J.attribute("tid", int64_t(Tid));
 J.attribute("ph", "X");
 J.attribute("ts", StartUs);
@@ -205,7 +214,7 @@
   auto Count = AllCountAndTotalPerName[Total.first].first;
 
   J.object([&]{
-J.attribute("pid", 1);
+J.attribute("pid", Pid);
 J.attribute("tid", int64_t(TotalTid));
 J.attribute("ph", "X");
 J.attribute("ts", 0);
@@ -220,16 +229,25 @@
   ++TotalTid;
 }
 
-// Emit metadata event with process name.
-J.object([&] {
-  J.attribute("cat", "");
-  J.attribute("pid", 1);
-  J.attribute("tid", 0);
-  J.attribute("ts", 0);
-  J.attribute("ph", "M");
-  J.attribute("name", "process_name");
-  J.attributeObject("args", [&] { J.attribute("name", ProcName); });
-});
+auto writeMetadataEvent = [&](const char* Name, uint64_t Tid, auto arg) {
+  J.object([&] {
+J.attribute("cat", "");
+J.attribute("pid", Pid);
+J.attribute("tid", int64_t(Tid));
+J.attribute("ts", 0);
+J.attribute("ph", "M");
+J.attribute("name", Name);
+J.attributeObject("args", [&] { J.attribute("name", arg); });
+  });
+};
+
+writeMetadataEvent("process_name", Tid, ProcName);
+
+writeMetadataEvent("thread_name", Tid, ThreadName);
+
+for (const auto &TTP : ThreadTimeTraceProfilerInstances) {
+  writeMetadataEvent("thread_name", TTP->Tid, TTP->ThreadName);
+}
 
 J.arrayEnd();
 J.attributeEnd();
@@ -241,6 +259,8 @@
   StringMap CountAndTotalPerName;
   const TimePointType StartTime;
   const std::string ProcName;
+  const sys::Process::Pid Pid;
+  const std::string ThreadName;
   const uint64_t Tid;
 
   // Minimum time granularity (in microseconds)
Index: lld/test/ELF/time-trace.s
===
--- lld/test/ELF/time-trace.s
+++ lld/test/ELF/time-trace.s
@@ -34,6 +34,7 @@
 # Check process_name entry field
 # CHECK: "name": "ld.lld{{(.exe)?}}"
 # CHECK: "name": "process_name"
+# CHECK: "name": "thread_name"
 
 .globl _start
 _start:
Index: clang/test/Driver/check-time-trace.cpp
===
--- clang/test/Driver/check-time-trace.cpp
+++ clang/test/Driver/check-time-trace.cpp
@@ -14,6 +14,7 @@
 // CHECK-NEXT: "ts":
 // CHECK: "name": "clang{{.*}}"
 // CHECK: "name": "process_name"
+// CHECK: "name": "thread_name"
 
 template 
 struct Struct {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

[Melanie Blower via Phabricator via cfe-commits]

mibintc updated this revision to Diff 256998.
mibintc added a comment.

I made the changes requested by @rjmccall ; I also used clang-format on the 
tip. check-clang is passing.  Look OK?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76384

Files:
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/ExprCXX.h
  clang/include/clang/AST/Stmt.h
  clang/include/clang/Basic/LangOptions.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprCXX.cpp
  clang/lib/Analysis/BodyFarm.cpp
  clang/lib/Basic/LangOptions.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/CodeGen/CGObjC.cpp
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/lib/Frontend/Rewrite/RewriteModernObjC.cpp
  clang/lib/Frontend/Rewrite/RewriteObjC.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaPseudoObject.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp

Index: clang/lib/Serialization/ASTWriterStmt.cpp
===
--- clang/lib/Serialization/ASTWriterStmt.cpp
+++ clang/lib/Serialization/ASTWriterStmt.cpp
@@ -918,11 +918,16 @@
 
 void ASTStmtWriter::VisitBinaryOperator(BinaryOperator *E) {
   VisitExpr(E);
+  bool HasFPFeatures = E->hasStoredFPFeatures();
+  // Write this first for easy access when deserializing, as they affect the
+  // size of the UnaryOperator.
+  Record.push_back(HasFPFeatures);
+  Record.push_back(E->getOpcode()); // FIXME: stable encoding
   Record.AddStmt(E->getLHS());
   Record.AddStmt(E->getRHS());
-  Record.push_back(E->getOpcode()); // FIXME: stable encoding
   Record.AddSourceLocation(E->getOperatorLoc());
-  Record.push_back(E->getFPFeatures().getInt());
+  if (HasFPFeatures)
+Record.push_back(E->getStoredFPFeatures().getAsOpaqueInt());
   Code = serialization::EXPR_BINARY_OPERATOR;
 }
 
@@ -1513,7 +1518,7 @@
 void ASTStmtWriter::VisitCXXOperatorCallExpr(CXXOperatorCallExpr *E) {
   VisitCallExpr(E);
   Record.push_back(E->getOperator());
-  Record.push_back(E->getFPFeatures().getInt());
+  Record.push_back(E->getFPFeatures().getAsOpaqueInt());
   Record.AddSourceRange(E->Range);
   Code = serialization::EXPR_CXX_OPERATOR_CALL;
 }
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -3905,7 +3905,7 @@
 
 /// Write an FP_PRAGMA_OPTIONS block for the given FPOptions.
 void ASTWriter::WriteFPPragmaOptions(const FPOptions &Opts) {
-  RecordData::value_type Record[] = {Opts.getInt()};
+  RecordData::value_type Record[] = {Opts.getAsOpaqueInt()};
   Stream.EmitRecord(FP_PRAGMA_OPTIONS, Record);
 }
 
Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -1050,12 +1050,16 @@
 }
 
 void ASTStmtReader::VisitBinaryOperator(BinaryOperator *E) {
+  bool hasFP_Features;
+  BinaryOperator::Opcode opc;
   VisitExpr(E);
+  E->setHasStoredFPFeatures(hasFP_Features = Record.readInt());
+  E->setOpcode(opc = (BinaryOperator::Opcode)Record.readInt());
   E->setLHS(Record.readSubExpr());
   E->setRHS(Record.readSubExpr());
-  E->setOpcode((BinaryOperator::Opcode)Record.readInt());
   E->setOperatorLoc(readSourceLocation());
-  E->setFPFeatures(FPOptions(Record.readInt()));
+  if (hasFP_Features)
+E->setStoredFPFeatures(FPOptions(Record.readInt()));
 }
 
 void ASTStmtReader::VisitCompoundAssignOperator(CompoundAssignOperator *E) {
@@ -2937,11 +2941,13 @@
   break;
 
 case EXPR_BINARY_OPERATOR:
-  S = new (Context) BinaryOperator(Empty);
+  S = BinaryOperator::CreateEmpty(Context,
+  Record[ASTStmtReader::NumExprFields]);
   break;
 
 case EXPR_COMPOUND_ASSIGN_OPERATOR:
-  S = new (Context) CompoundAssignOperator(Empty);
+  S = CompoundAssignOperator::CreateEmpty(
+  Context, Record[ASTStmtReader::NumExprFields]);
   break;
 
 case EXPR_CONDITIONAL_OPERATOR:
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -10267,8 +10267,12 @@
   RHS.get() == E->getRHS())
 return E;
 
+  if (E->isCompoundAssignmentOp())
+// FPFeatures has already been established from trailing storage
+return getDerived().RebuildBinaryOperator(
+E->getOperatorLoc(), E->getOpcode(), LHS.get(), RHS.get());
   Sema::FPFeaturesStateRAII FPFeaturesState(getSema());
-  getSema().FPFeatures = E->getFPFeatures();
+  getSema().FPFeatures = E->getFPFeatures(

[PATCH] D77233: [NFC] Refactoring PropertyAttributeKind for ObjCPropertyDecl and ObjCDeclSpec.

[Puyan Lotfi via Phabricator via cfe-commits]

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



Comment at: clang/include/clang-c/Index.h:4503
   CXObjCPropertyAttr_unsafe_unretained = 0x800,
-  CXObjCPropertyAttr_class = 0x1000
+  CXObjCPropertyAttr_classattr = 0x1000
 } CXObjCPropertyAttrKind;

erik.pilkington wrote:
> plotfi wrote:
> > @erik.pilkington  Do you think we should be adding the C-API analogs of 
> > nullability / null_resettable (and direct) to 
> > clang/include/clang-c/Index.h? 
> > 
> > I noticed those are missing.
> > 
> > 
> Oh, sorry, I missed this. The C API is supposed to be stable, so I don't 
> think we should change the name of this enumerator (@arphaman can you 
> confirm?). Adding the missing attributes seem fine though. 
@erik.pilkington  @arphaman Any chance I can land the C API changes in a follow 
up commit? That's what I was thinking. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77233



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

[PATCH] D77732: [clangd] Shard preamble symbols in dynamic index

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 257003.
kadircet marked 4 inline comments as done.
kadircet added a comment.

- Unify sharding logic in BackgroundIndex and FileIndex.
- Make sure relations have valid subjects.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77732

Files:
  clang-tools-extra/clangd/index/Background.cpp
  clang-tools-extra/clangd/index/FileIndex.cpp
  clang-tools-extra/clangd/index/FileIndex.h
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp

Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -116,9 +116,10 @@
 std::unique_ptr TestTU::index() const {
   auto AST = build();
   auto Idx = std::make_unique(/*UseDex=*/true);
-  Idx->updatePreamble(Filename, /*Version=*/"null", AST.getASTContext(),
-  AST.getPreprocessorPtr(), AST.getCanonicalIncludes());
-  Idx->updateMain(Filename, AST);
+  Idx->updatePreamble(testPath(Filename), /*Version=*/"null",
+  AST.getASTContext(), AST.getPreprocessorPtr(),
+  AST.getCanonicalIncludes());
+  Idx->updateMain(testPath(Filename), AST);
   return std::move(Idx);
 }
 
Index: clang-tools-extra/clangd/unittests/FileIndexTests.cpp
===
--- clang-tools-extra/clangd/unittests/FileIndexTests.cpp
+++ clang-tools-extra/clangd/unittests/FileIndexTests.cpp
@@ -151,8 +151,9 @@
   File.HeaderFilename = (Basename + ".h").str();
   File.HeaderCode = std::string(Code);
   auto AST = File.build();
-  M.updatePreamble(File.Filename, /*Version=*/"null", AST.getASTContext(),
-   AST.getPreprocessorPtr(), AST.getCanonicalIncludes());
+  M.updatePreamble(testPath(File.Filename), /*Version=*/"null",
+   AST.getASTContext(), AST.getPreprocessorPtr(),
+   AST.getCanonicalIncludes());
 }
 
 TEST(FileIndexTest, CustomizedURIScheme) {
@@ -393,8 +394,9 @@
   TU.HeaderCode = "class A {}; class B : public A {};";
   auto AST = TU.build();
   FileIndex Index;
-  Index.updatePreamble(TU.Filename, /*Version=*/"null", AST.getASTContext(),
-   AST.getPreprocessorPtr(), AST.getCanonicalIncludes());
+  Index.updatePreamble(testPath(TU.Filename), /*Version=*/"null",
+   AST.getASTContext(), AST.getPreprocessorPtr(),
+   AST.getCanonicalIncludes());
   SymbolID A = findSymbol(TU.headerSymbols(), "A").ID;
   uint32_t Results = 0;
   RelationsRequest Req;
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -283,6 +283,18 @@
   if (!ID)
 return true;
 
+  // ND is the canonical (i.e. first) declaration. If it's in the main file
+  // (which is not a header), then no public declaration was visible, so assume
+  // it's main-file only.
+  bool IsMainFileOnly =
+  SM.isWrittenInMainFile(SM.getExpansionLoc(ND->getBeginLoc())) &&
+  !isHeaderFile(SM.getFileEntryForID(SM.getMainFileID())->getName(),
+ASTCtx->getLangOpts());
+  // In C, printf is a redecl of an implicit builtin! So check OrigD instead.
+  if (ASTNode.OrigD->isImplicit() ||
+  !shouldCollectSymbol(*ND, *ASTCtx, Opts, IsMainFileOnly))
+return true;
+
   // Note: we need to process relations for all decl occurrences, including
   // refs, because the indexing code only populates relations for specific
   // occurrences. For example, RelationBaseOf is only populated for the
@@ -297,17 +309,6 @@
   if (IsOnlyRef && !CollectRef)
 return true;
 
-  // ND is the canonical (i.e. first) declaration. If it's in the main file
-  // (which is not a header), then no public declaration was visible, so assume
-  // it's main-file only.
-  bool IsMainFileOnly =
-  SM.isWrittenInMainFile(SM.getExpansionLoc(ND->getBeginLoc())) &&
-  !isHeaderFile(SM.getFileEntryForID(SM.getMainFileID())->getName(),
-ASTCtx->getLangOpts());
-  // In C, printf is a redecl of an implicit builtin! So check OrigD instead.
-  if (ASTNode.OrigD->isImplicit() ||
-  !shouldCollectSymbol(*ND, *ASTCtx, Opts, IsMainFileOnly))
-return true;
   // Do not store references to main-file symbols.
   // Unlike other fields, e.g. Symbols (which use spelling locations), we use
   // file locations for references (as it aligns the behavior of clangd's
Index: clang-tools-extra/clangd/index/FileIndex.h
===
--- clang-tools-extra/clangd/index/FileIndex.h
+++ clang-tools-extra/clangd/

[PATCH] D78030: [TimeProfiler] Emit clock synchronization point

[Sergej Jaskiewicz via Phabricator via cfe-commits]

broadwaylamb created this revision.
broadwaylamb added reviewers: anton-afanasyev, russell.gallop, espindola.
Herald added subscribers: cfe-commits, MaskRay, hiraditya, emaste.
Herald added a project: clang.
broadwaylamb added a parent revision: D78027: [TimeProfiler] Emit real process 
ID and thread names.

`TimeProfiler` emits relative timestamps for events (the number of microseconds 
passed since the start of the current process).

This patch allows combining events from different processes while preserving 
their relative timing by emitting a new attribute `beginningOfTime`. This 
attribute contains the system time that corresponds to the zero timestamp of 
the time profiler.

This has at least two use cases:

- Build systems can use this to merge time traces from multiple compiler 
invocations and generate statistics for the whole build. Tools like 
ClangBuildAnalyzer  could also 
leverage this feature.
- Compilers that use LLVM as their backend by invoking `llc`/`opt` in a child 
process. I'm currently working on supporting the `-ftime-trace` functionality 
in GHC. A single GHC invocation can emit GHC-specific events, but with this 
patch it could also include LLVM-specific events in its log.

I wrote a proof-of-concept script that merges multiple logs that contain a 
synchronization point into one log: 
https://github.com/broadwaylamb/merge_trace_events

This is how the result looks like for GHC:

F11724922: 79080338-ad673580-7d1c-11ea-9e30-5e6f72e77555.png 



Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78030

Files:
  clang/test/Driver/check-time-trace-sections.py
  clang/test/Driver/check-time-trace.cpp
  lld/test/ELF/time-trace.s
  llvm/lib/Support/TimeProfiler.cpp


Index: llvm/lib/Support/TimeProfiler.cpp
===
--- llvm/lib/Support/TimeProfiler.cpp
+++ llvm/lib/Support/TimeProfiler.cpp
@@ -251,6 +251,21 @@
 
 J.arrayEnd();
 J.attributeEnd();
+
+// Emit synchronization point, i. e. the absolute time of StartTime.
+// When combining time profiler logs from different processes,
+// this attribute helps preserve relative timing.
+{
+  const auto SystemTime =
+  time_point_cast(system_clock::now());
+  const microseconds ProcessLocalTime =
+  time_point_cast(steady_clock::now()) -
+  time_point_cast(StartTime);
+  const auto BeginningOfTimeUs = SystemTime - ProcessLocalTime;
+  J.attribute("beginningOfTime",
+  BeginningOfTimeUs.time_since_epoch().count());
+}
+
 J.objectEnd();
   }
 
Index: lld/test/ELF/time-trace.s
===
--- lld/test/ELF/time-trace.s
+++ lld/test/ELF/time-trace.s
@@ -18,7 +18,8 @@
 # RUN:   | %python -c 'import json, sys; 
json.dump(json.loads(sys.stdin.read()), sys.stdout, sort_keys=True, indent=2)' \
 # RUN:   | FileCheck %s
 
-# CHECK: "traceEvents": [
+# CHECK: "beginningOfTime": {{[0-9]{16},}}
+# CHECK-NEXT: "traceEvents": [
 
 # Check one event has correct fields
 # CHECK:  "dur":
Index: clang/test/Driver/check-time-trace.cpp
===
--- clang/test/Driver/check-time-trace.cpp
+++ clang/test/Driver/check-time-trace.cpp
@@ -3,7 +3,8 @@
 // RUN:   | %python -c 'import json, sys; 
json.dump(json.loads(sys.stdin.read()), sys.stdout, sort_keys=True, indent=2)' \
 // RUN:   | FileCheck %s
 
-// CHECK: "traceEvents": [
+// CHECK: "beginningOfTime": {{[0-9]{16},}}
+// CHECK-NEXT: "traceEvents": [
 // CHECK: "args":
 // CHECK: "detail":
 // CHECK: "dur":
Index: clang/test/Driver/check-time-trace-sections.py
===
--- clang/test/Driver/check-time-trace-sections.py
+++ clang/test/Driver/check-time-trace-sections.py
@@ -1,6 +1,6 @@
 #!/usr/bin/env python
 
-import json, sys
+import json, sys, time
 
 def is_inside(range1, range2):
 a = range1["ts"]; b = a + range1["dur"]
@@ -11,11 +11,20 @@
 b = range1["ts"] + range1["dur"]; c = range2["ts"]
 return b <= c
 
-events = json.loads(sys.stdin.read())["traceEvents"]
+log_contents = json.loads(sys.stdin.read())
+events = log_contents["traceEvents"]
 codegens = [event for event in events if event["name"] == "CodeGen Function"]
 frontends = [event for event in events if event["name"] == "Frontend"]
 backends = [event for event in events if event["name"] == "Backend"]
 
+beginning_of_time = log_contents["beginningOfTime"] / 100
+seconds_since_epoch = time.time()
+
+# Make sure that the 'beginningOfTime' is not earlier than 10 seconds ago.
+if seconds_since_epoch - beginning_of_time > 10:
+sys.exit("'beginningOfTime' should represent the absolute time when the "
+ "process has started")
+
 if not all([any([is_inside(codegen, frontend) for frontend in frontends])
   

[PATCH] D78020: clang/AMDGPU: Assume denormals are enabled for the default target.

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

I think https://reviews.llvm.org/D78019 should fix the issue about HIP not 
using correct default denormal value if no arch is specified.

In that case, driver actually sets offloading arch to gfx803 and clang should 
be able to see it in JobAction.


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

https://reviews.llvm.org/D78020



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

[PATCH] D77732: [clangd] Shard preamble symbols in dynamic index

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/index/FileIndex.cpp:97
+/// paths used by \p FileSyms.
+void shardSlabToFiles(const SymbolSlab &Syms, const RelationSlab &Rels,
+  FileSymbols &FileSyms, llvm::StringRef HintPath) {

kadircet wrote:
> sammccall wrote:
> > This screams out to be shared with BackgroundIndex::update(). Is it very 
> > hard to do?
> > 
> > Things that are different:
> >  - no duplication of symbol into defining file (why?)
> >  - refs not gathered here (but I guess empty input -> empty output?)
> >  - include graph not gathered here (same)
> >  - this holds 3 copies of all the data at peak, other impl holds 2
> > 
> > I imagine factoring out a function that returns a StringMap like in 
> > BackgroundIndex::Update, but File has a build() function that returns an 
> > IndexFileIn or so.
> > (Ooh, is depending on IndexFileIn a layering violation? Maybe we should 
> > move that into another header. Duplicating the struct is probably OK for 
> > now)
> In backgroundindex we "prefilter" symbols and only duplicate the ones coming 
> from modified files, whereas in this one we duplicate every symbol no matter 
> what. I didn't want to change backgroundindex into post filtering mode (I 
> suppose that's what you mean by the last item).
> 
> as for the first bullet point, duplicating in case of preamble symbols 
> wouldn't matter since we don't merge(rather pick one) while building the 
> index for preamblesymbols. I suppose storing them twice shouldn't be a huge 
> issue.
> 
> Depending to `IndexFileIn` in "Index.h" would be a violation, but depending 
> on it in "FileIndex.h" is fine.
> 
> I am happy to refactor a `StringMap shardIndexToFiles(IndexFileIn, 
> HintPath)` into "FileIndex.h" that can be used by both backgroundindex and 
> this, if you are OK with above mentioned regressions.
implemented a version of this that only copies data when requested. Hence the 
pre/post filtering argument is gone.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77732



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

[PATCH] D78019: HIP: Fix handling of denormal mode

[Matt Arsenault via Phabricator via cfe-commits]

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



Comment at: clang/test/Driver/cuda-flush-denormals-to-zero.cu:27
+// Test multiple offload archs with different defaults.
+// RUN: %clang -x hip -no-canonical-prefixes -### -target x86_64-linux-gnu -c 
-march=haswell --cuda-gpu-arch=gfx803 --cuda-gpu-arch=gfx900 -nocudainc 
-nogpulib %s 2>&1 | FileCheck -check-prefix=MIXED-DEFAULT-MODE %s
+// RUN: %clang -x hip -no-canonical-prefixes -### -target x86_64-linux-gnu -c 
-march=haswell -fcuda-flush-denormals-to-zero --cuda-gpu-arch=gfx803 
--cuda-gpu-arch=gfx900 -nocudainc -nogpulib %s 2>&1 | FileCheck 
-check-prefix=FTZX2 %s

yaxunl wrote:
> this will result in multiple clang -cc1 commands, each one corresponding to 
> an arch. You need to check each arch.
Since the flag is not printed for the default case, having a second arch check 
line would interfere with the -NOT check, as there is no CHECK-SAME-NOT


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

https://reviews.llvm.org/D78019



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

[PATCH] D78033: [cmake] Restrict symbols exported from libclang-cpp

[Pirama Arumuga Nainar via Phabricator via cfe-commits]

pirama created this revision.
pirama added reviewers: beanz, mgorny.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

In libclang-cpp, export only symbols from the clang namespace or clang_*
(functions for the C interface).  This fixes the use case where a tool
depends on both libclang-cpp and libLLVM.  Without this change,
command-line registries from libLLVMSupport are exported by libclang-cpp
and gets deduped with symbols of the same name from libLLVM .  But, the
arguments get registered separately from both the libraries during
startup, resulting in an error.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78033

Files:
  clang/tools/clang-shlib/CMakeLists.txt
  clang/tools/clang-shlib/libclang-cpp.exports


Index: clang/tools/clang-shlib/libclang-cpp.exports
===
--- /dev/null
+++ clang/tools/clang-shlib/libclang-cpp.exports
@@ -0,0 +1,2 @@
+_Z*5clang*
+clang_*
Index: clang/tools/clang-shlib/CMakeLists.txt
===
--- clang/tools/clang-shlib/CMakeLists.txt
+++ clang/tools/clang-shlib/CMakeLists.txt
@@ -36,6 +36,8 @@
   set(INSTALL_WITH_TOOLCHAIN INSTALL_WITH_TOOLCHAIN)
 endif()
 
+set(LLVM_EXPORTED_SYMBOL_FILE ${CMAKE_CURRENT_SOURCE_DIR}/libclang-cpp.exports)
+
 add_clang_library(clang-cpp
   SHARED
   ${INSTALL_WITH_TOOLCHAIN}


Index: clang/tools/clang-shlib/libclang-cpp.exports
===
--- /dev/null
+++ clang/tools/clang-shlib/libclang-cpp.exports
@@ -0,0 +1,2 @@
+_Z*5clang*
+clang_*
Index: clang/tools/clang-shlib/CMakeLists.txt
===
--- clang/tools/clang-shlib/CMakeLists.txt
+++ clang/tools/clang-shlib/CMakeLists.txt
@@ -36,6 +36,8 @@
   set(INSTALL_WITH_TOOLCHAIN INSTALL_WITH_TOOLCHAIN)
 endif()
 
+set(LLVM_EXPORTED_SYMBOL_FILE ${CMAKE_CURRENT_SOURCE_DIR}/libclang-cpp.exports)
+
 add_clang_library(clang-cpp
   SHARED
   ${INSTALL_WITH_TOOLCHAIN}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78020: clang/AMDGPU: Assume denormals are enabled for the default target.

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

In D78020#1978034 , @yaxunl wrote:

> I think https://reviews.llvm.org/D78019 should fix the issue about HIP not 
> using correct default denormal value if no arch is specified.
>
> In that case, driver actually sets offloading arch to gfx803 and clang should 
> be able to see it in JobAction.


It doesn't, that patch is the parent of this one. This use an empty offloading 
arch and I wasn't sure where the codegen default was forced


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

https://reviews.llvm.org/D78020



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

[PATCH] D78019: HIP: Fix handling of denormal mode

[Matt Arsenault via Phabricator via cfe-commits]

arsenm updated this revision to Diff 257011.
arsenm added a comment.

Remove leftover comment from before I used JobAction


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

https://reviews.llvm.org/D78019

Files:
  clang/include/clang/Driver/ToolChain.h
  clang/lib/Driver/ToolChains/AMDGPU.cpp
  clang/lib/Driver/ToolChains/AMDGPU.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/Cuda.cpp
  clang/lib/Driver/ToolChains/Cuda.h
  clang/lib/Driver/ToolChains/Linux.cpp
  clang/lib/Driver/ToolChains/Linux.h
  clang/lib/Driver/ToolChains/PS4CPU.h
  clang/test/Driver/cuda-flush-denormals-to-zero.cu

Index: clang/test/Driver/cuda-flush-denormals-to-zero.cu
===
--- clang/test/Driver/cuda-flush-denormals-to-zero.cu
+++ clang/test/Driver/cuda-flush-denormals-to-zero.cu
@@ -7,16 +7,28 @@
 // RUN: %clang -no-canonical-prefixes -### -target x86_64-linux-gnu -c -march=haswell --cuda-gpu-arch=sm_70 -fcuda-flush-denormals-to-zero -nocudainc -nocudalib %s 2>&1 | FileCheck -check-prefix=FTZ %s
 // RUN: %clang -no-canonical-prefixes -### -target x86_64-linux-gnu -c -march=haswell --cuda-gpu-arch=sm_70 -fno-cuda-flush-denormals-to-zero -nocudainc -nocudalib %s 2>&1 | FileCheck -check-prefix=NOFTZ %s
 
-// Test explicit argument.
+// Test explicit argument, with CUDA offload kind
 // RUN: %clang -no-canonical-prefixes -### -target x86_64-linux-gnu -c -march=haswell --cuda-gpu-arch=gfx803 -fcuda-flush-denormals-to-zero -nocudainc -nogpulib %s 2>&1 | FileCheck -check-prefix=FTZ %s
 // RUN: %clang -no-canonical-prefixes -### -target x86_64-linux-gnu -c -march=haswell --cuda-gpu-arch=gfx803 -fno-cuda-flush-denormals-to-zero -nocudainc -nogpulib %s 2>&1 | FileCheck -check-prefix=NOFTZ %s
+
+// Test explicit argument, with HIP offload kind
+// RUN: %clang -x hip -no-canonical-prefixes -### -target x86_64-linux-gnu -c -march=haswell --cuda-gpu-arch=gfx803 -fcuda-flush-denormals-to-zero -nocudainc -nogpulib %s 2>&1 | FileCheck -check-prefix=FTZ %s
+// RUN: %clang -x hip -no-canonical-prefixes -### -target x86_64-linux-gnu -c -march=haswell --cuda-gpu-arch=gfx803 -fno-cuda-flush-denormals-to-zero -nocudainc -nogpulib %s 2>&1 | FileCheck -check-prefix=NOFTZ %s
+
 // RUN: %clang -x hip -no-canonical-prefixes -### -target x86_64-linux-gnu -c -march=haswell --cuda-gpu-arch=gfx900 -fcuda-flush-denormals-to-zero -nocudainc -nogpulib %s 2>&1 | FileCheck -check-prefix=FTZ %s
 // RUN: %clang -x hip -no-canonical-prefixes -### -target x86_64-linux-gnu -c -march=haswell --cuda-gpu-arch=gfx900 -fno-cuda-flush-denormals-to-zero -nocudainc -nogpulib %s 2>&1 | FileCheck -check-prefix=NOFTZ %s
 
-// Test the default changing with no argument based on the subtarget.
+// Test the default changing with no argument based on the subtarget in HIP mode
 // RUN: %clang -x hip -no-canonical-prefixes -### -target x86_64-linux-gnu -c -march=haswell --cuda-gpu-arch=gfx803 -nocudainc -nogpulib %s 2>&1 | FileCheck -check-prefix=FTZ %s
 // RUN: %clang -x hip -no-canonical-prefixes -### -target x86_64-linux-gnu -c -march=haswell --cuda-gpu-arch=gfx900 -nocudainc -nogpulib %s 2>&1 | FileCheck -check-prefix=NOFTZ %s
 
+
+// Test multiple offload archs with different defaults.
+// RUN: %clang -x hip -no-canonical-prefixes -### -target x86_64-linux-gnu -c -march=haswell --cuda-gpu-arch=gfx803 --cuda-gpu-arch=gfx900 -nocudainc -nogpulib %s 2>&1 | FileCheck -check-prefix=MIXED-DEFAULT-MODE %s
+// RUN: %clang -x hip -no-canonical-prefixes -### -target x86_64-linux-gnu -c -march=haswell -fcuda-flush-denormals-to-zero --cuda-gpu-arch=gfx803 --cuda-gpu-arch=gfx900 -nocudainc -nogpulib %s 2>&1 | FileCheck -check-prefix=FTZX2 %s
+// RUN: %clang -x hip -no-canonical-prefixes -### -target x86_64-linux-gnu -c -march=haswell -fno-cuda-flush-denormals-to-zero --cuda-gpu-arch=gfx803 --cuda-gpu-arch=gfx900 -nocudainc -nogpulib %s 2>&1 | FileCheck -check-prefix=NOFTZ %s
+
+
 // CPUFTZ-NOT: -fdenormal-fp-math
 
 // FTZ-NOT: -fdenormal-fp-math-f32=
@@ -25,3 +37,13 @@
 // The default of ieee is omitted
 // NOFTZ-NOT: "-fdenormal-fp-math"
 // NOFTZ-NOT: "-fdenormal-fp-math-f32"
+
+// MIXED-DEFAULT-MODE-NOT: -denormal-fp-math
+// MIXED-DEFAULT-MODE: "-fdenormal-fp-math-f32=preserve-sign,preserve-sign"
+// MIXED-DEFAULT-MODE-SAME: "-target-cpu" "gfx803"
+// MIXED-DEFAULT-MODE-NOT: -denormal-fp-math
+
+// FTZX2: "-fdenormal-fp-math-f32=preserve-sign,preserve-sign"
+// FTZX2-SAME: "-target-cpu" "gfx803"
+// FTZX2: "-fdenormal-fp-math-f32=preserve-sign,preserve-sign"
+// FTZX2-SAME: "-target-cpu" "gfx900"
Index: clang/lib/Driver/ToolChains/PS4CPU.h
===
--- clang/lib/Driver/ToolChains/PS4CPU.h
+++ clang/lib/Driver/ToolChains/PS4CPU.h
@@ -94,9 +94,8 @@
 Action::OffloadKind DeviceOffloadingKind) const override;
 
   llvm::DenormalMode getDefaultDenormalModeForType(
-const llvm::opt::ArgList &DriverArgs,
-Action::OffloadKind 

[PATCH] D77683: [Docs] Make code review policy clearer about requested pre-commit reviews

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In D77683#1973762 , @dblaikie wrote:

> In D77683#1973757 , @jdoerfert wrote:
>
> > In D77683#1970826 , @mehdi_amini 
> > wrote:
> >
> > > I am still not sure what "if someone has asked for extra review of a 
> > > specific area" refers to?
> >
> >
> > As said earlier
> >
> > >>   If I understand this correctly, this is meant to cover situations 
> > >> where reviewers are active in an area and indicated an interest in 
> > >> reviewing basically everything.
> > > 
> > > Pretty much, yes.
> >
> > this should mean that, if requested, all non-trivial patches should go 
> > through review. The current wording is very lenient, especially wrt. code 
> > owners.
> >  While most people I talked to don't see owners as special per se (but just 
> > assume they have more responsibility), this paragraph says they have 
> > special rights.
> >  Given the murky ways owners are "selected", I think we should have a well 
> > defined way to limit these rights without revoking the status.
>
>
> I disagree here. I think it's suitable (within the way the LLVM project 
> works) for code owners to commit without review - given they are the arbiters 
> of what's acceptable in that subcomponent - ultimately they can veto anyone 
> else (short of a broader project/code owner). Doesn't usually come to that, 
> but that's what the ownership role means.
>
> I don't think it's correct for arbitrary contributors to say "you need 
> to/this component needs review-before-commit" - the code owner could say that 
> if they really don't trust any of the contributors to conform to the 
> direction they have in mind for the component (that's not to discredit the 
> contributors - but that's the point of pre-commit review: to ensure it 
> conforms to the code owners vision for the component). 
> privledges
>  (yes, code owners aren't meant to be dictators - but they're ultimately the 
> final decider)


I disagree that this characterizes our conception of the role of code owner 
within the project, but there is some mitigating context. In my experience, we 
describe code owners as "reviewers of last resort", and their primary 
responsibility is to prevent review stagnation with their component (including 
from new/infrequent contributors whose patches might otherwise go unreviewed). 
The code owner is not the final decider in all cases, but can serve as a tie 
breaker when community consensus is unclear. That's an important role, because 
it ensures our ability to make forward progress, but is different in character 
from someone with overriding final authority. The code owners vision matters 
only slightly if the community consensus is for a different vision. There 
certainly are parts of LLVM that are developed by one person, and that person 
is the code owner, and thus excessive a lot of influence over the future 
direction of the component. Patches are often contributed without pre-commit 
review, in this case, and the code owner is often the most-skilled reviewer for 
any other patches as well. However, this state exists only at the pleasure of 
the rest of the community - we all see these patches on the commits list, and 
should more people become involved and request a more-inclusive pre-commit 
review cycle, that's what should happen. For components actively developed by 
many people, code owners have relatively minor privileges in this regard.

I suppose that we never wrote this down anywhere, but when I became code owner 
for the AA subsystem, we had an understanding that, because AA is so pervasive 
and subtle, even as code owner I would never commit anything (aside from 
reverts or similar) without pre-commit review. IMHO, this is the only 
responsible way to handle this subsystem. Not all subsystems are so risky, so 
there's appropriately a spectrum of development practices (some favoring 
velocity over stability), but I still view this as being more democratic than 
hierarchically authoritative.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77683



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

[PATCH] D77732: [clangd] Shard preamble symbols in dynamic index

[Kadir Cetinkaya via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:286
 
+  // ND is the canonical (i.e. first) declaration. If it's in the main file
+  // (which is not a header), then no public declaration was visible, so assume

put this before `processRelations` to ensure `Subject` of a relation is a 
symbol we are going to collect.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77732



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

[PATCH] D77908: [WebAssembly] Enable nontrapping-fptoint for `default` cpu

[Alon Zakai via Phabricator via cfe-commits]

kripken added a comment.

In D77908#1977039 , @sbc100 wrote:

> As a less controversial version of this change I could instead create a new 
> CPU called `current` and leave `generic` as is (basically leave it at mvp) 
> until we can agree that a features is widespread enough to warrant being part 
> of generic?


That makes a lot of sense to me. `current` or `current-spec` or such seems 
pretty clear. Then `generic` stays as it always was (at mvp) and that seems 
safer for the ecosystem.

Btw, how does LLVM handle this issue with other backends? When say Intel 
releases a new CPU with a new feature, are those automatically applied in the 
`generic` CPU for Intel? (and hence people that want older CPUs will get 
breakage unless they change the CPU  target) If those are automatically 
applied, at what frequency/policy?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77908



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

[PATCH] D77859: Revert "[DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff."

[Mehdi AMINI via Phabricator via cfe-commits]

mehdi_amini added a comment.

Was reverted, can we drop this one?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77859



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

[PATCH] D77982: [Windows SEH] Fix the frame-ptr of a nested-filter within a _finally

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

Again, using the name isn't reliable.  Among other things, in release builds, 
IR values don't have names at all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77982



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

[PATCH] D78027: [TimeProfiler] Emit real process ID and thread names

[Sergej Jaskiewicz via Phabricator via cfe-commits]

broadwaylamb updated this revision to Diff 257015.
broadwaylamb added a comment.

Explicitly specify argument types in `writeMetadataEvent` lambda.


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

https://reviews.llvm.org/D78027

Files:
  clang/test/Driver/check-time-trace.cpp
  lld/test/ELF/time-trace.s
  llvm/lib/Support/TimeProfiler.cpp

Index: llvm/lib/Support/TimeProfiler.cpp
===
--- llvm/lib/Support/TimeProfiler.cpp
+++ llvm/lib/Support/TimeProfiler.cpp
@@ -15,6 +15,7 @@
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/Process.h"
 #include "llvm/Support/Threading.h"
 #include 
 #include 
@@ -73,10 +74,18 @@
   }
 };
 
+static std::string getThreadName() {
+  SmallString<64> Name;
+  llvm::get_thread_name(Name);
+  return std::string(std::move(Name));
+}
+
 struct TimeTraceProfiler {
   TimeTraceProfiler(unsigned TimeTraceGranularity = 0, StringRef ProcName = "")
   : StartTime(steady_clock::now()), ProcName(ProcName),
-Tid(llvm::get_threadid()), TimeTraceGranularity(TimeTraceGranularity) {}
+Pid(sys::Process::getProcessId()), ThreadName(getThreadName()),
+Tid(llvm::get_threadid()),
+TimeTraceGranularity(TimeTraceGranularity) {}
 
   void begin(std::string Name, llvm::function_ref Detail) {
 Stack.emplace_back(steady_clock::now(), TimePointType(), std::move(Name),
@@ -141,7 +150,7 @@
   auto DurUs = E.getFlameGraphDurUs();
 
   J.object([&]{
-J.attribute("pid", 1);
+J.attribute("pid", Pid);
 J.attribute("tid", int64_t(Tid));
 J.attribute("ph", "X");
 J.attribute("ts", StartUs);
@@ -205,7 +214,7 @@
   auto Count = AllCountAndTotalPerName[Total.first].first;
 
   J.object([&]{
-J.attribute("pid", 1);
+J.attribute("pid", Pid);
 J.attribute("tid", int64_t(TotalTid));
 J.attribute("ph", "X");
 J.attribute("ts", 0);
@@ -220,16 +229,26 @@
   ++TotalTid;
 }
 
-// Emit metadata event with process name.
-J.object([&] {
-  J.attribute("cat", "");
-  J.attribute("pid", 1);
-  J.attribute("tid", 0);
-  J.attribute("ts", 0);
-  J.attribute("ph", "M");
-  J.attribute("name", "process_name");
-  J.attributeObject("args", [&] { J.attribute("name", ProcName); });
-});
+auto writeMetadataEvent = [&](const char *Name, uint64_t Tid,
+  StringRef arg) {
+  J.object([&] {
+J.attribute("cat", "");
+J.attribute("pid", Pid);
+J.attribute("tid", int64_t(Tid));
+J.attribute("ts", 0);
+J.attribute("ph", "M");
+J.attribute("name", Name);
+J.attributeObject("args", [&] { J.attribute("name", arg); });
+  });
+};
+
+writeMetadataEvent("process_name", Tid, ProcName);
+
+writeMetadataEvent("thread_name", Tid, ThreadName);
+
+for (const auto &TTP : ThreadTimeTraceProfilerInstances) {
+  writeMetadataEvent("thread_name", TTP->Tid, TTP->ThreadName);
+}
 
 J.arrayEnd();
 J.attributeEnd();
@@ -241,6 +260,8 @@
   StringMap CountAndTotalPerName;
   const TimePointType StartTime;
   const std::string ProcName;
+  const sys::Process::Pid Pid;
+  const std::string ThreadName;
   const uint64_t Tid;
 
   // Minimum time granularity (in microseconds)
Index: lld/test/ELF/time-trace.s
===
--- lld/test/ELF/time-trace.s
+++ lld/test/ELF/time-trace.s
@@ -34,6 +34,7 @@
 # Check process_name entry field
 # CHECK: "name": "ld.lld{{(.exe)?}}"
 # CHECK: "name": "process_name"
+# CHECK: "name": "thread_name"
 
 .globl _start
 _start:
Index: clang/test/Driver/check-time-trace.cpp
===
--- clang/test/Driver/check-time-trace.cpp
+++ clang/test/Driver/check-time-trace.cpp
@@ -14,6 +14,7 @@
 // CHECK-NEXT: "ts":
 // CHECK: "name": "clang{{.*}}"
 // CHECK: "name": "process_name"
+// CHECK: "name": "thread_name"
 
 template 
 struct Struct {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

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

In D73307#1972388 , @rnk wrote:

> Regarding the alias attribute, it occurs to me that reimplementing this as an 
> early LLVM pass would not have that problem. Do you think that would be worth 
> doing?


I can try doing this.  If my understanding is right,  you are suggesting that 
doing this later would mean the alias would have already been applied?.  
However, for multi-versioning symbols, I must parse the target name to insert 
the module name in between right?




Comment at: clang/docs/UsersManual.rst:1684
+   linkage symbols. The unique name is obtained by appending the hash of the
+   full module name to the original symbol. This option is particularly useful
+   in attributing profile information to the correct function when multiple

rnk wrote:
> tmsriram wrote:
> > hubert.reinterpretcast wrote:
> > > MaskRay wrote:
> > > > rnk wrote:
> > > > > I think it's important to be specific here. I'd go so far as to say:
> > > > > "When this option is set, compiler hashes the main source file path 
> > > > > from the command line and appends it to all internal symbols. If a 
> > > > > program contains multiple objects compiled from the same source file 
> > > > > path, the symbols are not guaranteed to be unique."
> > > > Should option like -ffile-prefix-map= affect the hashed source file 
> > > > path? I suspect it already does this but I haven't verified.
> > > > 
> > > > This may need tests for Windows and non-Windows, similar to D77223.
> > > With respect to @rnk's suggestion, "from the same source file path" 
> > > implies the same file. I would suggest saying "with the same command-line 
> > > source file specification".
> > I have handled this now and added a test.  I saw the reference but don't 
> > quite understand the need for windows test?  I am not familiar with this.
> Would the file prefix map not already have been applied when creating the 
> LLVM IR module source file path? I think it should be, so I'm not sure the 
> new code you added is in the right place.
Module.getSourceFileName() never changes.   I checked here and also at 
AsmPrinter.cpp.  I am not sure this a bug, maybe I should just fix that first.



Comment at: clang/lib/CodeGen/CodeGenModule.h:31
 #include "clang/Basic/XRayLists.h"
+#include "clang/Lex/PreprocessorOptions.h"
 #include "llvm/ADT/DenseMap.h"

rnk wrote:
> Why add this include here? I don't see any reason it is needed. Maybe the cpp 
> file needs it?
Yes, the cpp file needs this to access the MacroPrefixMap.


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

https://reviews.llvm.org/D73307



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

[PATCH] D77984: Make IRBuilder automatically set alignment on load/store/alloca.

[Eli Friedman via Phabricator via cfe-commits]

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



Comment at: llvm/include/llvm/IR/IRBuilder.h:1600
+return CreateAlignedLoad(Ty, Ptr, DL.getABITypeAlign(Ty), isVolatile, 
Name);
   }
 

jdoerfert wrote:
> Can't we just pawn of the alignment stuff to `CreateAlignedLoad`? We cal lit 
> with 0 here and we determine an alignment there anyway?
> Nit: `BB->getModule()` should work.
> 
I was thinking we might want to switch CreateAlignedLoad to take an Align 
instead of a MaybeAlign: I'm not sure there's any good reason to use a 
MaybeAlign once we clean up the getters on load/store/alloca.  But I guess we 
can do that later, if it turns out to actually be feasible; I'll change it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77984



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

[PATCH] D72624: [WIP] TargetMachine Hook for Module Metadata

[Kuan Hsu Chen (Zakk) via Phabricator via cfe-commits]

khchen added a comment.
Herald added a reviewer: herhut.
Herald added subscribers: frgossen, grosul1, Joonsoo.

Hi @lenary, I added a PoC patch D78035  to 
complete ThinLTO based on this patch.

There is also a missed hook in `ParallelCG.cpp`

  @@ -28,6 +28,7 @@ static void codegen(Module *M, llvm::raw_pwrite_stream &OS,
   function_ref()> TMFactory,
   CodeGenFileType FileType) {
 std::unique_ptr TM = TMFactory();
  +  TM->initializeOptionsWithModuleMetadata(*M);
 legacy::PassManager CodeGenPasses;
 if (TM->addPassesToEmitFile(CodeGenPasses, OS, nullptr, FileType))
   report_fatal_error("Failed to setup codegen");

I'm not sure passing Module in Target::createTargetMachine() is a good or not,
because maybe the module flag will changed in next time on JIT compilation 
scenario?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72624



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

[clang-tools-extra] 31db1e0 - [clangd] Send the correct error code when cancelling requests.

[Sam McCall via cfe-commits]

Author: Sam McCall
Date: 2020-04-13T19:42:38+02:00
New Revision: 31db1e0bd1ea020046eb7ac2359ced8c7239de1e

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

LOG: [clangd] Send the correct error code when cancelling requests.

Summary:
I couldn't quite bring myself to make Cancellation depend on LSP ErrorCode.
Magic numbers instead...

Reviewers: kadircet

Subscribers: ilya-biryukov, javed.absar, MaskRay, jkorous, arphaman, jfb, 
usaxena95, cfe-commits

Tags: #clang

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

Added: 


Modified: 
clang-tools-extra/clangd/Cancellation.cpp
clang-tools-extra/clangd/Cancellation.h
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/JSONTransport.cpp
clang-tools-extra/clangd/Protocol.h
clang-tools-extra/clangd/TUScheduler.cpp
clang-tools-extra/clangd/unittests/CancellationTests.cpp
clang-tools-extra/clangd/unittests/JSONTransportTests.cpp
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp

Removed: 




diff  --git a/clang-tools-extra/clangd/Cancellation.cpp 
b/clang-tools-extra/clangd/Cancellation.cpp
index ccd27a1452d1..2120e700e719 100644
--- a/clang-tools-extra/clangd/Cancellation.cpp
+++ b/clang-tools-extra/clangd/Cancellation.cpp
@@ -16,27 +16,28 @@ char CancelledError::ID = 0;
 
 // We don't want a cancelable scope to "shadow" an enclosing one.
 struct CancelState {
-  std::shared_ptr> Cancelled;
+  std::shared_ptr> Cancelled;
   const CancelState *Parent;
 };
 static Key StateKey;
 
-std::pair cancelableTask() {
+std::pair cancelableTask(int Reason) {
+  assert(Reason != 0 && "Can't detect cancellation if Reason is zero");
   CancelState State;
-  State.Cancelled = std::make_shared>();
+  State.Cancelled = std::make_shared>();
   State.Parent = Context::current().get(StateKey);
   return {
   Context::current().derive(StateKey, State),
-  [Flag(State.Cancelled)] { *Flag = true; },
+  [Reason, Flag(State.Cancelled)] { *Flag = Reason; },
   };
 }
 
-bool isCancelled(const Context &Ctx) {
+int isCancelled(const Context &Ctx) {
   for (const CancelState *State = Ctx.get(StateKey); State != nullptr;
State = State->Parent)
-if (State->Cancelled->load())
-  return true;
-  return false;
+if (int Reason = State->Cancelled->load())
+  return Reason;
+  return 0;
 }
 
 } // namespace clangd

diff  --git a/clang-tools-extra/clangd/Cancellation.h 
b/clang-tools-extra/clangd/Cancellation.h
index bdb0bd4ef09f..0bee6f355c39 100644
--- a/clang-tools-extra/clangd/Cancellation.h
+++ b/clang-tools-extra/clangd/Cancellation.h
@@ -71,20 +71,24 @@ using Canceler = std::function;
 
 /// Defines a new task whose cancellation may be requested.
 /// The returned Context defines the scope of the task.
-/// When the context is active, isCancelled() is false until the Canceler is
-/// invoked, and true afterwards.
-std::pair cancelableTask();
+/// When the context is active, isCancelled() is 0 until the Canceler is
+/// invoked, and equal to Reason afterwards.
+/// Conventionally, Reason may be the LSP error code to return.
+std::pair cancelableTask(int Reason = 1);
 
-/// True if the current context is within a cancelable task which was 
cancelled.
+/// If the current context is within a cancelled task, returns the reason.
 /// (If the context is within multiple nested tasks, true if any are 
cancelled).
-/// Always false if there is no active cancelable task.
+/// Always zero if there is no active cancelable task.
 /// This isn't free (context lookup) - don't call it in a tight loop.
-bool isCancelled(const Context &Ctx = Context::current());
+int isCancelled(const Context &Ctx = Context::current());
 
 /// Conventional error when no result is returned due to cancellation.
 class CancelledError : public llvm::ErrorInfo {
 public:
   static char ID;
+  const int Reason;
+
+  CancelledError(int Reason) : Reason(Reason) {}
 
   void log(llvm::raw_ostream &OS) const override {
 OS << "Task was cancelled.";

diff  --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp 
b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index eafe353bb70e..f8e1d1f8bd3f 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -409,7 +409,8 @@ class ClangdLSPServer::MessageHandler : public 
Transport::MessageHandler {
   //  - cleans up the entry in RequestCancelers when it's no longer needed
   // If a client reuses an ID, the last wins and the first cannot be canceled.
   Context cancelableRequestContext(const llvm::json::Value &ID) {
-auto Task = cancelableTask();
+auto Task = cancelableTask(
+/*Reason=*/static_cast(ErrorCode::RequestCancelled));
 auto StrID = llvm::to_str

[PATCH] D77954: [CUDA][HIP] Fix host/device based overload resolution

[John McCall via Phabricator via cfe-commits]

rjmccall added a reviewer: echristo.
rjmccall added a subscriber: echristo.
rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaOverload.cpp:9481
+  // emitted, Cand1 is not better than Cand2. This rule should have precedence
+  // over other rules.
+  //

yaxunl wrote:
> rjmccall wrote:
> > Please add `[CUDA]` or something similar to the top of this comment so that 
> > readers can immediately know that it's dialect-specific.
> > 
> > At a high level, this part of the rule is essentially saying that CUDA 
> > non-emittability is a kind of non-viability.  Should we just make 
> > non-emittable functions get flagged as non-viable (which will avoid a lot 
> > of relatively expensive conversion checking), or is it important to be able 
> > to select non-emittable candidates over candidates that are non-viable for 
> > other reasons?
> There are two situations for "bad" callees:
> 
> 1. the callee should never be called. It is not just invalid call in codegen, 
> but also invalid call in AST. e.g. a host function call a device function. In 
> CUDA call preference, it is termed "never". And clang already removed such 
> callees from overload candidates.
> 
> 2. the callee should not be called in codegen, but may be called in AST. This 
> happens with `__host__ __device__` functions when calling a "wrong sided" 
> function. e.g. in device compilation, a `__host__ __device__` function calls 
> a `__host__` function. This is valid in AST since the `__host__ __device__` 
> function may be an inline function which is only called by a `__host__` 
> function. There is a deferred diagnostic for the wrong-sided call, which is 
> triggered only if the caller is emitted. However in overloading resolution, 
> if no better candidates are available, wrong-sided candidates are still 
> viable.
Oh, I see what you're saying; sorry, I mis-read the code.  So anything with a 
preference *worse* than wrong-sided is outright non-viable; there's a very 
strong preference against wrong-sided calls that takes priority of all of the 
normal overload-resolution rules; and then there's a very weak preference 
against non-exact matches that everything else takes priority over.  Okay.



Comment at: clang/lib/Sema/SemaOverload.cpp:9749
+  if (isBetterMultiversionCandidate(Cand1, Cand2))
+return true;
+

yaxunl wrote:
> rjmccall wrote:
> > If we move anything below this check, it needs to figure out a tri-state so 
> > that it can return false if `Cand2` is a better candidate than `Cand1`.  
> > Now, that only matters if multiversion functions are supported under CUDA, 
> > but if you're relying on them not being supported, that should at least be 
> > commented on.
> multiversion host functions is orthogonal to CUDA therefore should be 
> supported. multiversion in device, host device, and global functions are not 
> supported. However this change does not make things worse, and should 
> continue to work if they are supported.
> 
> host/device based overloading resolution is mostly for determining viability 
> of a function. If two functions are both viable, other factors should take 
> precedence in preference. This general rule has been taken for cases other 
> than multiversion, I think it should also apply to multiversion.
> 
> I will make isBetterMultiversionCandidate three states.
> This general rule has been taken for cases other than multiversion, I think 
> it should also apply to multiversion.

Well, but the multiversion people could say the same: that multiversioning is 
for picking an alternative among otherwise-identical functions, and HD and H 
functions are not otherwise-identical.

CC'ing @echristo for his thoughts on the right ordering here.


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

https://reviews.llvm.org/D77954



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

[PATCH] D76182: [AVR] Support aliases in non-zero address space

[John McCall via Phabricator via cfe-commits]

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

Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76182



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

[PATCH] D77962: PR38490: Support reading values out of __uuidof(X) expressions during constant evaluation.

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:3949
+  // Special-case reading from __uuidof expressions so we don't have to
+  // construct an APValue for the whole uuid.
+  if (LVal.Designator.isOnePastTheEnd()) {

For my knowledge, why try to avoid constructing an APValue for the whole uuid? 
Is special casing this necessary?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77962



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

[PATCH] D77923: OpenCL: Fix some missing predefined macros

[Scott Linder via Phabricator via cfe-commits]

scott.linder added a comment.

https://www.khronos.org/registry/OpenCL/sdk/2.0/docs/man/xhtml/preprocessorDirectives.html
 describes `__OPENCL_VERSION__` as "an integer reflecting the version number of 
the OpenCL supported by the OpenCL device." as contrasted with 
`__OPENCL_C_VERSION__` which is described as "an integer reflecting the OpenCL 
C version specified by the -cl-std build option to clBuildProgram or 
clCompileProgram. If the -cl-std build option is not specified, the OpenCL C 
version supported by the compiler for this OpenCL device will be used."

But I don't see where the correct use of these is defined? How should the user 
decide which to use? It does seem like `__OPENCL_VERSION__` and 
`__OPENCL_C_VERSION__` can differ, e.g. if you compile for a device supporting 
2.0 with `-cl-std=1.2`, but why would `__OPENCL_VERSION__` ever be referenced 
then?


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

https://reviews.llvm.org/D77923



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

[PATCH] D77859: Revert "[DomTree] Replace ChildrenGetter with GraphTraits over GraphDiff."

[Uday Bondhugula via Phabricator via cfe-commits]

bondhugula abandoned this revision.
bondhugula added a comment.

Another revision performed this revert. Dropping.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77859



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

[PATCH] D78038: [clangd] WIP: fix several bugs relating to include insertion

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay, 
ilya-biryukov.
Herald added a project: clang.

This should probably be untangled into 3 separate patches, but
looking for feedback first on whether we want to do this at all.

a) store include-insertion information for main-file symbols that are

  not eligible for code completion indexing.

b) consider an #ifndef guard that's dangling at the end of the preamble

  to be a well-formed header guard (otherwise we never will)

c) load HeaderFileInfo for the preamble main-file even if the file-size

  mismatches


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78038

Files:
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
  clang/lib/Lex/Lexer.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  llvm/include/llvm/Support/OnDiskHashTable.h

Index: llvm/include/llvm/Support/OnDiskHashTable.h
===
--- llvm/include/llvm/Support/OnDiskHashTable.h
+++ llvm/include/llvm/Support/OnDiskHashTable.h
@@ -320,8 +320,8 @@
 
   class iterator {
 internal_key_type Key;
-const unsigned char *const Data;
-const offset_type Len;
+const unsigned char *Data;
+offset_type Len;
 Info *InfoObj;
 
   public:
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1648,7 +1648,6 @@
 
   endian::Writer LE(Out, little);
   uint64_t Start = Out.tell(); (void)Start;
-
   unsigned char Flags = (Data.HFI.isImport << 5)
   | (Data.HFI.isPragmaOnce << 4)
   | (Data.HFI.DirInfo << 1)
@@ -1768,6 +1767,10 @@
 
   SmallVector FilesByUID;
   HS.getFileMgr().GetUniqueIDMapping(FilesByUID);
+  const auto &SM = Context->getSourceManager();
+  unsigned MainFileUID = -1;
+  if (const auto *Entry = SM.getFileEntryForID(SM.getMainFileID()))
+MainFileUID = Entry->getUID();
 
   if (FilesByUID.size() > HS.header_file_size())
 FilesByUID.resize(HS.header_file_size());
@@ -1806,6 +1809,13 @@
 };
 Generator.insert(Key, Data, GeneratorTrait);
 ++NumHeaderSearchEntries;
+// We may reuse a preamble even if the rest of the file is different, so
+// allow looking up info for the main file with a zero size.
+if (File->getUID() == MainFileUID) {
+  Key.Size = 0;
+  Generator.insert(Key, Data, GeneratorTrait);
+  ++NumHeaderSearchEntries;
+}
   }
 
   // Create the on-disk hash table in a buffer.
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -6147,9 +6147,19 @@
 = static_cast(M.HeaderFileInfoTable);
   if (!Table)
 return false;
-
   // Look in the on-disk hash table for an entry for this file name.
   HeaderFileInfoLookupTable::iterator Pos = Table->find(FE);
+  // Preambles may be reused with different main-file content.
+  // A second entry with size zero is stored for the main-file, try that.
+  // To avoid doing this on every miss, require the bare filename to match.
+  if (Pos == Table->end() && M.Kind == clang::serialization::MK_Preamble &&
+  llvm::sys::path::filename(FE->getName()) ==
+  llvm::sys::path::filename(M.OriginalSourceFileName)) {
+auto InternalKey = Table->getInfoObj().GetInternalKey(FE);
+InternalKey.Size = 0;
+Pos = Table->find_hashed(InternalKey,
+ Table->getInfoObj().ComputeHash(InternalKey));
+  }
   if (Pos == Table->end())
 return false;
 
Index: clang/lib/Lex/Lexer.cpp
===
--- clang/lib/Lex/Lexer.cpp
+++ clang/lib/Lex/Lexer.cpp
@@ -2743,6 +2743,11 @@
 
   if (PP->isRecordingPreamble() && PP->isInPrimaryFile()) {
 PP->setRecordedPreambleConditionalStack(ConditionalStack);
+// If the preamble cuts off the end of a header guard, consider it guarded.
+// The guard is valid for the preamble content itself, and for tools the
+// most useful answer is "yes, this file has a header guard".
+if (!ConditionalStack.empty())
+  MIOpt.ExitTopLevelConditional();
 ConditionalStack.clear();
   }
 
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -1283,6 +1283,47 @@
   EXPECT_THAT(Symbols, Each(IncludeHeader()));
 }
 
+TEST_F(SymbolCollectorTest, HeaderGuardDetectedPragmaInPreamble) {
+  // TestTU builds with a 

[PATCH] D77936: [Windows SEH] Fix abnormal-exits in _try

[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/D77936/new/

https://reviews.llvm.org/D77936



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

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Thanks.  Just a bunch of fairly minor requests now.




Comment at: clang/include/clang/AST/Expr.h:3474
+  size_t offsetOfTrailingStorage() const;
+  size_t offsetOfTrailingStorage();
+

You don't need the non-const overload.



Comment at: clang/include/clang/AST/Expr.h:3719
+  /// Used to allocate the right amount of storage.
+  static unsigned sizeOfTrailingObjects(unsigned HasFPFeatures) {
+return HasFPFeatures * sizeof(FPOptions);

Should `HasFPFeatures` be a `bool` everywhere you're passing it around?



Comment at: clang/lib/AST/Expr.cpp:4397
+   : sizeof(BinaryOperator);
+}
+

Please define this in the header.  You can just do:

```
inline size_t BinaryOperator::offsetOfTrailingStorage() const { ... }
```

after both of the classes are fully defined.



Comment at: clang/lib/Analysis/BodyFarm.cpp:120
+VK_RValue, OK_Ordinary, SourceLocation(),
+FPOptions::defaultWithoutTrailingStorage(C));
 }

These construction sites don't seem like appropriate uses of 
`defaultWithoutTrailingStorage`, which is an implementation detail of the AST 
nodes.  The right behavior here is for all of these places to use the default 
`FPOptions` from the language options, then let the AST figure out the right 
way to store that.  That happens to have the same effect currently as 
`defaultWithoutTrailingStorage`, but the latter should be able to change, while 
the right behavior for these places will remain the same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76384



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

[PATCH] D78019: HIP: Fix handling of denormal mode

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

LGTM. The patch appears to be an NFC one for CUDA.


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

https://reviews.llvm.org/D78019



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

[PATCH] D77947: [clangd] Send the correct error code when cancelling requests.

[Sam McCall via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clangd/JSONTransport.cpp:28
+  [&](const CancelledError &C) -> llvm::Error {
+switch (C.Reason) {
+  case static_cast(ErrorCode::ContentModified):

kadircet wrote:
> Maybe `static_cast(C.Reason)` instead of casting cases.
> 
> I know the reason is not necessarily LSP specific, but we seem to be 
> defaulting to `ErrorCode::RequestCancelled` anyways.
Sadly this is UB if the value isn't an element of the enum (or in its range, or 
something). Whereas casting the cases leaves the code valid for arbitrary 
integers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77947



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

[PATCH] D76612: [Matrix] Add draft specification for matrix support in Clang.

[Florian Hahn via Phabricator via cfe-commits]

fhahn added inline comments.



Comment at: clang/docs/MatrixSupport.rst:3
+Matrix Support
+==
+

rjmccall wrote:
> This extension should be called something like "Matrices" or "Matrix Types".  
> The "X Support" name makes it sound like it's a support layer for some 
> external technology.
I changed it to "Matrix Types"



Comment at: clang/docs/MatrixSupport.rst:30
+directly. An *attribute-argument-clause* must be present and it shall have the
+form:
+

rjmccall wrote:
> You can assume the existence of a hypothetical external language 
> specification for GNU attribute syntax, so these starting paragraphs whittle 
> down to:
> 
> > Matrix types can be declared by adding the `matrix_type` attribute to the 
> > declaration of a `typedef` (or a C++ alias declaration).  The underlying 
> > type of the `typedef` must be an unqualified integer or floating-point 
> > type.  The attribute takes two arguments, both of which must be integer 
> > constant expressions that evaluate to a value greater than zero.  The first 
> > specifies the number of rows, and the second specifies the number of 
> > columns.  The underlying type of the `typedef` becomes a matrix type with 
> > the given dimensions and an element type of the former underlying type.
> 
> The paragraph about redeclarations is good.
Thanks, that helps to simplify this section a lot.



Comment at: clang/docs/MatrixSupport.rst:48
+Matrix Type
+---
+

rjmccall wrote:
> I would put this first before getting into the spelling.  You can also put 
> the stuff about implementation limits on dimensions in here.
Sounds good! Moved.



Comment at: clang/docs/MatrixSupport.rst:57
+
+A matrix type is a *scalar type* and its alignment is implementation defined.
+

rjmccall wrote:
> These are both important to include, but they're unrelated and shouldn't be 
> in the same sentence.
I moved the `scalar type` bit to the first sentence and move the part about the 
alignment to a separate sentence stating that the layout overall size and 
alignment are implementation defined.



Comment at: clang/docs/MatrixSupport.rst:64
+
+TODO: Specify how matrix values are passed to functions.
+

rjmccall wrote:
> That doesn't belong in a language specification, but you could reasonably add 
> a non-normative section at the end about the decisions that Clang currently 
> makes for things like size, alignment, internal layout, and argument/result 
> conventions.
I've added a new  `Decisions for the Implementation in Clang` section



Comment at: clang/docs/MatrixSupport.rst:110
+the matrix. The expression E1 is sequenced before E2 and E3. The expressions
+E2 and E3 are unsequenced.
+

rjmccall wrote:
> I'd put all this like:
> 
> > An expression of the form ``E1 [E2] [E3]``, where ``E1`` has matrix type 
> > ``cv M``, is a matrix element access expression.  Let ``T`` be the element 
> > type of ``M``, and let ``R`` and ``C`` be the number of rows and columns in 
> > ``M`` respectively.  The index expressions shall have integral or unscoped 
> > enumeration type and shall not be uses of the comma operator unless 
> > parenthesized.  The first index expression shall evaluate to a non-negative 
> > value less than ``R``, and the second index expression shall evaluate to a 
> > non-negative value less than ``C``, or else the expression has undefined 
> > behavior.  If ``E1`` is a prvalue, the result is a prvalue with type ``T`` 
> > and is the value of the element at the given row and column in the matrix.  
> > Otherwise, the result is a glvalue with type ``cv T`` and with the same 
> > value category as ``E1`` which refers to the element at the given row and 
> > column in the matrix.
Updated, thanks.



Comment at: clang/docs/MatrixSupport.rst:118
+builtins to extract rows and columns from a matrix. This makes the operations
+more explicit.
+

rjmccall wrote:
> You should add a normative paragraph saying that a program is ill-formed if 
> it insufficiently subscripts into a matrix.
I added the following
> Programs containing a single subscript expression into a matrix are 
> ill-formed.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76612



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

[PATCH] D76612: [Matrix] Add draft specification for matrix support in Clang.

[Florian Hahn via Phabricator via cfe-commits]

fhahn updated this revision to Diff 257025.
fhahn marked 15 inline comments as done.
fhahn added a comment.

Address @rjmccall comments.

In D76612#1975719 , @rjmccall wrote:

> Scanned through the first bit.


Thanks a lot! I hope I managed to address the comments adequately.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76612

Files:
  clang/docs/LanguageExtensions.rst
  clang/docs/MatrixSupport.rst

Index: clang/docs/MatrixSupport.rst
===
--- /dev/null
+++ clang/docs/MatrixSupport.rst
@@ -0,0 +1,312 @@
+==
+Matrix Types
+==
+
+.. contents::
+   :local:
+
+.. _matrixsupport:
+
+Clang provides a C/C++ language extension that allows users to directly express
+fixed-size matrices as language values and perform arithmetic on them.
+
+This feature is currently experimental, and both its design and its
+implementation are in flux.
+
+.. _matrixsupport-draftspec:
+
+Draft Specification
+===
+
+Matrix Type
+---
+
+A matrix type is a scalar type with an underlying *element type*, a constant
+number of rows, and a constant number of columns. Matrix types with the same
+element type, rows, and columns are the same type. A value of a matrix type
+includes storage for ``rows * columns`` values of the *element type*. The
+internal layout, overall size and alignment are implementation-defined.
+
+The maximum of the product of the number of rows and columns is
+implementation-defined. If that implementation-defined limit is exceeded, the
+program is ill-formed.
+
+TODO: Does it make sense to allow M::element_type, M::rows, and M::columns
+where M is a matrix type? We don’t support this anywhere else, but it’s
+convenient. The alternative is using template deduction to extract this
+information. Also add spelling for C.
+
+Future Work: Initialization syntax.
+
+Matrix Type Attribute
+-
+
+Matrix types can be declared by adding the ``matrix_type`` attribute to the
+declaration of a *typedef* (or a C++ alias declaration). The underlying type
+of the *typedef* must be an unqualified integer or floating-point type. The
+attribute takes two arguments, both of which must be integer constant
+expressions that evaluate to a value greater than zero. The first specifies the
+number of rows, and the second specifies the number of columns. The underlying
+type of the *typedef* becomes a matrix type with the given dimensions and an
+element type of the former underlying type.
+
+If a declaration of a *typedef-name* has a ``matrix_type`` attribute, then all
+declaration of that *typedef-name* shall have a matrix_type attribute with the
+same element type, number of rows, and number of columns.
+
+Standard Conversions
+
+
+The standard conversions are extended as follows. Note that these conversions
+are intentionally not listed as satisfying the constraints for assignment,
+which is to say, they are only permitted as explicit casts, not as implicit
+conversions.
+
+For integral promotions, floating-point promotion, integral conversions,
+floating-point conversions, and floating-integral conversions: apply the rules
+to the underlying type of the matrix type. The resulting type is a matrix type
+with that underlying element type. If the number of rows or columns differ
+between the original and resulting type, the program is ill-formed. Otherwise
+the resulting value is as follows:
+
+* If the original value was of matrix type, each element is converted element
+  by element.
+* If the original value was not of matrix type, each element takes the value of
+  the original value.
+
+Arithmetic Conversions
+--
+
+The usual arithmetic conversions are extended as follows.
+
+Insert at the start:
+
+* If both operands are of matrix type, no arithmetic conversion is performed.
+* If one operand is of matrix type and the other operand is of an integer or
+  floating point type, convert the integer or floating point operand to the
+  underlying element type of the operand of matrix type.
+
+Matrix Type Element Access Operator
+---
+
+An expression of the form ``E1 [E2] [E3]``, where ``E1`` has matrix type ``cv
+M``, is a matrix element access expression.  Let ``T`` be the element type
+of ``M``, and let ``R`` and ``C`` be the number of rows and columns in ``M``
+respectively.  The index expressions shall have integral or unscoped
+enumeration type and shall not be uses of the comma operator unless
+parenthesized.  The first index expression shall evaluate to a
+non-negative value less than ``R``, and the second index expression shall
+evaluate to a non-negative value less than ``C``, or else the expression has
+undefined behavior.  If ``E1`` is a prvalue, the result is a prvalue with type
+``T`` and is the valu

[PATCH] D77923: OpenCL: Fix some missing predefined macros

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

In D77923#1978172 , @scott.linder 
wrote:

> https://www.khronos.org/registry/OpenCL/sdk/2.0/docs/man/xhtml/preprocessorDirectives.html
>  describes `__OPENCL_VERSION__` as "an integer reflecting the version number 
> of the OpenCL supported by the OpenCL device." as contrasted with 
> `__OPENCL_C_VERSION__` which is described as "an integer reflecting the 
> OpenCL C version specified by the -cl-std build option to clBuildProgram or 
> clCompileProgram. If the -cl-std build option is not specified, the OpenCL C 
> version supported by the compiler for this OpenCL device will be used."
>
> But I don't see where the correct use of these is defined? How should the 
> user decide which to use? It does seem like `__OPENCL_VERSION__` and 
> `__OPENCL_C_VERSION__` can differ, e.g. if you compile for a device 
> supporting 2.0 with `-cl-std=1.2`, but why would `__OPENCL_VERSION__` ever be 
> referenced then?


I don't know why you would ever use this; but the standard says it should be 
defined, so I guess we have to define it


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

https://reviews.llvm.org/D77923



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

[PATCH] D77947: [clangd] Send the correct error code when cancelling requests.

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG31db1e0bd1ea: [clangd] Send the correct error code when 
cancelling requests. (authored by sammccall).

Changed prior to commit:
  https://reviews.llvm.org/D77947?vs=256777&id=257031#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77947

Files:
  clang-tools-extra/clangd/Cancellation.cpp
  clang-tools-extra/clangd/Cancellation.h
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/JSONTransport.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/unittests/CancellationTests.cpp
  clang-tools-extra/clangd/unittests/JSONTransportTests.cpp
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp

Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -415,7 +415,9 @@
 EXPECT_FALSE(bool(AST));
 llvm::Error E = AST.takeError();
 EXPECT_TRUE(E.isA());
-consumeError(std::move(E));
+handleAllErrors(std::move(E), [&](const CancelledError &E) {
+  EXPECT_EQ(E.Reason, static_cast(ErrorCode::ContentModified));
+});
   },
   TUScheduler::InvalidateOnUpdate);
   S.runWithAST(
Index: clang-tools-extra/clangd/unittests/JSONTransportTests.cpp
===
--- clang-tools-extra/clangd/unittests/JSONTransportTests.cpp
+++ clang-tools-extra/clangd/unittests/JSONTransportTests.cpp
@@ -5,6 +5,7 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===--===//
+#include "Cancellation.h"
 #include "Protocol.h"
 #include "Transport.h"
 #include "gmock/gmock.h"
@@ -70,6 +71,9 @@
 if (Method == "err")
   Target.reply(
   ID, llvm::make_error("trouble at mill", ErrorCode(88)));
+else if (Method == "invalidated") // gone out skew on treadle
+  Target.reply(ID, llvm::make_error(
+   static_cast(ErrorCode::ContentModified)));
 else
   Target.reply(ID, std::move(Params));
 return true;
@@ -140,7 +144,7 @@
 ---
 {"jsonrpc": "2.0", "id": "xyz", "error": {"code": 99, "message": "bad!"}}
 ---
-{"jsonrpc": "2.0", "method": "err", "id": "wxyz", "params": "boom!"}
+{"jsonrpc": "2.0", "method": "invalidated", "id": "wxyz", "params": "boom!"}
 ---
 {"jsonrpc": "2.0", "method": "exit"}
   )jsonrpc",
@@ -154,7 +158,7 @@
 Reply(1234): 5678
 Call foo("abcd"): "efgh"
 Reply("xyz"): error = 99: bad!
-Call err("wxyz"): "boom!"
+Call invalidated("wxyz"): "boom!"
 Notification exit: null
   )";
   EXPECT_EQ(trim(E.log()), trim(WantLog));
@@ -171,11 +175,11 @@
   "jsonrpc": "2.0",
   "result": "efgh"
 })"
-   "Content-Length: 105\r\n\r\n"
+   "Content-Length: 145\r\n\r\n"
R"({
   "error": {
-"code": 88,
-"message": "trouble at mill"
+"code": -32801,
+"message": "Request cancelled because the document was modified"
   },
   "id": "wxyz",
   "jsonrpc": "2.0"
Index: clang-tools-extra/clangd/unittests/CancellationTests.cpp
===
--- clang-tools-extra/clangd/unittests/CancellationTests.cpp
+++ clang-tools-extra/clangd/unittests/CancellationTests.cpp
@@ -45,12 +45,13 @@
 }
 
 struct NestedTasks {
+  enum { OuterReason = 1, InnerReason = 2 };
   std::pair Outer, Inner;
   NestedTasks() {
-Outer = cancelableTask();
+Outer = cancelableTask(OuterReason);
 {
   WithContext WithOuter(Outer.first.clone());
-  Inner = cancelableTask();
+  Inner = cancelableTask(InnerReason);
 }
   }
 };
@@ -59,13 +60,13 @@
   // Cancelling inner task works but leaves outer task unaffected.
   NestedTasks CancelInner;
   CancelInner.Inner.second();
-  EXPECT_TRUE(isCancelled(CancelInner.Inner.first));
+  EXPECT_EQ(NestedTasks::InnerReason, isCancelled(CancelInner.Inner.first));
   EXPECT_FALSE(isCancelled(CancelInner.Outer.first));
   // Cancellation of outer task is inherited by inner task.
   NestedTasks CancelOuter;
   CancelOuter.Outer.second();
-  EXPECT_TRUE(isCancelled(CancelOuter.Inner.first));
-  EXPECT_TRUE(isCancelled(CancelOuter.Outer.first));
+  EXPECT_EQ(NestedTasks::OuterReason, isCancelled(CancelOuter.Inner.first));
+  EXPECT_EQ(NestedTasks::OuterReason, isCancelled(CancelOuter.Outer.first));
 }
 
 TEST(CancellationTest, AsynCancellationTest) {
Index: clang-tools-extra/clangd/TUScheduler.cpp
===
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUSchedule

[PATCH] D74387: [SYCL] Defer __float128 type usage diagnostics

[Mariya Podchishchaeva via Phabricator via cfe-commits]

Fznamznon added a comment.

In D74387#1974981 , @jdoerfert wrote:

> As I mentioned before. As long as the type is not "used" you can treat it as 
> a sequence of bytes just as well. So we can lower `__float128` to `char [16]` 
> with the right alignment. SPIRV will never see unsupported types and the code 
> works because we never access it as `float128` anyway. WDYT?


Yes, it can work for SYCL without additional diagnostics if it is possible to 
replace `__float128` with `char [16]` everywhere (including struct definitions 
and so on) in the resulting LLVM IR module.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74387



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

[PATCH] D78019: HIP: Fix handling of denormal mode

[Yaxun Liu via Phabricator via cfe-commits]

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

LGTM. Thanks.


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

https://reviews.llvm.org/D78019



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

[PATCH] D77923: OpenCL: Fix some missing predefined macros

[Scott Linder via Phabricator via cfe-commits]

scott.linder added a comment.

In D77923#1978261 , @arsenm wrote:

> In D77923#1978172 , @scott.linder 
> wrote:
>
> > https://www.khronos.org/registry/OpenCL/sdk/2.0/docs/man/xhtml/preprocessorDirectives.html
> >  describes `__OPENCL_VERSION__` as "an integer reflecting the version 
> > number of the OpenCL supported by the OpenCL device." as contrasted with 
> > `__OPENCL_C_VERSION__` which is described as "an integer reflecting the 
> > OpenCL C version specified by the -cl-std build option to clBuildProgram or 
> > clCompileProgram. If the -cl-std build option is not specified, the OpenCL 
> > C version supported by the compiler for this OpenCL device will be used."
> >
> > But I don't see where the correct use of these is defined? How should the 
> > user decide which to use? It does seem like `__OPENCL_VERSION__` and 
> > `__OPENCL_C_VERSION__` can differ, e.g. if you compile for a device 
> > supporting 2.0 with `-cl-std=1.2`, but why would `__OPENCL_VERSION__` ever 
> > be referenced then?
>
>
> I don't know why you would ever use this; but the standard says it should be 
> defined, so I guess we have to define it


Right, I just want to understand the actual semantics and it doesn't seem 
clear. I agree that I don't see a use for `__OPENCL_VERSION__` but we define it 
so I'm sure people are using it, so likely we need to be careful that this 
patch preserves the existing behavior of the runtime.

In D77923#1976497 , @jvesely wrote:

> OPENCL_VERSION is something that should be really set by the opencl driver 
> rather than the compiler.
>  coarse-grained SVM can be done without FEATURE_FLAT_ADDRESS_SPACE, so those 
> gpus can get over 1.2, while the compiler can be used in an implementation 
> that doesn't go above 1.2 even for gfx700+


I don't know why this would imply we can't just have a static 
`__OPENCL_VERSION__` for each device in Clang? I don't see anything in the 
standard that says you are not allowed to have (going with your example) 
`__OPENCL_VERSION__=200` and `__OPENCL_C_VERSION=120` at the same time.

It seems like we should be able to just have `__OPENCL_VERSION__` defined in 
Clang, and let the runtime control setting `__OPENCL_C_VERSION__` via 
`-cl-std=`. If the current patch sets `__OPENCL_VERSION__` differently than the 
runtime does then it seems like we should make it match, but I'm not sure how 
best to confirm that.


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

https://reviews.llvm.org/D77923



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

[PATCH] D77683: [Docs] Make code review policy clearer about requested pre-commit reviews

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D77683#1978070 , @hfinkel wrote:

> In D77683#1973762 , @dblaikie wrote:
>
> > In D77683#1973757 , @jdoerfert 
> > wrote:
> >
> > > In D77683#1970826 , @mehdi_amini 
> > > wrote:
> > >
> > > > I am still not sure what "if someone has asked for extra review of a 
> > > > specific area" refers to?
> > >
> > >
> > > As said earlier
> > >
> > > >>   If I understand this correctly, this is meant to cover situations 
> > > >> where reviewers are active in an area and indicated an interest in 
> > > >> reviewing basically everything.
> > > > 
> > > > Pretty much, yes.
> > >
> > > this should mean that, if requested, all non-trivial patches should go 
> > > through review. The current wording is very lenient, especially wrt. code 
> > > owners.
> > >  While most people I talked to don't see owners as special per se (but 
> > > just assume they have more responsibility), this paragraph says they have 
> > > special rights.
> > >  Given the murky ways owners are "selected", I think we should have a 
> > > well defined way to limit these rights without revoking the status.
> >
> >
> > I disagree here. I think it's suitable (within the way the LLVM project 
> > works) for code owners to commit without review - given they are the 
> > arbiters of what's acceptable in that subcomponent - ultimately they can 
> > veto anyone else (short of a broader project/code owner). Doesn't usually 
> > come to that, but that's what the ownership role means.
> >
> > I don't think it's correct for arbitrary contributors to say "you need 
> > to/this component needs review-before-commit" - the code owner could say 
> > that if they really don't trust any of the contributors to conform to the 
> > direction they have in mind for the component (that's not to discredit the 
> > contributors - but that's the point of pre-commit review: to ensure it 
> > conforms to the code owners vision for the component). 
> > privledges
> >  (yes, code owners aren't meant to be dictators - but they're ultimately 
> > the final decider)
>
>
> I disagree that this characterizes our conception of the role of code owner 
> within the project, but there is some mitigating context. In my experience, 
> we describe code owners as "reviewers of last resort", and their primary 
> responsibility is to prevent review stagnation with their component 
> (including from new/infrequent contributors whose patches might otherwise go 
> unreviewed). The code owner is not the final decider in all cases, but can 
> serve as a tie breaker when community consensus is unclear. That's an 
> important role, because it ensures our ability to make forward progress, but 
> is different in character from someone with overriding final authority. The 
> code owners vision matters only slightly if the community consensus is for a 
> different vision. There certainly are parts of LLVM that are developed by one 
> person, and that person is the code owner, and thus excessive a lot of 
> influence over the future direction of the component. Patches are often 
> contributed without pre-commit review, in this case, and the code owner is 
> often the most-skilled reviewer for any other patches as well. However, this 
> state exists only at the pleasure of the rest of the community - we all see 
> these patches on the commits list, and should more people become involved and 
> request a more-inclusive pre-commit review cycle, that's what should happen. 
> For components actively developed by many people, code owners have relatively 
> minor privileges in this regard.
>
> I suppose that we never wrote this down anywhere, but when I became code 
> owner for the AA subsystem, we had an understanding that, because AA is so 
> pervasive and subtle, even as code owner I would never commit anything (aside 
> from reverts or similar) without pre-commit review. IMHO, this is the only 
> responsible way to handle this subsystem. Not all subsystems are so risky, so 
> there's appropriately a spectrum of development practices (some favoring 
> velocity over stability), but I still view this as being more democratic than 
> hierarchically authoritative.


Fair enough - it sounded like the instigating incident in this case was maybe 
the other end of the spectrum - one developer telling another developer that 
they had to send more things for review when neither were code owner & they 
seem to disagree on that line. I'm not sure that's something we could/should 
codify. I think if there's ongoing disagreement about the suitable norms in 
part of the project it'd probably be useful for a code owner (or at least some 
precedence from a collection of developers in that space) to help codify 
practices/norms in that component.


Repository:
  rG LLVM Github Monorepo

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

Re: [clang] 246398e - [clang][Parse] properly parse asm-qualifiers, asm inline

[Nick Desaulniers via cfe-commits]

On Sun, Apr 12, 2020 at 3:45 PM Joerg Sonnenberger  wrote:
>
> On Thu, Mar 12, 2020 at 03:25:49PM -0700, Nick Desaulniers via cfe-commits 
> wrote:
> >
> > Author: Nick Desaulniers
> > Date: 2020-03-12T15:13:59-07:00
> > New Revision: 246398ece7115b57a02dbe7876d86ae8e72406ef
> >
> > URL: 
> > https://github.com/llvm/llvm-project/commit/246398ece7115b57a02dbe7876d86ae8e72406ef
> > DIFF: 
> > https://github.com/llvm/llvm-project/commit/246398ece7115b57a02dbe7876d86ae8e72406ef.diff
> >
> > LOG: [clang][Parse] properly parse asm-qualifiers, asm inline
> >
> > Summary:
> > The parsing of GNU C extended asm statements was a little brittle and
> > had a few issues:
>
> I find it very questionable that this change drops a warning flag making
> IMO benign code differences like marking a global asm as volatile an
> unconditional error. This strongly seems to be something that we
> shouldn't blindly follow GCC on.

We can always bring them back.  Now that we can parse asm statements
correctly, I think we could even unify asm parsing (there's one spot
that parses asm expressions again, but without looking for the asm
qualifiers (parseAsmSimple() IIRC).  Then you could pass in whether
the qualifiers should be ignored and warn on or not.

-Wasm-file-asm-volatile was certainly too specific to `volatile`, but
it makes sense to bring back `-Wasm-ignored-qualifier` for asm
qualifiers (previously it warned for unrelated type qualifiers, which
I'm quite certain wasn't very helpful).
-- 
Thanks,
~Nick Desaulniers
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] dc89a3e - HIP: Fix handling of denormal mode

[Matt Arsenault via cfe-commits]

Author: Matt Arsenault
Date: 2020-04-13T11:48:45-07:00
New Revision: dc89a3efb43feedec04facfa2206de011d2606e7

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

LOG: HIP: Fix handling of denormal mode

I didn't realize HIP was a distinct offloading kind, so the subtarget
was looking for -march, which isn't correct for HIP. We also have the
possibility of different denormal defaults in the case of multiple
offload targets, so we need to thread the JobAction through the target
hook.

Added: 


Modified: 
clang/include/clang/Driver/ToolChain.h
clang/lib/Driver/ToolChains/AMDGPU.cpp
clang/lib/Driver/ToolChains/AMDGPU.h
clang/lib/Driver/ToolChains/Clang.cpp
clang/lib/Driver/ToolChains/Cuda.cpp
clang/lib/Driver/ToolChains/Cuda.h
clang/lib/Driver/ToolChains/Linux.cpp
clang/lib/Driver/ToolChains/Linux.h
clang/lib/Driver/ToolChains/PS4CPU.h
clang/test/Driver/cuda-flush-denormals-to-zero.cu

Removed: 




diff  --git a/clang/include/clang/Driver/ToolChain.h 
b/clang/include/clang/Driver/ToolChain.h
index 66f22d538138..fb3cbd7f84c8 100644
--- a/clang/include/clang/Driver/ToolChain.h
+++ b/clang/include/clang/Driver/ToolChain.h
@@ -636,8 +636,7 @@ class ToolChain {
   /// environment for the given \p FPType if given. Otherwise, the default
   /// assumed mode for any floating point type.
   virtual llvm::DenormalMode getDefaultDenormalModeForType(
-  const llvm::opt::ArgList &DriverArgs,
-  Action::OffloadKind DeviceOffloadKind,
+  const llvm::opt::ArgList &DriverArgs, const JobAction &JA,
   const llvm::fltSemantics *FPType = nullptr) const {
 return llvm::DenormalMode::getIEEE();
   }

diff  --git a/clang/lib/Driver/ToolChains/AMDGPU.cpp 
b/clang/lib/Driver/ToolChains/AMDGPU.cpp
index f09578f4769e..2a796f28403f 100644
--- a/clang/lib/Driver/ToolChains/AMDGPU.cpp
+++ b/clang/lib/Driver/ToolChains/AMDGPU.cpp
@@ -273,18 +273,22 @@ bool AMDGPUToolChain::getDefaultDenormsAreZeroForTarget(
 }
 
 llvm::DenormalMode AMDGPUToolChain::getDefaultDenormalModeForType(
-const llvm::opt::ArgList &DriverArgs, Action::OffloadKind 
DeviceOffloadKind,
+const llvm::opt::ArgList &DriverArgs, const JobAction &JA,
 const llvm::fltSemantics *FPType) const {
   // Denormals should always be enabled for f16 and f64.
   if (!FPType || FPType != &llvm::APFloat::IEEEsingle())
 return llvm::DenormalMode::getIEEE();
 
-  if (DeviceOffloadKind == Action::OFK_Cuda) {
+  if (JA.getOffloadingDeviceKind() == Action::OFK_HIP ||
+  JA.getOffloadingDeviceKind() == Action::OFK_Cuda) {
+auto Kind = llvm::AMDGPU::parseArchAMDGCN(JA.getOffloadingArch());
 if (FPType && FPType == &llvm::APFloat::IEEEsingle() &&
 DriverArgs.hasFlag(options::OPT_fcuda_flush_denormals_to_zero,
options::OPT_fno_cuda_flush_denormals_to_zero,
-   false))
+   getDefaultDenormsAreZeroForTarget(Kind)))
   return llvm::DenormalMode::getPreserveSign();
+
+return llvm::DenormalMode::getIEEE();
   }
 
   const StringRef GpuArch = DriverArgs.getLastArgValue(options::OPT_mcpu_EQ);
@@ -294,7 +298,9 @@ llvm::DenormalMode 
AMDGPUToolChain::getDefaultDenormalModeForType(
   // them all?
   bool DAZ = DriverArgs.hasArg(options::OPT_cl_denorms_are_zero) ||
  getDefaultDenormsAreZeroForTarget(Kind);
-  // Outputs are flushed to zero, preserving sign
+
+  // Outputs are flushed to zero (FTZ), preserving sign. Denormal inputs are
+  // also implicit treated as zero (DAZ).
   return DAZ ? llvm::DenormalMode::getPreserveSign() :
llvm::DenormalMode::getIEEE();
 }

diff  --git a/clang/lib/Driver/ToolChains/AMDGPU.h 
b/clang/lib/Driver/ToolChains/AMDGPU.h
index 87a16272d624..afd71e1f595b 100644
--- a/clang/lib/Driver/ToolChains/AMDGPU.h
+++ b/clang/lib/Driver/ToolChains/AMDGPU.h
@@ -214,8 +214,7 @@ class LLVM_LIBRARY_VISIBILITY AMDGPUToolChain : public 
Generic_ELF {
   static bool getDefaultDenormsAreZeroForTarget(llvm::AMDGPU::GPUKind GPUKind);
 
   llvm::DenormalMode getDefaultDenormalModeForType(
-  const llvm::opt::ArgList &DriverArgs,
-  Action::OffloadKind DeviceOffloadKind,
+  const llvm::opt::ArgList &DriverArgs, const JobAction &JA,
   const llvm::fltSemantics *FPType = nullptr) const override;
 };
 

diff  --git a/clang/lib/Driver/ToolChains/Clang.cpp 
b/clang/lib/Driver/ToolChains/Clang.cpp
index 5f9b6d813416..415ef27eee0a 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -2510,7 +2510,7 @@ static void CollectArgsForIntegratedAssembler(Compilation 
&C,
 static void RenderFloatingPointOptions(const ToolChain &TC, const Driver &D,
bool OFastEnabled, const ArgList &

[PATCH] D77918: [OpenMP] Avoid crash in preparation for diagnose of unsupported type

[Jon Chesterfield via Phabricator via cfe-commits]

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

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77918



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

[PATCH] D78019: HIP: Fix handling of denormal mode

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

In D78019#1978216 , @tra wrote:

> LGTM. The patch appears to be an NFC one for CUDA.


CUDA currently isn't changing the default FTZ mode based on the subtarget, 
which differs from nvcc according to the documentation


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

https://reviews.llvm.org/D78019



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

[PATCH] D77954: [CUDA][HIP] Fix host/device based overload resolution

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

LGTM in principle. That said, my gut feeling is that this patch has a good 
chance of breaking something in sufficiently convoluted CUDA code like Eigen. 
When you land this patch, I'd appreciate if you could do it on a workday 
morning (Pacific time) so I'm around to test it on our code and revert if 
something unexpected pops up.

On a side note, this case is another point towards having to redo handling of 
`__host__ __device__`.  There are way too many corner cases all over the place. 
Things will only get worse as we move towards newer C++ standard where a lot 
more code becomes constexpr which is implicitly `HD`. Having calls from `HD` 
functions resolve in a different way during host/device compilation is 
observable and may result in host and device code diverging unexpectedly.


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

https://reviews.llvm.org/D77954



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

[PATCH] D77621: ADT: SmallVector size & capacity use word-size integers when elements are small.

[Andrew via Phabricator via cfe-commits]

browneee updated this revision to Diff 257044.
browneee added a comment.

Changed SizeMax to static constexpr function.
Changed static asserts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77621

Files:
  llvm/include/llvm/ADT/SmallVector.h
  llvm/lib/Support/SmallVector.cpp

Index: llvm/lib/Support/SmallVector.cpp
===
--- llvm/lib/Support/SmallVector.cpp
+++ llvm/lib/Support/SmallVector.cpp
@@ -36,30 +36,6 @@
 static_assert(sizeof(SmallVector) ==
   sizeof(unsigned) * 2 + sizeof(void *) * 2,
   "wasted space in SmallVector size 1");
-
-/// grow_pod - This is an implementation of the grow() method which only works
-/// on POD-like datatypes and is out of line to reduce code duplication.
-void SmallVectorBase::grow_pod(void *FirstEl, size_t MinCapacity,
-   size_t TSize) {
-  // Ensure we can fit the new capacity in 32 bits.
-  if (MinCapacity > UINT32_MAX)
-report_bad_alloc_error("SmallVector capacity overflow during allocation");
-
-  size_t NewCapacity = 2 * capacity() + 1; // Always grow.
-  NewCapacity =
-  std::min(std::max(NewCapacity, MinCapacity), size_t(UINT32_MAX));
-
-  void *NewElts;
-  if (BeginX == FirstEl) {
-NewElts = safe_malloc(NewCapacity * TSize);
-
-// Copy the elements over.  No need to run dtors on PODs.
-memcpy(NewElts, this->BeginX, size() * TSize);
-  } else {
-// If this wasn't grown from the inline copy, grow the allocated space.
-NewElts = safe_realloc(this->BeginX, NewCapacity * TSize);
-  }
-
-  this->BeginX = NewElts;
-  this->Capacity = NewCapacity;
-}
+static_assert(sizeof(SmallVector) ==
+  sizeof(void *) * 2 + sizeof(void *),
+  "1 byte elements have word-sized type for size and capacity");
Index: llvm/include/llvm/ADT/SmallVector.h
===
--- llvm/include/llvm/ADT/SmallVector.h
+++ llvm/include/llvm/ADT/SmallVector.h
@@ -16,10 +16,10 @@
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/AlignOf.h"
 #include "llvm/Support/Compiler.h"
+#include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/MemAlloc.h"
 #include "llvm/Support/type_traits.h"
-#include "llvm/Support/ErrorHandling.h"
 #include 
 #include 
 #include 
@@ -27,6 +27,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -34,11 +35,23 @@
 
 namespace llvm {
 
-/// This is all the non-templated stuff common to all SmallVectors.
-class SmallVectorBase {
+/// This is all the stuff common to all SmallVectors.
+///
+/// The template parameter specifies the type which should be used to hold the
+/// Size and Capacity of the SmallVector, so it can be adjusted.
+/// Using 32 bit size is desirable to shink the size of the SmallVector.
+/// Using 64 bit size is desirable for cases like SmallVector, where a
+/// 32 bit size would limit the vector to ~4GB. SmallVectors are used for
+/// buffering bitcode output - which can exceed 4GB.
+template  class SmallVectorBase {
 protected:
   void *BeginX;
-  unsigned Size = 0, Capacity;
+  Size_T Size = 0, Capacity;
+
+  // The maximum size depends on size_type used.
+  static constexpr size_t SizeMax() {
+return std::numeric_limits::max();
+  }
 
   SmallVectorBase() = delete;
   SmallVectorBase(void *FirstEl, size_t TotalCapacity)
@@ -46,7 +59,39 @@
 
   /// This is an implementation of the grow() method which only works
   /// on POD-like data types and is out of line to reduce code duplication.
-  void grow_pod(void *FirstEl, size_t MinCapacity, size_t TSize);
+  /// This function will report a fatal error if it cannot increase capacity.
+  void grow_pod(void *FirstEl, size_t MinCapacity, size_t TSize) {
+// Ensure we can fit the new capacity.
+// This is only going to be applicable if the when the capacity is 32 bit.
+if (MinCapacity > SizeMax())
+  report_bad_alloc_error("SmallVector capacity overflow during allocation");
+
+// Ensure we can meet the guarantee of space for at least one more element.
+// The above check alone will not catch the case where grow is called with a
+// default MinCapacity of 0, but the current capacity cannot be increased.
+// This is only going to be applicable if the when the capacity is 32 bit.
+if (capacity() == SizeMax())
+  report_bad_alloc_error("SmallVector capacity unable to grow");
+
+// In theory 2*capacity can overflow if the capacity is 64 bit, but the
+// original capacity would never be large enough for this to be a problem.
+size_t NewCapacity = 2 * capacity() + 1; // Always grow.
+NewCapacity = std::min(std::max(NewCapacity, MinCapacity), SizeMax());
+
+void *NewElts;
+if (BeginX == FirstEl) {
+  NewElts = safe_malloc(NewCapacity * TSize);
+
+  // Copy t

[PATCH] D78019: HIP: Fix handling of denormal mode

[Matt Arsenault via Phabricator via cfe-commits]

arsenm closed this revision.
arsenm added a comment.

dc89a3efb43feedec04facfa2206de011d2606e7 



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

https://reviews.llvm.org/D78019



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

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

[Melanie Blower via Phabricator via cfe-commits]

mibintc updated this revision to Diff 257055.
mibintc added a comment.

Responded to @rjmccall 's review


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76384

Files:
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/ExprCXX.h
  clang/include/clang/AST/Stmt.h
  clang/include/clang/Basic/LangOptions.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/ExprCXX.cpp
  clang/lib/Analysis/BodyFarm.cpp
  clang/lib/Basic/LangOptions.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/CodeGen/CGObjC.cpp
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/lib/Frontend/Rewrite/RewriteModernObjC.cpp
  clang/lib/Frontend/Rewrite/RewriteObjC.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaPseudoObject.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp

Index: clang/lib/Serialization/ASTWriterStmt.cpp
===
--- clang/lib/Serialization/ASTWriterStmt.cpp
+++ clang/lib/Serialization/ASTWriterStmt.cpp
@@ -918,11 +918,16 @@
 
 void ASTStmtWriter::VisitBinaryOperator(BinaryOperator *E) {
   VisitExpr(E);
+  bool HasFPFeatures = E->hasStoredFPFeatures();
+  // Write this first for easy access when deserializing, as they affect the
+  // size of the UnaryOperator.
+  Record.push_back(HasFPFeatures);
+  Record.push_back(E->getOpcode()); // FIXME: stable encoding
   Record.AddStmt(E->getLHS());
   Record.AddStmt(E->getRHS());
-  Record.push_back(E->getOpcode()); // FIXME: stable encoding
   Record.AddSourceLocation(E->getOperatorLoc());
-  Record.push_back(E->getFPFeatures().getInt());
+  if (HasFPFeatures)
+Record.push_back(E->getStoredFPFeatures().getAsOpaqueInt());
   Code = serialization::EXPR_BINARY_OPERATOR;
 }
 
@@ -1513,7 +1518,7 @@
 void ASTStmtWriter::VisitCXXOperatorCallExpr(CXXOperatorCallExpr *E) {
   VisitCallExpr(E);
   Record.push_back(E->getOperator());
-  Record.push_back(E->getFPFeatures().getInt());
+  Record.push_back(E->getFPFeatures().getAsOpaqueInt());
   Record.AddSourceRange(E->Range);
   Code = serialization::EXPR_CXX_OPERATOR_CALL;
 }
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -3905,7 +3905,7 @@
 
 /// Write an FP_PRAGMA_OPTIONS block for the given FPOptions.
 void ASTWriter::WriteFPPragmaOptions(const FPOptions &Opts) {
-  RecordData::value_type Record[] = {Opts.getInt()};
+  RecordData::value_type Record[] = {Opts.getAsOpaqueInt()};
   Stream.EmitRecord(FP_PRAGMA_OPTIONS, Record);
 }
 
Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -1050,12 +1050,16 @@
 }
 
 void ASTStmtReader::VisitBinaryOperator(BinaryOperator *E) {
+  bool hasFP_Features;
+  BinaryOperator::Opcode opc;
   VisitExpr(E);
+  E->setHasStoredFPFeatures(hasFP_Features = Record.readInt());
+  E->setOpcode(opc = (BinaryOperator::Opcode)Record.readInt());
   E->setLHS(Record.readSubExpr());
   E->setRHS(Record.readSubExpr());
-  E->setOpcode((BinaryOperator::Opcode)Record.readInt());
   E->setOperatorLoc(readSourceLocation());
-  E->setFPFeatures(FPOptions(Record.readInt()));
+  if (hasFP_Features)
+E->setStoredFPFeatures(FPOptions(Record.readInt()));
 }
 
 void ASTStmtReader::VisitCompoundAssignOperator(CompoundAssignOperator *E) {
@@ -2937,11 +2941,13 @@
   break;
 
 case EXPR_BINARY_OPERATOR:
-  S = new (Context) BinaryOperator(Empty);
+  S = BinaryOperator::CreateEmpty(Context,
+  Record[ASTStmtReader::NumExprFields]);
   break;
 
 case EXPR_COMPOUND_ASSIGN_OPERATOR:
-  S = new (Context) CompoundAssignOperator(Empty);
+  S = CompoundAssignOperator::CreateEmpty(
+  Context, Record[ASTStmtReader::NumExprFields]);
   break;
 
 case EXPR_CONDITIONAL_OPERATOR:
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -10267,8 +10267,12 @@
   RHS.get() == E->getRHS())
 return E;
 
+  if (E->isCompoundAssignmentOp())
+// FPFeatures has already been established from trailing storage
+return getDerived().RebuildBinaryOperator(
+E->getOperatorLoc(), E->getOpcode(), LHS.get(), RHS.get());
   Sema::FPFeaturesStateRAII FPFeaturesState(getSema());
-  getSema().FPFeatures = E->getFPFeatures();
+  getSema().FPFeatures = E->getFPFeatures(getSema().getASTContext());
 
   return getDerived().RebuildBinaryOperator(E->getOp

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

[Melanie Blower via Phabricator via cfe-commits]

mibintc marked 3 inline comments as done.
mibintc added a comment.

Adding an inline reply for John. rebased the patch, also re-applied 
clang-format.  What do you think?




Comment at: clang/lib/Analysis/BodyFarm.cpp:120
+VK_RValue, OK_Ordinary, SourceLocation(),
+FPOptions::defaultWithoutTrailingStorage(C));
 }

rjmccall wrote:
> These construction sites don't seem like appropriate uses of 
> `defaultWithoutTrailingStorage`, which is an implementation detail of the AST 
> nodes.  The right behavior here is for all of these places to use the default 
> `FPOptions` from the language options, then let the AST figure out the right 
> way to store that.  That happens to have the same effect currently as 
> `defaultWithoutTrailingStorage`, but the latter should be able to change, 
> while the right behavior for these places will remain the same.
OK i changed these in BodyFarm to use the default constructor.  In Sema I 
changed them from defaultWithoutTrailingStorage to use Sema.FPFeatures which is 
the current setting combining pragma values + command line options. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76384



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

[PATCH] D77621: ADT: SmallVector size & capacity use word-size integers when elements are small.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Looks good to me at this point (I have some vague quandries about whether the 
report_fatal_error stuff could be improved/made more clear, but couldn't come 
up with an actionable suggestion so far) - @dexonsmith could you check this 
over and offer final approval?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77621



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

[PATCH] D77984: Make IRBuilder automatically set alignment on load/store/alloca.

[Eli Friedman via Phabricator via cfe-commits]

efriedma updated this revision to Diff 257059.
efriedma added a comment.

Address review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77984

Files:
  clang/test/CodeGen/arm_neon_intrinsics.c
  llvm/include/llvm/IR/IRBuilder.h
  llvm/test/Bindings/llvm-c/atomics.ll
  llvm/test/Bindings/llvm-c/echo.ll
  llvm/test/Bindings/llvm-c/memops.ll
  llvm/test/CodeGen/AMDGPU/widen_extending_scalar_loads.ll
  llvm/test/Instrumentation/AddressSanitizer/debug-info-alloca.ll
  llvm/test/Instrumentation/SanitizerCoverage/inline-8bit-counters.ll
  llvm/test/Instrumentation/SanitizerCoverage/inline-bool-flag.ll
  llvm/test/Transforms/ArgumentPromotion/dbg.ll
  llvm/test/Transforms/SROA/alignment.ll
  llvm/test/Transforms/SROA/basictest.ll
  llvm/test/Transforms/SROA/preserve-nonnull.ll
  polly/test/Isl/CodeGen/invariant_load_alias_metadata.ll
  polly/test/Isl/CodeGen/non-affine-phi-node-expansion-2.ll
  polly/test/Isl/CodeGen/partial_write_array.ll
  polly/test/Isl/CodeGen/partial_write_impossible_restriction.ll

Index: polly/test/Isl/CodeGen/partial_write_impossible_restriction.ll
===
--- polly/test/Isl/CodeGen/partial_write_impossible_restriction.ll
+++ polly/test/Isl/CodeGen/partial_write_impossible_restriction.ll
@@ -50,10 +50,10 @@
 
 ; CHECK-LABEL: polly.stmt.cond.false:
 ; CHECK: %polly.access..pn2 = getelementptr i32, i32* %.pn, i64 %polly.indvar
-; CHECK: store i32 %cond.in.sroa.speculate.load.cond.false_p_scalar_, i32* %polly.access..pn2, !alias.scope !0, !noalias !2
+; CHECK: store i32 %cond.in.sroa.speculate.load.cond.false_p_scalar_, i32* %polly.access..pn2, align 4, !alias.scope !0, !noalias !2
 ; CHECK: br label %polly.merge
 
 ; CHECK-LABEL: polly.stmt.cond.false11:
 ; CHECK: %polly.access..pn14 = getelementptr i32, i32* %.pn, i64 0
-; CHECK: store i32 %cond.in.sroa.speculate.load.cond.false_p_scalar_13, i32* %polly.access..pn14, !alias.scope !0, !noalias !2
+; CHECK: store i32 %cond.in.sroa.speculate.load.cond.false_p_scalar_13, i32* %polly.access..pn14, align 4, !alias.scope !0, !noalias !2
 ; CHECK: br label %polly.stmt.cond.end15
Index: polly/test/Isl/CodeGen/partial_write_array.ll
===
--- polly/test/Isl/CodeGen/partial_write_array.ll
+++ polly/test/Isl/CodeGen/partial_write_array.ll
@@ -38,7 +38,7 @@
 
 ; CHECK:  polly.stmt.body.Stmt_body_Write0.partial:
 ; CHECK-NEXT:   %polly.access.A = getelementptr double, double* %A, i64 0
-; CHECK-NEXT:   store double 4.20e+01, double* %polly.access.A, !alias.scope !0, !noalias !2
+; CHECK-NEXT:   store double 4.20e+01, double* %polly.access.A, align 8, !alias.scope !0, !noalias !2
 ; CHECK-NEXT:   br label %polly.stmt.body.cont
 
 ; CHECK:  polly.stmt.body.cont:
Index: polly/test/Isl/CodeGen/non-affine-phi-node-expansion-2.ll
===
--- polly/test/Isl/CodeGen/non-affine-phi-node-expansion-2.ll
+++ polly/test/Isl/CodeGen/non-affine-phi-node-expansion-2.ll
@@ -4,7 +4,7 @@
 
 
 ; CHECK: polly.stmt.bb3:   ; preds = %polly.stmt.bb3.entry
-; CHECK:   %tmp6_p_scalar_ = load double, double* %arg1{{[0-9]*}}, !alias.scope !0, !noalias !2
+; CHECK:   %tmp6_p_scalar_ = load double, double* %arg1{{[0-9]*}}, align 8, !alias.scope !0, !noalias !2
 ; CHECK:   %p_tmp7 = fadd double 1.00e+00, %tmp6_p_scalar_
 ; CHECK:   %p_tmp8 = fcmp olt double 1.40e+01, %p_tmp7
 ; CHECK:   br i1 %p_tmp8, label %polly.stmt.bb9, label %polly.stmt.bb10
Index: polly/test/Isl/CodeGen/invariant_load_alias_metadata.ll
===
--- polly/test/Isl/CodeGen/invariant_load_alias_metadata.ll
+++ polly/test/Isl/CodeGen/invariant_load_alias_metadata.ll
@@ -4,7 +4,7 @@
 ; This test case checks whether Polly generates alias metadata in case of
 ; the ublas gemm kernel and polly-invariant-load-hoisting.
 ;
-; CHECK: store float 4.20e+01, float* %polly.access.A.load, !alias.scope !3, !noalias !4
+; CHECK: store float 4.20e+01, float* %polly.access.A.load, align 4, !alias.scope !3, !noalias !4
 ;
 ; CHECK: !0 = distinct !{!0, !1, !"polly.alias.scope.MemRef_A"}
 ; CHECK-NEXT: !1 = distinct !{!1, !"polly.alias.scope.domain"}
Index: llvm/test/Transforms/SROA/preserve-nonnull.ll
===
--- llvm/test/Transforms/SROA/preserve-nonnull.ll
+++ llvm/test/Transforms/SROA/preserve-nonnull.ll
@@ -12,7 +12,7 @@
 ; CHECK-NEXT:%[[A:.*]] = alloca i8*
 ; CHECK-NEXT:%[[V_CAST:.*]] = bitcast i32* %v to i8*
 ; CHECK-NEXT:store i8* %[[V_CAST]], i8** %[[A]]
-; CHECK-NEXT:%[[LOAD:.*]] = load volatile i8*, i8** %[[A]], !nonnull !0
+; CHECK-NEXT:%[[LOAD:.*]] = load volatile i8*, i8** %[[A]], align 

[PATCH] D77461: [WIP][clang-tidy] Remove false positive in AvoidNonConstGlobalVariables

[Kim Viggedal via Phabricator via cfe-commits]

vingeldal updated this revision to Diff 257058.
vingeldal added a comment.

Modified the check to not report static variables in function scope.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77461

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp
@@ -231,7 +231,8 @@
 // CHECKING AGAINST FALSE POSITIVES INSIDE FUNCTION SCOPE /
 int main() {
   for (int i = 0; i < 3; ++i) {
+static int staticNonConstLoopVariable = 42;
 int nonConstLoopVariable = 42;
-nonConstInt = nonConstLoopVariable + i;
+nonConstInt = nonConstLoopVariable + i + staticNonConstLoopVariable;
   }
 }
Index: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
===
--- 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
+++ 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
@@ -45,11 +45,14 @@
 
   if (const auto *Variable =
   Result.Nodes.getNodeAs("non-const_variable")) {
-diag(Variable->getLocation(), "variable %0 is non-const and globally "
-  "accessible, consider making it const")
-<< Variable; // FIXME: Add fix-it hint to Variable
-// Don't return early, a non-const variable may also be a pointer or
-// reference to non-const data.
+  // To avoid warning about local variables with global storage, like 
static variables in a function.
+  if (!Variable->isLocalVarDecl()) {
+  diag(Variable->getLocation(), "variable %0 is non-const and globally 
"
+"accessible, consider making it const")
+  << Variable; // FIXME: Add fix-it hint to Variable
+  // Don't return early, a non-const variable may also be a pointer or
+  // reference to non-const data.
+  }
   }
 
   if (const auto *VD =


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp
@@ -231,7 +231,8 @@
 // CHECKING AGAINST FALSE POSITIVES INSIDE FUNCTION SCOPE /
 int main() {
   for (int i = 0; i < 3; ++i) {
+static int staticNonConstLoopVariable = 42;
 int nonConstLoopVariable = 42;
-nonConstInt = nonConstLoopVariable + i;
+nonConstInt = nonConstLoopVariable + i + staticNonConstLoopVariable;
   }
 }
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
@@ -45,11 +45,14 @@
 
   if (const auto *Variable =
   Result.Nodes.getNodeAs("non-const_variable")) {
-diag(Variable->getLocation(), "variable %0 is non-const and globally "
-  "accessible, consider making it const")
-<< Variable; // FIXME: Add fix-it hint to Variable
-// Don't return early, a non-const variable may also be a pointer or
-// reference to non-const data.
+  // To avoid warning about local variables with global storage, like static variables in a function.
+  if (!Variable->isLocalVarDecl()) {
+  diag(Variable->getLocation(), "variable %0 is non-const and globally "
+"accessible, consider making it const")
+  << Variable; // FIXME: Add fix-it hint to Variable
+  // Don't return early, a non-const variable may also be a pointer or
+  // reference to non-const data.
+  }
   }
 
   if (const auto *VD =
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77461: [WIP][clang-tidy] Remove false positive in AvoidNonConstGlobalVariables

[Kim Viggedal via Phabricator via cfe-commits]

vingeldal updated this revision to Diff 257060.
vingeldal added a comment.

Ran automatic formatting on the patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77461

Files:
  
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp
@@ -231,7 +231,8 @@
 // CHECKING AGAINST FALSE POSITIVES INSIDE FUNCTION SCOPE /
 int main() {
   for (int i = 0; i < 3; ++i) {
+static int staticNonConstLoopVariable = 42;
 int nonConstLoopVariable = 42;
-nonConstInt = nonConstLoopVariable + i;
+nonConstInt = nonConstLoopVariable + i + staticNonConstLoopVariable;
   }
 }
Index: 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
===
--- 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
+++ 
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
@@ -45,11 +45,15 @@
 
   if (const auto *Variable =
   Result.Nodes.getNodeAs("non-const_variable")) {
-diag(Variable->getLocation(), "variable %0 is non-const and globally "
-  "accessible, consider making it const")
-<< Variable; // FIXME: Add fix-it hint to Variable
-// Don't return early, a non-const variable may also be a pointer or
-// reference to non-const data.
+// To avoid warning about local variables with global storage, like static
+// variables in a function.
+if (!Variable->isLocalVarDecl()) {
+  diag(Variable->getLocation(), "variable %0 is non-const and globally "
+"accessible, consider making it const")
+  << Variable; // FIXME: Add fix-it hint to Variable
+  // Don't return early, a non-const variable may also be a pointer or
+  // reference to non-const data.
+}
   }
 
   if (const auto *VD =


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp
@@ -231,7 +231,8 @@
 // CHECKING AGAINST FALSE POSITIVES INSIDE FUNCTION SCOPE /
 int main() {
   for (int i = 0; i < 3; ++i) {
+static int staticNonConstLoopVariable = 42;
 int nonConstLoopVariable = 42;
-nonConstInt = nonConstLoopVariable + i;
+nonConstInt = nonConstLoopVariable + i + staticNonConstLoopVariable;
   }
 }
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
@@ -45,11 +45,15 @@
 
   if (const auto *Variable =
   Result.Nodes.getNodeAs("non-const_variable")) {
-diag(Variable->getLocation(), "variable %0 is non-const and globally "
-  "accessible, consider making it const")
-<< Variable; // FIXME: Add fix-it hint to Variable
-// Don't return early, a non-const variable may also be a pointer or
-// reference to non-const data.
+// To avoid warning about local variables with global storage, like static
+// variables in a function.
+if (!Variable->isLocalVarDecl()) {
+  diag(Variable->getLocation(), "variable %0 is non-const and globally "
+"accessible, consider making it const")
+  << Variable; // FIXME: Add fix-it hint to Variable
+  // Don't return early, a non-const variable may also be a pointer or
+  // reference to non-const data.
+}
   }
 
   if (const auto *VD =
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits