[PATCH] D37449: [X86][AVX512] _mm512_stream_load_si512 should take a void const* argument (PR33977)

[Elena Demikhovsky via Phabricator via cfe-commits]

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

LGTM


Repository:
  rL LLVM

https://reviews.llvm.org/D37449



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

Re: r311857 - Emit static constexpr member as available_externally definition

[Mehdi AMINI via cfe-commits]

Hey Hans,

Just a heads up that I fixed the issue and reapplies the commit in r312512.
Let me know if any new issues occur! (and feel free to revert of course).

Thanks,

-- 
Mehdi

2017-08-28 14:18 GMT-07:00 Mehdi AMINI :

> Sorry for the inconvenience!
>
> I will follow-up in the PR.
>
> --
> Mehdi
>
>
> 2017-08-28 12:42 GMT-07:00 Hans Wennborg :
>
>> I reverted this in r311898 as it caused Chromium builds to fail with
>> an assertion; see PR34348.
>>
>> On Sun, Aug 27, 2017 at 1:24 PM, Mehdi Amini via cfe-commits
>>  wrote:
>> > Author: mehdi_amini
>> > Date: Sun Aug 27 13:24:09 2017
>> > New Revision: 311857
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=311857=rev
>> > Log:
>> > Emit static constexpr member as available_externally definition
>> >
>> > By exposing the constant initializer, the optimizer can fold many
>> > of these constructs.
>> >
>> > Differential Revision: https://reviews.llvm.org/D34992
>> >
>> > Added:
>> > cfe/trunk/test/CodeGenCXX/cxx11-extern-constexpr.cpp
>> > Modified:
>> > cfe/trunk/lib/CodeGen/CodeGenModule.cpp
>> >
>> > Modified: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
>> > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/Co
>> deGenModule.cpp?rev=311857=311856=311857=diff
>> > 
>> ==
>> > --- cfe/trunk/lib/CodeGen/CodeGenModule.cpp (original)
>> > +++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp Sun Aug 27 13:24:09 2017
>> > @@ -2437,6 +2437,28 @@ CodeGenModule::GetOrCreateLLVMGlobal(Str
>> >  D->getType().isConstant(Context) &&
>> >  isExternallyVisible(D->getLinkageAndVisibility().getLinkage(
>> )))
>> >GV->setSection(".cp.rodata");
>> > +
>> > +// Check if we a have a const declaration with an initializer, we
>> may be
>> > +// able to emit it as available_externally to expose it's value to
>> the
>> > +// optimizer.
>> > +if (Context.getLangOpts().CPlusPlus && GV->hasExternalLinkage() &&
>> > +D->getType().isConstQualified() && !GV->hasInitializer() &&
>> > +!D->hasDefinition() && D->hasInit() &&
>> !D->hasAttr()) {
>> > +  const auto *Record =
>> > +  Context.getBaseElementType(D->getType())->getAsCXXRecordDecl
>> ();
>> > +  bool HasMutableFields = Record && Record->hasMutableFields();
>> > +  if (!HasMutableFields) {
>> > +const VarDecl *InitDecl;
>> > +const Expr *InitExpr = D->getAnyInitializer(InitDecl);
>> > +if (InitExpr) {
>> > +  GV->setConstant(true);
>> > +  GV->setLinkage(llvm::GlobalValue::AvailableExternallyLinkage
>> );
>> > +  ConstantEmitter emitter(*this);
>> > +  GV->setInitializer(emitter.tryEmitForInitializer(*InitDecl))
>> ;
>> > +  emitter.finalize(GV);
>> > +}
>> > +  }
>> > +}
>> >}
>> >
>> >auto ExpectedAS =
>> >
>> > Added: cfe/trunk/test/CodeGenCXX/cxx11-extern-constexpr.cpp
>> > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCX
>> X/cxx11-extern-constexpr.cpp?rev=311857=auto
>> > 
>> ==
>> > --- cfe/trunk/test/CodeGenCXX/cxx11-extern-constexpr.cpp (added)
>> > +++ cfe/trunk/test/CodeGenCXX/cxx11-extern-constexpr.cpp Sun Aug 27
>> 13:24:09 2017
>> > @@ -0,0 +1,55 @@
>> > +// RUN: %clang_cc1 -std=c++11 %s -emit-llvm -o - -triple
>> x86_64-linux-gnu | FileCheck %s --check-prefix=CHECK --check-prefix=CXX11
>> > +// RUN: %clang_cc1 -std=c++1z %s -emit-llvm -o - -triple
>> x86_64-linux-gnu | FileCheck %s --check-prefix=CHECK --check-prefix=CXX17
>> > +
>> > +struct A {
>> > +  static const int Foo = 123;
>> > +};
>> > +// CHECK: @_ZN1A3FooE = constant i32 123, align 4
>> > +const int *p = ::Foo; // emit available_externally
>> > +const int A::Foo;   // convert to full definition
>> > +
>> > +struct Bar {
>> > +  int b;
>> > +};
>> > +
>> > +struct MutableBar {
>> > +  mutable int b;
>> > +};
>> > +
>> > +struct Foo {
>> > +  // CXX11: @_ZN3Foo21ConstexprStaticMemberE = available_externally
>> constant i32 42,
>> > +  // CXX17: @_ZN3Foo21ConstexprStaticMemberE = linkonce_odr constant
>> i32 42,
>> > +  static constexpr int ConstexprStaticMember = 42;
>> > +  // CHECK: @_ZN3Foo17ConstStaticMemberE = available_externally
>> constant i32 43,
>> > +  static const int ConstStaticMember = 43;
>> > +
>> > +  // CXX11: @_ZN3Foo23ConstStaticStructMemberE = available_externally
>> constant %struct.Bar { i32 44 },
>> > +  // CXX17: @_ZN3Foo23ConstStaticStructMemberE = linkonce_odr
>> constant %struct.Bar { i32 44 },
>> > +  static constexpr Bar ConstStaticStructMember = {44};
>> > +
>> > +  // CXX11: @_ZN3Foo34ConstexprStaticMutableStructMemberE = external
>> global %struct.MutableBar,
>> > +  // CXX17: @_ZN3Foo34ConstexprStaticMutableStructMemberE =
>> linkonce_odr global %struct.MutableBar { i32 45 },
>> > +  static constexpr 

r312512 - Emit static constexpr member as available_externally definition

[Mehdi Amini via cfe-commits]

Author: mehdi_amini
Date: Mon Sep  4 20:58:35 2017
New Revision: 312512

URL: http://llvm.org/viewvc/llvm-project?rev=312512=rev
Log:
Emit static constexpr member as available_externally definition

By exposing the constant initializer, the optimizer can fold many
of these constructs.

This is a recommit of r311857 that was reverted in r311898 because
an assert was hit when building Chromium.
We have to take into account that the GlobalVariable may be first
created with a different type than the initializer. This can
happen for example when the variable is a struct with tail padding
while the initializer does not have padding. In such case, the
variable needs to be destroyed an replaced with a new one with the
type of the initializer.

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

Added:
cfe/trunk/test/CodeGenCXX/cxx11-extern-constexpr.cpp
Modified:
cfe/trunk/lib/CodeGen/CodeGenModule.cpp

Modified: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenModule.cpp?rev=312512=312511=312512=diff
==
--- cfe/trunk/lib/CodeGen/CodeGenModule.cpp (original)
+++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp Mon Sep  4 20:58:35 2017
@@ -2437,6 +2437,48 @@ CodeGenModule::GetOrCreateLLVMGlobal(Str
 D->getType().isConstant(Context) &&
 isExternallyVisible(D->getLinkageAndVisibility().getLinkage()))
   GV->setSection(".cp.rodata");
+
+// Check if we a have a const declaration with an initializer, we may be
+// able to emit it as available_externally to expose it's value to the
+// optimizer.
+if (Context.getLangOpts().CPlusPlus && GV->hasExternalLinkage() &&
+D->getType().isConstQualified() && !GV->hasInitializer() &&
+!D->hasDefinition() && D->hasInit() && !D->hasAttr()) {
+  const auto *Record =
+  Context.getBaseElementType(D->getType())->getAsCXXRecordDecl();
+  bool HasMutableFields = Record && Record->hasMutableFields();
+  if (!HasMutableFields) {
+const VarDecl *InitDecl;
+const Expr *InitExpr = D->getAnyInitializer(InitDecl);
+if (InitExpr) {
+  ConstantEmitter emitter(*this);
+  llvm::Constant *Init = emitter.tryEmitForInitializer(*InitDecl);
+  if (Init) {
+auto *InitType = Init->getType();
+if (GV->getType()->getElementType() != InitType) {
+  // The type of the initializer does not match the definition.
+  // This happens when an initializer has a different type from
+  // the type of the global (because of padding at the end of a
+  // structure for instance).
+  GV->setName(StringRef());
+  // Make a new global with the correct type, this is now 
guaranteed
+  // to work.
+  auto *NewGV = cast(
+  GetAddrOfGlobalVar(D, InitType, IsForDefinition));
+
+  // Erase the old global, since it is no longer used.
+  cast(GV)->eraseFromParent();
+  GV = NewGV;
+} else {
+  GV->setInitializer(Init);
+  GV->setConstant(true);
+  GV->setLinkage(llvm::GlobalValue::AvailableExternallyLinkage);
+}
+emitter.finalize(GV);
+  }
+}
+  }
+}
   }
 
   auto ExpectedAS =

Added: cfe/trunk/test/CodeGenCXX/cxx11-extern-constexpr.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/cxx11-extern-constexpr.cpp?rev=312512=auto
==
--- cfe/trunk/test/CodeGenCXX/cxx11-extern-constexpr.cpp (added)
+++ cfe/trunk/test/CodeGenCXX/cxx11-extern-constexpr.cpp Mon Sep  4 20:58:35 
2017
@@ -0,0 +1,68 @@
+// RUN: %clang_cc1 -std=c++11 %s -emit-llvm -o - -triple x86_64-linux-gnu | 
FileCheck %s --check-prefix=CHECK --check-prefix=CXX11
+// RUN: %clang_cc1 -std=c++1z %s -emit-llvm -o - -triple x86_64-linux-gnu | 
FileCheck %s --check-prefix=CHECK --check-prefix=CXX17
+
+struct A {
+  static const int Foo = 123;
+};
+// CHECK: @_ZN1A3FooE = constant i32 123, align 4
+const int *p = ::Foo; // emit available_externally
+const int A::Foo;   // convert to full definition
+
+struct PODWithInit {
+  int g = 42;
+  char h = 43;
+};
+struct CreatePOD {
+  // Deferred initialization of the structure here requires changing
+  // the type of the global variable: the initializer list does not include
+  // the tail padding.
+  // CXX11: @_ZN9CreatePOD3podE = available_externally constant { i32, i8 } { 
i32 42, i8 43 },
+  static constexpr PODWithInit pod{};
+};
+const int *p_pod = ::pod.g;
+
+struct Bar {
+  int b;
+};
+
+struct MutableBar {
+  mutable int b;
+};
+
+struct Foo {
+  // CXX11: @_ZN3Foo21ConstexprStaticMemberE = available_externally constant 
i32 42,
+  // CXX17: @_ZN3Foo21ConstexprStaticMemberE = linkonce_odr constant i32 42,

[PATCH] D37454: [coroutines] Make sure auto return type of await_resume is properly handled

[Gor Nishanov via Phabricator via cfe-commits]

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



Comment at: lib/Sema/SemaCoroutine.cpp:451
   if (RetType->isReferenceType() ||
   (AdjRetType != S.Context.BoolTy && AdjRetType != S.Context.VoidTy)) {
 S.Diag(AwaitSuspend->getCalleeDecl()->getLocation(),

rsmith wrote:
> Don't desugar the type; instead, use `AdjRetType->isBooleanType() || 
> AdjRetType->isVoidType()` here. You can also delete the 
> `getUnqualifiedType()` call above if you do this, and just use `RetType` 
> here, since `->isBooleanType()` on `const bool` returns `true`.
Cool! Much shorter and prettier. Thank you!


https://reviews.llvm.org/D37454



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

[PATCH] D37454: [coroutines] Make sure auto return type of await_resume is properly handled

[Gor Nishanov via Phabricator via cfe-commits]

GorNishanov updated this revision to Diff 113800.
GorNishanov retitled this revision from "[coroutines] desugar auto type of 
await_resume before checking whether it is void or bool" to "[coroutines] Make 
sure auto return type of await_resume is properly handled".
GorNishanov added a comment.

use isVoidType() and isBooleantType() instead of cruft that used to be there 
before


https://reviews.llvm.org/D37454

Files:
  lib/Sema/SemaCoroutine.cpp
  test/SemaCXX/coroutines.cpp


Index: test/SemaCXX/coroutines.cpp
===
--- test/SemaCXX/coroutines.cpp
+++ test/SemaCXX/coroutines.cpp
@@ -66,6 +66,12 @@
   void await_resume() {}
 };
 
+struct auto_await_suspend {
+  bool await_ready();
+  template  auto await_suspend(F) {}
+  void await_resume();
+};
+
 struct DummyVoidTag {};
 DummyVoidTag no_specialization() { // expected-error {{this function cannot be 
a coroutine: 'std::experimental::coroutine_traits' has no member 
named 'promise_type'}}
   co_await a;
@@ -159,6 +165,10 @@
   co_yield yield; // expected-error {{no member named 'await_ready' in 
'not_awaitable'}}
 }
 
+void check_auto_await_suspend() {
+  co_await auto_await_suspend{}; // OK
+}
+
 void coreturn(int n) {
   co_await a;
   if (n == 0)
Index: lib/Sema/SemaCoroutine.cpp
===
--- lib/Sema/SemaCoroutine.cpp
+++ lib/Sema/SemaCoroutine.cpp
@@ -438,14 +438,14 @@
 //   - await-suspend is the expression e.await_suspend(h), which shall be
 // a prvalue of type void or bool.
 QualType RetType = AwaitSuspend->getCallReturnType(S.Context);
+
 // Experimental support for coroutine_handle returning await_suspend.
 if (Expr *TailCallSuspend = maybeTailCall(S, RetType, AwaitSuspend, Loc))
   Calls.Results[ACT::ACT_Suspend] = TailCallSuspend;
 else {
   // non-class prvalues always have cv-unqualified types
-  QualType AdjRetType = RetType.getUnqualifiedType();
   if (RetType->isReferenceType() ||
-  (AdjRetType != S.Context.BoolTy && AdjRetType != S.Context.VoidTy)) {
+  (!RetType->isBooleanType() && !RetType->isVoidType())) {
 S.Diag(AwaitSuspend->getCalleeDecl()->getLocation(),
diag::err_await_suspend_invalid_return_type)
 << RetType;


Index: test/SemaCXX/coroutines.cpp
===
--- test/SemaCXX/coroutines.cpp
+++ test/SemaCXX/coroutines.cpp
@@ -66,6 +66,12 @@
   void await_resume() {}
 };
 
+struct auto_await_suspend {
+  bool await_ready();
+  template  auto await_suspend(F) {}
+  void await_resume();
+};
+
 struct DummyVoidTag {};
 DummyVoidTag no_specialization() { // expected-error {{this function cannot be a coroutine: 'std::experimental::coroutine_traits' has no member named 'promise_type'}}
   co_await a;
@@ -159,6 +165,10 @@
   co_yield yield; // expected-error {{no member named 'await_ready' in 'not_awaitable'}}
 }
 
+void check_auto_await_suspend() {
+  co_await auto_await_suspend{}; // OK
+}
+
 void coreturn(int n) {
   co_await a;
   if (n == 0)
Index: lib/Sema/SemaCoroutine.cpp
===
--- lib/Sema/SemaCoroutine.cpp
+++ lib/Sema/SemaCoroutine.cpp
@@ -438,14 +438,14 @@
 //   - await-suspend is the expression e.await_suspend(h), which shall be
 // a prvalue of type void or bool.
 QualType RetType = AwaitSuspend->getCallReturnType(S.Context);
+
 // Experimental support for coroutine_handle returning await_suspend.
 if (Expr *TailCallSuspend = maybeTailCall(S, RetType, AwaitSuspend, Loc))
   Calls.Results[ACT::ACT_Suspend] = TailCallSuspend;
 else {
   // non-class prvalues always have cv-unqualified types
-  QualType AdjRetType = RetType.getUnqualifiedType();
   if (RetType->isReferenceType() ||
-  (AdjRetType != S.Context.BoolTy && AdjRetType != S.Context.VoidTy)) {
+  (!RetType->isBooleanType() && !RetType->isVoidType())) {
 S.Diag(AwaitSuspend->getCalleeDecl()->getLocation(),
diag::err_await_suspend_invalid_return_type)
 << RetType;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r312506 - Always allocate room for a ModuleDecl on the TranslationUnitDecl.

[Richard Smith via cfe-commits]

Author: rsmith
Date: Mon Sep  4 17:50:19 2017
New Revision: 312506

URL: http://llvm.org/viewvc/llvm-project?rev=312506=rev
Log:
Always allocate room for a ModuleDecl on the TranslationUnitDecl.

Sometimes we create the ASTContext and thus the TranslationUnitDecl before we 
know the LangOptions. This should fix the asan buildbot failures after r312467.

Modified:
cfe/trunk/lib/AST/DeclBase.cpp

Modified: cfe/trunk/lib/AST/DeclBase.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/DeclBase.cpp?rev=312506=312505=312506=diff
==
--- cfe/trunk/lib/AST/DeclBase.cpp (original)
+++ cfe/trunk/lib/AST/DeclBase.cpp Mon Sep  4 17:50:19 2017
@@ -74,8 +74,9 @@ void *Decl::operator new(std::size_t Siz
  DeclContext *Parent, std::size_t Extra) {
   assert(!Parent || >getParentASTContext() == );
   // With local visibility enabled, we track the owning module even for local
-  // declarations.
-  if (Ctx.getLangOpts().trackLocalOwningModule()) {
+  // declarations. We create the TU decl early and may not yet know what the
+  // LangOpts are, so conservatively allocate the storage.
+  if (Ctx.getLangOpts().trackLocalOwningModule() || !Parent) {
 // Ensure required alignment of the resulting object by adding extra
 // padding at the start if required.
 size_t ExtraAlign =


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

[PATCH] D37454: [coroutines] desugar auto type of await_resume before checking whether it is void or bool

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

rsmith added inline comments.



Comment at: lib/Sema/SemaCoroutine.cpp:451
   if (RetType->isReferenceType() ||
   (AdjRetType != S.Context.BoolTy && AdjRetType != S.Context.VoidTy)) {
 S.Diag(AwaitSuspend->getCalleeDecl()->getLocation(),

Don't desugar the type; instead, use `AdjRetType->isBooleanType() || 
AdjRetType->isVoidType()` here. You can also delete the `getUnqualifiedType()` 
call above if you do this, and just use `RetType` here, since 
`->isBooleanType()` on `const bool` returns `true`.


https://reviews.llvm.org/D37454



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

[PATCH] D37454: [coroutines] desugar auto type of await_resume before checking whether it is void or bool

[Gor Nishanov via Phabricator via cfe-commits]

GorNishanov created this revision.

https://reviews.llvm.org/D37454

Files:
  lib/Sema/SemaCoroutine.cpp
  test/SemaCXX/coroutines.cpp


Index: test/SemaCXX/coroutines.cpp
===
--- test/SemaCXX/coroutines.cpp
+++ test/SemaCXX/coroutines.cpp
@@ -66,6 +66,12 @@
   void await_resume() {}
 };
 
+struct auto_await_suspend {
+  bool await_ready();
+  template  auto await_suspend(F) {}
+  void await_resume();
+};
+
 struct DummyVoidTag {};
 DummyVoidTag no_specialization() { // expected-error {{this function cannot be 
a coroutine: 'std::experimental::coroutine_traits' has no member 
named 'promise_type'}}
   co_await a;
@@ -159,6 +165,10 @@
   co_yield yield; // expected-error {{no member named 'await_ready' in 
'not_awaitable'}}
 }
 
+void check_auto_await_suspend() {
+  co_await auto_await_suspend{}; // OK
+}
+
 void coreturn(int n) {
   co_await a;
   if (n == 0)
Index: lib/Sema/SemaCoroutine.cpp
===
--- lib/Sema/SemaCoroutine.cpp
+++ lib/Sema/SemaCoroutine.cpp
@@ -444,6 +444,9 @@
 else {
   // non-class prvalues always have cv-unqualified types
   QualType AdjRetType = RetType.getUnqualifiedType();
+  if (auto *AT = dyn_cast(AdjRetType))
+AdjRetType = AT->desugar();
+
   if (RetType->isReferenceType() ||
   (AdjRetType != S.Context.BoolTy && AdjRetType != S.Context.VoidTy)) {
 S.Diag(AwaitSuspend->getCalleeDecl()->getLocation(),


Index: test/SemaCXX/coroutines.cpp
===
--- test/SemaCXX/coroutines.cpp
+++ test/SemaCXX/coroutines.cpp
@@ -66,6 +66,12 @@
   void await_resume() {}
 };
 
+struct auto_await_suspend {
+  bool await_ready();
+  template  auto await_suspend(F) {}
+  void await_resume();
+};
+
 struct DummyVoidTag {};
 DummyVoidTag no_specialization() { // expected-error {{this function cannot be a coroutine: 'std::experimental::coroutine_traits' has no member named 'promise_type'}}
   co_await a;
@@ -159,6 +165,10 @@
   co_yield yield; // expected-error {{no member named 'await_ready' in 'not_awaitable'}}
 }
 
+void check_auto_await_suspend() {
+  co_await auto_await_suspend{}; // OK
+}
+
 void coreturn(int n) {
   co_await a;
   if (n == 0)
Index: lib/Sema/SemaCoroutine.cpp
===
--- lib/Sema/SemaCoroutine.cpp
+++ lib/Sema/SemaCoroutine.cpp
@@ -444,6 +444,9 @@
 else {
   // non-class prvalues always have cv-unqualified types
   QualType AdjRetType = RetType.getUnqualifiedType();
+  if (auto *AT = dyn_cast(AdjRetType))
+AdjRetType = AT->desugar();
+
   if (RetType->isReferenceType() ||
   (AdjRetType != S.Context.BoolTy && AdjRetType != S.Context.VoidTy)) {
 S.Diag(AwaitSuspend->getCalleeDecl()->getLocation(),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37386: [AMDGPU] Implement infrastructure to set options in AMDGPUToolChain

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl accepted this revision.
yaxunl added a comment.

LGTM. Thanks.


https://reviews.llvm.org/D37386



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

[PATCH] D37133: [libc++] Handle object files named *.obj in merge_archives.py

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In https://reviews.llvm.org/D37133#860563, @compnerd wrote:

> `$` should give you the `.o` or `.obj` 
> files used to construct the library.


I guess that would only work if they're built all as part of the same CMake 
invocation? In the setup where I'm trying this (for bootstrapping a MinGW 
environment), they're built standalone, with the libcxx cmake invocation 
pointing to the built libcxxabi static library.


https://reviews.llvm.org/D37133



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

[PATCH] D37133: [libc++] Handle object files named *.obj in merge_archives.py

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

`$` should give you the `.o` or `.obj` 
files used to construct the library.


https://reviews.llvm.org/D37133



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

[PATCH] D37133: [libc++] Handle object files named *.obj in merge_archives.py

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

Do you have any pointers on how to do it with CMake?


https://reviews.llvm.org/D37133



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

[libcxx] r312498 - Add MINGW_LIBRARIES to the linker flags

[Martin Storsjo via cfe-commits]

Author: mstorsjo
Date: Mon Sep  4 12:46:53 2017
New Revision: 312498

URL: http://llvm.org/viewvc/llvm-project?rev=312498=rev
Log:
Add MINGW_LIBRARIES to the linker flags

This is essential when building with -nodefaultlibs.

This is similar to what already is done in libcxxabi in SVN r302760.

Differential revision: https://reviews.llvm.org/D37207

Modified:
libcxx/trunk/lib/CMakeLists.txt

Modified: libcxx/trunk/lib/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/libcxx/trunk/lib/CMakeLists.txt?rev=312498=312497=312498=diff
==
--- libcxx/trunk/lib/CMakeLists.txt (original)
+++ libcxx/trunk/lib/CMakeLists.txt Mon Sep  4 12:46:53 2017
@@ -91,6 +91,7 @@ else()
   add_library_flags_if(LIBCXX_HAS_GCC_S_LIB gcc_s)
 endif()
 add_library_flags_if(LIBCXX_HAVE_CXX_ATOMICS_WITH_LIB atomic)
+add_library_flags_if(MINGW "${MINGW_LIBRARIES}")
 
 # Add the unwinder library.
 if (LIBCXXABI_USE_LLVM_UNWINDER)


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

[PATCH] D37207: [libc++] Add MINGW_LIBRARIES to the linker flags

[Martin Storsjö via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL312498: Add MINGW_LIBRARIES to the linker flags (authored by 
mstorsjo).

Changed prior to commit:
  https://reviews.llvm.org/D37207?vs=112873=113787#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D37207

Files:
  libcxx/trunk/lib/CMakeLists.txt


Index: libcxx/trunk/lib/CMakeLists.txt
===
--- libcxx/trunk/lib/CMakeLists.txt
+++ libcxx/trunk/lib/CMakeLists.txt
@@ -91,6 +91,7 @@
   add_library_flags_if(LIBCXX_HAS_GCC_S_LIB gcc_s)
 endif()
 add_library_flags_if(LIBCXX_HAVE_CXX_ATOMICS_WITH_LIB atomic)
+add_library_flags_if(MINGW "${MINGW_LIBRARIES}")
 
 # Add the unwinder library.
 if (LIBCXXABI_USE_LLVM_UNWINDER)


Index: libcxx/trunk/lib/CMakeLists.txt
===
--- libcxx/trunk/lib/CMakeLists.txt
+++ libcxx/trunk/lib/CMakeLists.txt
@@ -91,6 +91,7 @@
   add_library_flags_if(LIBCXX_HAS_GCC_S_LIB gcc_s)
 endif()
 add_library_flags_if(LIBCXX_HAVE_CXX_ATOMICS_WITH_LIB atomic)
+add_library_flags_if(MINGW "${MINGW_LIBRARIES}")
 
 # Add the unwinder library.
 if (LIBCXXABI_USE_LLVM_UNWINDER)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37134: [libc++] Rerun ranlib manually after merging the static libraries

[Saleem Abdulrasool via Phabricator via cfe-commits]

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

If we handle this in CMake, this will be unnecessary.


https://reviews.llvm.org/D37134



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

[PATCH] D37133: [libc++] Handle object files named *.obj in merge_archives.py

[Saleem Abdulrasool via Phabricator via cfe-commits]

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

I think we should avoid this logic entirely and use CMake to do this.


https://reviews.llvm.org/D37133



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

[PATCH] D37416: Use the VFS from the CompilerInvocation by default

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev added a comment.

You should probably update the code creating the vfs before the call to 
`createFileManager` in `lib/Frontend/FrontendAction.cpp`.


Repository:
  rL LLVM

https://reviews.llvm.org/D37416



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

[PATCH] D37449: [X86][AVX512] _mm512_stream_load_si512 should take a void const* argument (PR33977)

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon created this revision.

Based off the Intel Intrinsics guide, we should expect a void const* argument.

Prevents 'passing 'const void *' to parameter of type 'void *' discards 
qualifiers' warnings.


Repository:
  rL LLVM

https://reviews.llvm.org/D37449

Files:
  lib/Headers/avx512fintrin.h
  test/CodeGen/avx512f-builtins.c


Index: test/CodeGen/avx512f-builtins.c
===
--- test/CodeGen/avx512f-builtins.c
+++ test/CodeGen/avx512f-builtins.c
@@ -6258,6 +6258,12 @@
   return _mm512_stream_load_si512(__P); 
 }
 
+__m512i test_mm512_stream_load_si512_const(void const *__P) {
+  // CHECK-LABEL: @test_mm512_stream_load_si512_const
+  // CHECK: load <8 x i64>, <8 x i64>* %{{.*}}, align 64, !nontemporal
+  return _mm512_stream_load_si512(__P); 
+}
+
 void test_mm512_stream_pd(double *__P, __m512d __A) {
   // CHECK-LABEL: @test_mm512_stream_pd
   // CHECK: store <8 x double> %{{.*}}, <8 x double>* %{{.*}}, align 64, 
!nontemporal
Index: lib/Headers/avx512fintrin.h
===
--- lib/Headers/avx512fintrin.h
+++ lib/Headers/avx512fintrin.h
@@ -9040,7 +9040,7 @@
 }
 
 static __inline__ __m512i __DEFAULT_FN_ATTRS
-_mm512_stream_load_si512 (void *__P)
+_mm512_stream_load_si512 (void const *__P)
 {
   typedef __v8di __v8di_aligned __attribute__((aligned(64)));
   return (__m512i) __builtin_nontemporal_load((const __v8di_aligned *)__P);


Index: test/CodeGen/avx512f-builtins.c
===
--- test/CodeGen/avx512f-builtins.c
+++ test/CodeGen/avx512f-builtins.c
@@ -6258,6 +6258,12 @@
   return _mm512_stream_load_si512(__P); 
 }
 
+__m512i test_mm512_stream_load_si512_const(void const *__P) {
+  // CHECK-LABEL: @test_mm512_stream_load_si512_const
+  // CHECK: load <8 x i64>, <8 x i64>* %{{.*}}, align 64, !nontemporal
+  return _mm512_stream_load_si512(__P); 
+}
+
 void test_mm512_stream_pd(double *__P, __m512d __A) {
   // CHECK-LABEL: @test_mm512_stream_pd
   // CHECK: store <8 x double> %{{.*}}, <8 x double>* %{{.*}}, align 64, !nontemporal
Index: lib/Headers/avx512fintrin.h
===
--- lib/Headers/avx512fintrin.h
+++ lib/Headers/avx512fintrin.h
@@ -9040,7 +9040,7 @@
 }
 
 static __inline__ __m512i __DEFAULT_FN_ATTRS
-_mm512_stream_load_si512 (void *__P)
+_mm512_stream_load_si512 (void const *__P)
 {
   typedef __v8di __v8di_aligned __attribute__((aligned(64)));
   return (__m512i) __builtin_nontemporal_load((const __v8di_aligned *)__P);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37448: Fix cast assertion on MS inline assembly with vector spills (PR34021)

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon created this revision.

I can add the other test cases from PR34021 if required?


Repository:
  rL LLVM

https://reviews.llvm.org/D37448

Files:
  lib/CodeGen/CGStmt.cpp
  test/CodeGen/pr34021.c


Index: test/CodeGen/pr34021.c
===
--- test/CodeGen/pr34021.c
+++ test/CodeGen/pr34021.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fms-extensions %s -triple=i686-unknown-unknown -emit-llvm 
-o -
+// RUN: %clang_cc1 -fms-extensions %s -triple=x86_64-unknown-unknown 
-emit-llvm -o -
+// REQUIRES: asserts
+
+typedef int v4si __attribute__ ((vector_size (16)));
+v4si rep() {
+v4si res;
+__asm {}
+return res;
+}
Index: lib/CodeGen/CGStmt.cpp
===
--- lib/CodeGen/CGStmt.cpp
+++ lib/CodeGen/CGStmt.cpp
@@ -2210,7 +2210,7 @@
llvm::IntegerType::get(getLLVMContext(), 
(unsigned)TmpSize));
 Tmp = Builder.CreateTrunc(Tmp, TruncTy);
   } else if (TruncTy->isIntegerTy()) {
-Tmp = Builder.CreateTrunc(Tmp, TruncTy);
+Tmp = Builder.CreateZExtOrTrunc(Tmp, TruncTy);
   } else if (TruncTy->isVectorTy()) {
 Tmp = Builder.CreateBitCast(Tmp, TruncTy);
   }


Index: test/CodeGen/pr34021.c
===
--- test/CodeGen/pr34021.c
+++ test/CodeGen/pr34021.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fms-extensions %s -triple=i686-unknown-unknown -emit-llvm -o -
+// RUN: %clang_cc1 -fms-extensions %s -triple=x86_64-unknown-unknown -emit-llvm -o -
+// REQUIRES: asserts
+
+typedef int v4si __attribute__ ((vector_size (16)));
+v4si rep() {
+v4si res;
+__asm {}
+return res;
+}
Index: lib/CodeGen/CGStmt.cpp
===
--- lib/CodeGen/CGStmt.cpp
+++ lib/CodeGen/CGStmt.cpp
@@ -2210,7 +2210,7 @@
llvm::IntegerType::get(getLLVMContext(), (unsigned)TmpSize));
 Tmp = Builder.CreateTrunc(Tmp, TruncTy);
   } else if (TruncTy->isIntegerTy()) {
-Tmp = Builder.CreateTrunc(Tmp, TruncTy);
+Tmp = Builder.CreateZExtOrTrunc(Tmp, TruncTy);
   } else if (TruncTy->isVectorTy()) {
 Tmp = Builder.CreateBitCast(Tmp, TruncTy);
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

[Xinliang David Li via cfe-commits]

On Mon, Sep 4, 2017 at 1:57 AM, Chandler Carruth 
wrote:

> On Sun, Sep 3, 2017 at 10:41 PM Hal Finkel via llvm-commits <
> llvm-comm...@lists.llvm.org> wrote:
>
>> Nevertheless, I think that you've convinced me that this is a least-bad
>> solution. I'll want a flag preserving the old behavior. Something like
>> -fwiden-bitfield-accesses (modulo bikeshedding).
>>
> Several different approaches have been discussed in this thread, I'm not
> sure what you mean about "least-bad solution"...
>
> I remain completely unconvinced that we should change the default
> behavior. At most, I'm not strongly opposed to adding an attribute that
> indicates "please try to use narrow loads for this bitfield member" and is
> an error if applied to a misaligned or non-byte-sized bitfield member. But
> I remain strongly opposed to changing the default behavior.
>

Having an option and make it off by default for now is a fine solution. We
can also use this option to collect more empirical data on a wider range of
tests and architectures with the community's help.


> We have one or two cases that regress and are easily addressed by source
> changes (either to not use bitfields or to use an attribute). I don't think
> that is cause to change the lowering Clang has used for years and
> potentially regress many other use cases.
>

Note that this is not about that particular cases. It is more about doing
the right thing (to make compiler and HW work together to drive the best
performance for our users).

thanks,

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

Re: [PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

[Xinliang David Li via cfe-commits]

On Mon, Sep 4, 2017 at 9:17 AM, Hal Finkel  wrote:

>
> On 09/04/2017 03:57 AM, Chandler Carruth wrote:
>
> On Sun, Sep 3, 2017 at 10:41 PM Hal Finkel via llvm-commits <
> llvm-comm...@lists.llvm.org> wrote:
>
>> Nevertheless, I think that you've convinced me that this is a least-bad
>> solution. I'll want a flag preserving the old behavior. Something like
>> -fwiden-bitfield-accesses (modulo bikeshedding).
>>
> Several different approaches have been discussed in this thread, I'm not
> sure what you mean about "least-bad solution"...
>
> I remain completely unconvinced that we should change the default
> behavior. At most, I'm not strongly opposed to adding an attribute that
> indicates "please try to use narrow loads for this bitfield member" and is
> an error if applied to a misaligned or non-byte-sized bitfield member.
>
>
> I like this solution too (modulo the fact that I dislike all of these
> solutions). Restricting this only to affecting the loads, and not the
> stores, is also an interesting thought. The attribute could also be on the
> access itself (which, at least from the theoretical perspective, I'd
> prefer).
>
> But I remain strongly opposed to changing the default behavior. We have
> one or two cases that regress and are easily addressed by source changes
> (either to not use bitfields or to use an attribute). I don't think that is
> cause to change the lowering Clang has used for years and potentially
> regress many other use cases.
>
>
> I have mixed feelings about all of the potential fixes here. To walk
> through my thoughts on this:
>
>  1. I don't like any solutions that require changes affecting source level
> semantics. Something that the compiler does automatically is fine, as is an
> attribute.
>
>  2. Next, regarding default behavior, we have a trade off:
>
>A. Breaking apart the accesses, as proposed here, falls into the
> category of "generally, it makes everything a little bit slower." But it's
> worse than that because it comes on a spectrum. I can easily construct
> variants of the provided benchmark which make the separate loads have a bad
> performance penalty. For example:
>
> $ cat ~/tmp/3m.cpp
> class A {
> public:
> #ifdef BF
>   unsigned long f7:8;
>   unsigned long f6:8;
>   unsigned long f5:8;
>   unsigned long f4:8;
>   unsigned long f3:8;
>   unsigned long f2:8;
>   unsigned long f1:8;
>   unsigned long f0:8;
> #else
>   unsigned char f7;
>   unsigned char f6;
>   unsigned char f5;
>   unsigned char f4;
>   unsigned char f3;
>   unsigned char f2;
>   unsigned char f1;
>   unsigned char f0;
> #endif
> };
> A a;
> bool b;
> unsigned long N = 10;
>
> __attribute__((noinline))
> void foo() {
>   a.f4 = 3;
> }
>
> __attribute__((noinline))
> void goo() {
>   b = (a.f0 == 0 && a.f1 == (unsigned char)-1 &&
>a.f2 == 0 && a.f3 == 0 && a.f4 == 0 && a.f5 == 0 &&
>a.f6 == 0 && a.f7 == (unsigned char)-1);
> }
>
> int main() {
>   unsigned long i;
>   foo();
>   for (i = 0; i < N; i++) {
> goo();
>   }
> }
>
> Run this and you'll find that our current scheme, on Haswell, beats
> the separate-loads scheme by nearly 60% (e.g., 2.77s for separate loads vs.
> 1.75s for the current bitfield lowering).
>
> So, how common is it to have a bitfield with a large number of fields
> that could be loaded separately (i.e. have the right size and alignment)
> and have code that loads a large number of them like this (i.e. without
> other work to hide the relative costs)? I have no idea, but regardless,
> there is definitely a high-cost end to this spectrum.
>


Not sure about the answer. The test case itself is a little extreme. There
are many ways to change it slightly to tilt the balance. For instance
1) add one more field with store forwarding issue
2) reduce the number of tested fields in goo by 2 or 3 or more
3) having 2 or 3 more field stores in foo()
4) store non zero value to a.f0 so that short circuiting can happen without
load widening.
or
5) having more stores in foo, and enabling inlining and more aggressive
constant/copy propagation




>
>   B. However, our current scheme can trigger expensive architectural
> hazards. Moreover, there's no local after-the-fact analysis that can fix
> this consistently. I think that Wei has convincingly demonstrated both of
> these things. How common is this? I have no idea. More specifically, how do
> the relative costs of hitting these hazards compare to the costs of the
> increased number of loads under the proposed scheme? I have no idea (and
> this certainly has a workload-dependent answer).
>
>  C. This situation seems unlikely to change in the future: it seems like a
> physics problem. The data surrounding the narrower store is simply not in
> the pipeline to be matched with the wider load. Keeping the data in the
> pipeline would have extra costs, perhaps significant. I'm guessing the
> basic structure of this hazard is here to stay.
>
>  D. In the long run, could this be a 

[PATCH] D37386: [AMDGPU] Implement infrastructure to set options in AMDGPUToolChain

[Andrey Kasaurov via Phabricator via cfe-commits]

kasaurov updated this revision to Diff 113780.
kasaurov added a comment.

Test added to check passing of -On/default


https://reviews.llvm.org/D37386

Files:
  lib/Driver/ToolChains/AMDGPU.cpp
  lib/Driver/ToolChains/AMDGPU.h
  test/Driver/amdgpu-toolchain-opencl.cl

Index: test/Driver/amdgpu-toolchain-opencl.cl
===
--- /dev/null
+++ test/Driver/amdgpu-toolchain-opencl.cl
@@ -0,0 +1,19 @@
+// RUN: %clang -### -target amdgcn-amd-amdhsa-opencl -x cl -c -emit-llvm -mcpu=fiji -O0 %s 2>&1 | FileCheck -check-prefix=CHECK_O0 %s
+// RUN: %clang -### -target amdgcn-amd-amdhsa-opencl -x cl -c -emit-llvm -mcpu=fiji -O1 %s 2>&1 | FileCheck -check-prefix=CHECK_O1 %s
+// RUN: %clang -### -target amdgcn-amd-amdhsa-opencl -x cl -c -emit-llvm -mcpu=fiji -O2 %s 2>&1 | FileCheck -check-prefix=CHECK_O2 %s
+// RUN: %clang -### -target amdgcn-amd-amdhsa-opencl -x cl -c -emit-llvm -mcpu=fiji -O3 %s 2>&1 | FileCheck -check-prefix=CHECK_O3 %s
+// RUN: %clang -### -target amdgcn-amd-amdhsa-opencl -x cl -c -emit-llvm -mcpu=fiji -O4 %s 2>&1 | FileCheck -check-prefix=CHECK_O4 %s
+// RUN: %clang -### -target amdgcn-amd-amdhsa-opencl -x cl -c -emit-llvm -mcpu=fiji -O5 %s 2>&1 | FileCheck -check-prefix=CHECK_O5 %s
+// RUN: %clang -### -target amdgcn-amd-amdhsa-opencl -x cl -c -emit-llvm -mcpu=fiji -Og %s 2>&1 | FileCheck -check-prefix=CHECK_Og %s
+// RUN: %clang -### -target amdgcn-amd-amdhsa-opencl -x cl -c -emit-llvm -mcpu=fiji -Ofast %s 2>&1 | FileCheck -check-prefix=CHECK_Ofast %s
+// RUN: %clang -### -target amdgcn-amd-amdhsa-opencl -x cl -c -emit-llvm -mcpu=fiji %s 2>&1 | FileCheck -check-prefix=CHECK_O_DEFAULT %s
+// CHECK_O0: clang{{.*}} "-O0"
+// CHECK_O1: clang{{.*}} "-O1"
+// CHECK_O2: clang{{.*}} "-O2"
+// CHECK_O3: clang{{.*}} "-O3"
+// CHECK_O4: clang{{.*}} "-O3"
+// CHECK_O5: clang{{.*}} "-O5"
+// CHECK_Og: clang{{.*}} "-Og"
+// CHECK_Ofast: {{.*}}clang{{.*}} "-Ofast"
+// CHECK_O_DEFAULT: clang{{.*}} "-O3"
+
Index: lib/Driver/ToolChains/AMDGPU.h
===
--- lib/Driver/ToolChains/AMDGPU.h
+++ lib/Driver/ToolChains/AMDGPU.h
@@ -11,8 +11,10 @@
 #define LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_AMDGPU_H
 
 #include "Gnu.h"
+#include "clang/Driver/Options.h"
 #include "clang/Driver/Tool.h"
 #include "clang/Driver/ToolChain.h"
+#include 
 
 namespace clang {
 namespace driver {
@@ -37,14 +39,26 @@
 namespace toolchains {
 
 class LLVM_LIBRARY_VISIBILITY AMDGPUToolChain : public Generic_ELF {
+
+private:
+  const std::map OptionsDefault;
+
 protected:
   Tool *buildLinker() const override;
+  const StringRef getOptionDefault(options::ID OptID) const {
+auto opt = OptionsDefault.find(OptID);
+assert(opt != OptionsDefault.end() && "No Default for Option");
+return opt->second;
+  }
 
 public:
   AMDGPUToolChain(const Driver , const llvm::Triple ,
-const llvm::opt::ArgList );
+  const llvm::opt::ArgList );
   unsigned GetDefaultDwarfVersion() const override { return 2; }
   bool IsIntegratedAssemblerDefault() const override { return true; }
+  llvm::opt::DerivedArgList *
+  TranslateArgs(const llvm::opt::DerivedArgList , StringRef BoundArch,
+Action::OffloadKind DeviceOffloadKind) const override;
 };
 
 } // end namespace toolchains
Index: lib/Driver/ToolChains/AMDGPU.cpp
===
--- lib/Driver/ToolChains/AMDGPU.cpp
+++ lib/Driver/ToolChains/AMDGPU.cpp
@@ -8,8 +8,8 @@
 //===--===//
 
 #include "AMDGPU.h"
-#include "InputInfo.h"
 #include "CommonArgs.h"
+#include "InputInfo.h"
 #include "clang/Driver/Compilation.h"
 #include "llvm/Option/ArgList.h"
 
@@ -38,8 +38,45 @@
 /// AMDGPU Toolchain
 AMDGPUToolChain::AMDGPUToolChain(const Driver , const llvm::Triple ,
  const ArgList )
-  : Generic_ELF(D, Triple, Args) { }
+: Generic_ELF(D, Triple, Args),
+  OptionsDefault({{options::OPT_O, "3"},
+  {options::OPT_cl_std_EQ, "CL1.2"}}) {}
 
 Tool *AMDGPUToolChain::buildLinker() const {
   return new tools::amdgpu::Linker(*this);
 }
+
+DerivedArgList *
+AMDGPUToolChain::TranslateArgs(const DerivedArgList , StringRef BoundArch,
+   Action::OffloadKind DeviceOffloadKind) const {
+
+  DerivedArgList *DAL =
+  Generic_ELF::TranslateArgs(Args, BoundArch, DeviceOffloadKind);
+
+  // Do nothing if not OpenCL (-x cl)
+  if (!Args.getLastArgValue(options::OPT_x).equals("cl"))
+return DAL;
+
+  if (!DAL)
+DAL = new DerivedArgList(Args.getBaseArgs());
+  for (auto *A : Args)
+DAL->append(A);
+
+  const OptTable  = getDriver().getOpts();
+
+  // Phase 1 (.cl -> .bc)
+  if (Args.hasArg(options::OPT_c) && Args.hasArg(options::OPT_emit_llvm)) {
+DAL->AddFlagArg(nullptr, Opts.getOption(getTriple().isArch64Bit()
+  

[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file

[Boris Kolpackov via Phabricator via cfe-commits]

boris updated this revision to Diff 113779.
boris marked an inline comment as done.
boris added a comment.

New patch revision with David's comments addressed.


https://reviews.llvm.org/D37299

Files:
  docs/Modules.rst
  include/clang/Basic/DiagnosticDriverKinds.td
  include/clang/Driver/Options.td
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp

Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -1528,8 +1528,80 @@
   return P.str();
 }
 
+// Read the mapping of module names to precompiled module files from a file.
+// The argument can include an optional line prefix ([]=), in
+// which case only lines that start with the prefix are considered (with the
+// prefix and the following whitespaces, if any, ignored).
+//
+// Each mapping entry should be in the same form as the -fmodule-file option
+// value (=) with leading/trailing whitespaces ignored.
+//
+static void LoadModuleFileMap(HeaderSearchOptions ,
+  DiagnosticsEngine , FileManager ,
+  StringRef Val, const std::string ) {
+  // See if we have the prefix.
+  StringRef File;
+  StringRef Prefix;
+  if (Val.find('=') != StringRef::npos) {
+auto Pair = Val.split('=');
+Prefix = Pair.first;
+File = Pair.second;
+if (Prefix.empty()) {
+  Diags.Report(diag::err_drv_invalid_value) << Arg << Val;
+  return;
+}
+  } else
+File = Val;
+
+  if (File.empty()) {
+Diags.Report(diag::err_drv_invalid_value) << Arg << Val;
+return;
+  }
+
+  auto Buf = FileMgr.getBufferForFile(File);
+  if (!Buf) {
+Diags.Report(diag::err_cannot_open_file)
+<< File << Buf.getError().message();
+return;
+  }
+
+  // Read the file line by line.
+  StringRef Str = Buf.get()->getBuffer();
+  for (size_t B = 0, E = 0; B < Str.size(); B = E + 1) {
+E = Str.find_first_of(StringRef("\n\0", 2), B);
+
+if (E == StringRef::npos)
+  E = Str.size();
+else if (Str[E] == '\0')
+  break; // The file (or the rest of it) is binary, bail out.
+
+// [B, E) is our line. Compare and skip the prefix, if any.
+StringRef Line = Str.substr(B, E - B);
+if (!Prefix.empty()) {
+  if (!Line.startswith(Prefix))
+continue;
+
+  Line = Line.substr(Prefix.size());
+}
+
+// Skip leading and trailing whitespaces and ignore blanks (even if they
+// had prefix; think make comments).
+Line = Line.trim();
+if (Line.empty())
+  continue;
+
+if (Line.find('=') == StringRef::npos) {
+  Diags.Report(diag::err_drv_invalid_module_file_map) << Line;
+  continue;
+}
+
+Opts.PrebuiltModuleFiles.insert(Line.split('='));
+  }
+}
+
 static void ParseHeaderSearchArgs(HeaderSearchOptions , ArgList ,
-  const std::string ) {
+  DiagnosticsEngine ,
+  FileManager ) {
   using namespace options;
   Opts.Sysroot = Args.getLastArgValue(OPT_isysroot, "/");
   Opts.Verbose = Args.hasArg(OPT_v);
@@ -1543,6 +1615,7 @@
   // Canonicalize -fmodules-cache-path before storing it.
   SmallString<128> P(Args.getLastArgValue(OPT_fmodules_cache_path));
   if (!(P.empty() || llvm::sys::path::is_absolute(P))) {
+const std::string  (FileMgr.getFileSystemOpts().WorkingDir);
 if (WorkingDir.empty())
   llvm::sys::fs::make_absolute(P);
 else
@@ -1558,6 +1631,8 @@
 if (Val.find('=') != StringRef::npos)
   Opts.PrebuiltModuleFiles.insert(Val.split('='));
   }
+  for (const Arg *A : Args.filtered(OPT_fmodule_file_map))
+LoadModuleFileMap(Opts, Diags, FileMgr, A->getValue(), A->getAsString(Args));
   for (const Arg *A : Args.filtered(OPT_fprebuilt_module_path))
 Opts.AddPrebuiltModulePath(A->getValue());
   Opts.DisableModuleHash = Args.hasArg(OPT_fdisable_module_hash);
@@ -2511,7 +2586,6 @@
 }
 
 static void ParsePreprocessorArgs(PreprocessorOptions , ArgList ,
-  FileManager ,
   DiagnosticsEngine ,
   frontend::ActionKind Action) {
   using namespace options;
@@ -2680,14 +2754,19 @@
   false /*DefaultDiagColor*/, false /*DefaultShowOpt*/);
   ParseCommentArgs(LangOpts.CommentOpts, Args);
   ParseFileSystemArgs(Res.getFileSystemOpts(), Args);
+
+  // File manager used during option parsing (e.g., for loading map files,
+  // etc).
+  //
+  FileManager FileMgr(Res.getFileSystemOpts());
+
   // FIXME: We shouldn't have to pass the DashX option around here
   InputKind DashX = ParseFrontendArgs(Res.getFrontendOpts(), Args, Diags,
   LangOpts.IsHeaderFile);
   ParseTargetArgs(Res.getTargetOpts(), Args, Diags);
   Success &= ParseCodeGenArgs(Res.getCodeGenOpts(), Args, DashX, Diags,
   

[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file

[Boris Kolpackov via Phabricator via cfe-commits]

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

David, thanks for the review. Uploading the new revision (also rebased on HEAD).




Comment at: lib/Frontend/CompilerInvocation.cpp:1576
+  StringRef Str = Buf.get()->getBuffer();
+  for (size_t B(0), E(B); B < Str.size(); B = E + 1)
+  {

dblaikie wrote:
> Prefer copy init over direct init:
> 
>   for (size_t B = 0, E = 0;
> 
> & probably != rather than < is more canonical/common in the LLVM codebase.
All done except the != change -- B can actually go one over size (see the npos 
case in the loop body).


https://reviews.llvm.org/D37299



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

[PATCH] D37386: [AMDGPU] Implement infrastructure to set options in AMDGPUToolChain

[Stanislav Mekhanoshin via Phabricator via cfe-commits]

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

LGTM


https://reviews.llvm.org/D37386



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

[PATCH] D37390: [diagtool] Change default tree behavior to print only flags

[Alex Lorenz via Phabricator via cfe-commits]

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

LGTM.

Please precommit the `auto` loop modernization though. You might want to use 
`llvm::make_range` for for-ranged loops as well.


https://reviews.llvm.org/D37390



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

[PATCH] D37386: [AMDGPU] Implement infrastructure to set options in AMDGPUToolChain

[Andrey Kasaurov via Phabricator via cfe-commits]

kasaurov added inline comments.



Comment at: lib/Driver/ToolChains/AMDGPU.h:60
   bool IsIntegratedAssemblerDefault() const override { return true; }
+  llvm::opt::DerivedArgList *
+  TranslateArgs(const llvm::opt::DerivedArgList , StringRef BoundArch,

emankov wrote:
> Return arg on the same line.
That's how clang-format sees it: 
-DerivedArgList *AMDGPUToolChain::TranslateArgs(const DerivedArgList , 
StringRef BoundArch, Action::OffloadKind DeviceOffloadKind) const {
+DerivedArgList *
+AMDGPUToolChain::TranslateArgs(const DerivedArgList , StringRef BoundArch,
+   Action::OffloadKind DeviceOffloadKind) const {



https://reviews.llvm.org/D37386



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

[PATCH] D37386: [AMDGPU] Implement infrastructure to set options in AMDGPUToolChain

[Andrey Kasaurov via Phabricator via cfe-commits]

kasaurov updated this revision to Diff 113767.
kasaurov added a comment.

Several requested changes:

- move map initialization out of header file -- visually it looks better in .h 
file, but logically it should be in .cpp
- add assert() in getOptionDefault() -- check for unknown to OptionsDefault 
option
- combine args for OPT_O check
- use const StrinRef instead of std::string in the map and getOptionDefault() 
-- that eliminates conversion in AddJoinedArg()
- eliminate redundant brackets

ToDo:

- test


https://reviews.llvm.org/D37386

Files:
  lib/Driver/ToolChains/AMDGPU.cpp
  lib/Driver/ToolChains/AMDGPU.h

Index: lib/Driver/ToolChains/AMDGPU.h
===
--- lib/Driver/ToolChains/AMDGPU.h
+++ lib/Driver/ToolChains/AMDGPU.h
@@ -11,8 +11,10 @@
 #define LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_AMDGPU_H
 
 #include "Gnu.h"
+#include "clang/Driver/Options.h"
 #include "clang/Driver/Tool.h"
 #include "clang/Driver/ToolChain.h"
+#include 
 
 namespace clang {
 namespace driver {
@@ -37,14 +39,26 @@
 namespace toolchains {
 
 class LLVM_LIBRARY_VISIBILITY AMDGPUToolChain : public Generic_ELF {
+
+private:
+  const std::map OptionsDefault;
+
 protected:
   Tool *buildLinker() const override;
+  const StringRef getOptionDefault(options::ID OptID) const {
+auto opt = OptionsDefault.find(OptID);
+assert(opt != OptionsDefault.end() && "No Default for Option");
+return opt->second;
+  }
 
 public:
   AMDGPUToolChain(const Driver , const llvm::Triple ,
-const llvm::opt::ArgList );
+  const llvm::opt::ArgList );
   unsigned GetDefaultDwarfVersion() const override { return 2; }
   bool IsIntegratedAssemblerDefault() const override { return true; }
+  llvm::opt::DerivedArgList *
+  TranslateArgs(const llvm::opt::DerivedArgList , StringRef BoundArch,
+Action::OffloadKind DeviceOffloadKind) const override;
 };
 
 } // end namespace toolchains
Index: lib/Driver/ToolChains/AMDGPU.cpp
===
--- lib/Driver/ToolChains/AMDGPU.cpp
+++ lib/Driver/ToolChains/AMDGPU.cpp
@@ -8,8 +8,8 @@
 //===--===//
 
 #include "AMDGPU.h"
-#include "InputInfo.h"
 #include "CommonArgs.h"
+#include "InputInfo.h"
 #include "clang/Driver/Compilation.h"
 #include "llvm/Option/ArgList.h"
 
@@ -38,8 +38,45 @@
 /// AMDGPU Toolchain
 AMDGPUToolChain::AMDGPUToolChain(const Driver , const llvm::Triple ,
  const ArgList )
-  : Generic_ELF(D, Triple, Args) { }
+: Generic_ELF(D, Triple, Args),
+  OptionsDefault({{options::OPT_O, "3"},
+  {options::OPT_cl_std_EQ, "CL1.2"}}) {}
 
 Tool *AMDGPUToolChain::buildLinker() const {
   return new tools::amdgpu::Linker(*this);
 }
+
+DerivedArgList *
+AMDGPUToolChain::TranslateArgs(const DerivedArgList , StringRef BoundArch,
+   Action::OffloadKind DeviceOffloadKind) const {
+
+  DerivedArgList *DAL =
+  Generic_ELF::TranslateArgs(Args, BoundArch, DeviceOffloadKind);
+
+  // Do nothing if not OpenCL (-x cl)
+  if (!Args.getLastArgValue(options::OPT_x).equals("cl"))
+return DAL;
+
+  if (!DAL)
+DAL = new DerivedArgList(Args.getBaseArgs());
+  for (auto *A : Args)
+DAL->append(A);
+
+  const OptTable  = getDriver().getOpts();
+
+  // Phase 1 (.cl -> .bc)
+  if (Args.hasArg(options::OPT_c) && Args.hasArg(options::OPT_emit_llvm)) {
+DAL->AddFlagArg(nullptr, Opts.getOption(getTriple().isArch64Bit()
+? options::OPT_m64
+: options::OPT_m32));
+
+// Have to check OPT_O4, OPT_O0 & OPT_Ofast separately
+// as they defined that way in Options.td
+if (!Args.hasArg(options::OPT_O, options::OPT_O0, options::OPT_O4,
+ options::OPT_Ofast))
+  DAL->AddJoinedArg(nullptr, Opts.getOption(options::OPT_O),
+getOptionDefault(options::OPT_O));
+  }
+
+  return DAL;
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

[Hal Finkel via cfe-commits]

On 09/04/2017 03:57 AM, Chandler Carruth wrote:
On Sun, Sep 3, 2017 at 10:41 PM Hal Finkel via llvm-commits 
> wrote:


Nevertheless, I think that you've convinced me that this is a
least-bad solution. I'll want a flag preserving the old behavior.
Something like -fwiden-bitfield-accesses (modulo bikeshedding).

Several different approaches have been discussed in this thread, I'm 
not sure what you mean about "least-bad solution"...


I remain completely unconvinced that we should change the default 
behavior. At most, I'm not strongly opposed to adding an attribute 
that indicates "please try to use narrow loads for this bitfield 
member" and is an error if applied to a misaligned or non-byte-sized 
bitfield member.


I like this solution too (modulo the fact that I dislike all of these 
solutions). Restricting this only to affecting the loads, and not the 
stores, is also an interesting thought. The attribute could also be on 
the access itself (which, at least from the theoretical perspective, I'd 
prefer).


But I remain strongly opposed to changing the default behavior. We 
have one or two cases that regress and are easily addressed by source 
changes (either to not use bitfields or to use an attribute). I don't 
think that is cause to change the lowering Clang has used for years 
and potentially regress many other use cases.


I have mixed feelings about all of the potential fixes here. To walk 
through my thoughts on this:


 1. I don't like any solutions that require changes affecting source 
level semantics. Something that the compiler does automatically is fine, 
as is an attribute.


 2. Next, regarding default behavior, we have a trade off:

   A. Breaking apart the accesses, as proposed here, falls into the 
category of "generally, it makes everything a little bit slower." But 
it's worse than that because it comes on a spectrum. I can easily 
construct variants of the provided benchmark which make the separate 
loads have a bad performance penalty. For example:


$ cat ~/tmp/3m.cpp
class A {
public:
#ifdef BF
  unsigned long f7:8;
  unsigned long f6:8;
  unsigned long f5:8;
  unsigned long f4:8;
  unsigned long f3:8;
  unsigned long f2:8;
  unsigned long f1:8;
  unsigned long f0:8;
#else
  unsigned char f7;
  unsigned char f6;
  unsigned char f5;
  unsigned char f4;
  unsigned char f3;
  unsigned char f2;
  unsigned char f1;
  unsigned char f0;
#endif
};
A a;
bool b;
unsigned long N = 10;

__attribute__((noinline))
void foo() {
  a.f4 = 3;
}

__attribute__((noinline))
void goo() {
  b = (a.f0 == 0 && a.f1 == (unsigned char)-1 &&
   a.f2 == 0 && a.f3 == 0 && a.f4 == 0 && a.f5 == 0 &&
   a.f6 == 0 && a.f7 == (unsigned char)-1);
}

int main() {
  unsigned long i;
  foo();
  for (i = 0; i < N; i++) {
goo();
  }
}

Run this and you'll find that our current scheme, on Haswell, beats 
the separate-loads scheme by nearly 60% (e.g., 2.77s for separate loads 
vs. 1.75s for the current bitfield lowering).


So, how common is it to have a bitfield with a large number of 
fields that could be loaded separately (i.e. have the right size and 
alignment) and have code that loads a large number of them like this 
(i.e. without other work to hide the relative costs)? I have no idea, 
but regardless, there is definitely a high-cost end to this spectrum.


  B. However, our current scheme can trigger expensive architectural 
hazards. Moreover, there's no local after-the-fact analysis that can fix 
this consistently. I think that Wei has convincingly demonstrated both 
of these things. How common is this? I have no idea. More specifically, 
how do the relative costs of hitting these hazards compare to the costs 
of the increased number of loads under the proposed scheme? I have no 
idea (and this certainly has a workload-dependent answer).


 C. This situation seems unlikely to change in the future: it seems 
like a physics problem. The data surrounding the narrower store is 
simply not in the pipeline to be matched with the wider load. Keeping 
the data in the pipeline would have extra costs, perhaps significant. 
I'm guessing the basic structure of this hazard is here to stay.


 D. In the long run, could this be a PGO-driven decision? Yes, and this 
seems optimal. It would depend on infrastructure enhancements, and 
critically, the hardware having the right performance counter(s) to sample.


So, as to the question of what to do right now, I have two thoughts: 1) 
All of the solutions will be bad for someone. 2) Which is a least-bad 
default depends on the workload. Your colleagues seem to be asserting 
that, for Google, the separate loads are least bad (and, FWIW, you're 
more likely to have hot code like this than I am). This is definitely an 
issue on which reasonable people can disagree. In the end, I'll 
begrudgingly agree that this should be an empirical decision. We should 
have some 

[PATCH] D37442: [C++17] Disallow lambdas in template parameters (PR33696).

[Blitz Rakete via Phabricator via cfe-commits]

Rakete created this revision.

This revision disallows lambdas in template parameters, as reported in PR33696.


https://reviews.llvm.org/D37442

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/Parse/ParseTemplate.cpp
  lib/Sema/Sema.cpp
  lib/Sema/SemaExpr.cpp
  test/CXX/expr/expr.prim/expr.prim.lambda/p2-template-parameter.cpp

Index: test/CXX/expr/expr.prim/expr.prim.lambda/p2-template-parameter.cpp
===
--- test/CXX/expr/expr.prim/expr.prim.lambda/p2-template-parameter.cpp
+++ test/CXX/expr/expr.prim/expr.prim.lambda/p2-template-parameter.cpp
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -std=c++17 %s -verify
+
+template struct Nothing {};
+
+void pr33696() {
+Nothing<[]() { return 0; }()> nothing; // expected-error{{a lambda expression cannot appear in this context}}
+}
+
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -13355,9 +13355,10 @@
 void
 Sema::PushExpressionEvaluationContext(ExpressionEvaluationContext NewContext,
   Decl *LambdaContextDecl,
-  bool IsDecltype) {
+  bool IsDecltype,
+  bool IsLambdaValid) {
   ExprEvalContexts.emplace_back(NewContext, ExprCleanupObjects.size(), Cleanup,
-LambdaContextDecl, IsDecltype);
+LambdaContextDecl, IsDecltype, IsLambdaValid);
   Cleanup.reset();
   if (!MaybeODRUseExprs.empty())
 std::swap(MaybeODRUseExprs, ExprEvalContexts.back().SavedMaybeODRUseExprs);
@@ -13366,9 +13367,11 @@
 void
 Sema::PushExpressionEvaluationContext(ExpressionEvaluationContext NewContext,
   ReuseLambdaContextDecl_t,
-  bool IsDecltype) {
+  bool IsDecltype,
+  bool IsLambdaValid) {
   Decl *ClosureContextDecl = ExprEvalContexts.back().ManglingContextDecl;
-  PushExpressionEvaluationContext(NewContext, ClosureContextDecl, IsDecltype);
+  PushExpressionEvaluationContext(NewContext, ClosureContextDecl, IsDecltype,
+  IsLambdaValid);
 }
 
 void Sema::PopExpressionEvaluationContext() {
@@ -13376,7 +13379,8 @@
   unsigned NumTypos = Rec.NumTypos;
 
   if (!Rec.Lambdas.empty()) {
-if (Rec.isUnevaluated() || Rec.isConstantEvaluated()) {
+if (!Rec.IsLambdaValid || Rec.isUnevaluated() ||
+(Rec.isConstantEvaluated() && !getLangOpts().CPlusPlus1z)) {
   unsigned D;
   if (Rec.isUnevaluated()) {
 // C++11 [expr.prim.lambda]p2:
@@ -13383,23 +13387,21 @@
 //   A lambda-expression shall not appear in an unevaluated operand
 //   (Clause 5).
 D = diag::err_lambda_unevaluated_operand;
-  } else {
+  } else if (Rec.isConstantEvaluated() && !getLangOpts().CPlusPlus1z) {
 // C++1y [expr.const]p2:
 //   A conditional-expression e is a core constant expression unless the
 //   evaluation of e, following the rules of the abstract machine, would
 //   evaluate [...] a lambda-expression.
 D = diag::err_lambda_in_constant_expression;
-  }
+  } else if (!Rec.IsLambdaValid) {
+// C++17 [expr.prim.lambda]p2:
+// A lambda-expression shall not appear [...] in a template-argument.
+D = diag::err_lambda_in_invalid_context;
+  } else
+llvm_unreachable("Couldn't infer lambda error message.");
 
-  // C++1z allows lambda expressions as core constant expressions.
-  // FIXME: In C++1z, reinstate the restrictions on lambda expressions (CWG
-  // 1607) from appearing within template-arguments and array-bounds that
-  // are part of function-signatures.  Be mindful that P0315 (Lambdas in
-  // unevaluated contexts) might lift some of these restrictions in a 
-  // future version.
-  if (!Rec.isConstantEvaluated() || !getLangOpts().CPlusPlus1z)
-for (const auto *L : Rec.Lambdas)
-  Diag(L->getLocStart(), D);
+  for (const auto *L : Rec.Lambdas)
+Diag(L->getLocStart(), D);
 } else {
   // Mark the capture expressions odr-used. This was deferred
   // during lambda expression creation.
Index: lib/Sema/Sema.cpp
===
--- lib/Sema/Sema.cpp
+++ lib/Sema/Sema.cpp
@@ -159,7 +159,7 @@
 
   ExprEvalContexts.emplace_back(
   ExpressionEvaluationContext::PotentiallyEvaluated, 0, CleanupInfo{},
-  nullptr, false);
+  nullptr, false, true);
 
   FunctionScopes.push_back(new FunctionScopeInfo(Diags));
 
Index: lib/Parse/ParseTemplate.cpp
===
--- lib/Parse/ParseTemplate.cpp
+++ lib/Parse/ParseTemplate.cpp

[PATCH] D37299: [Modules] Add ability to specify module name to module file mapping in a file

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Couple of drive by comments - no doubt Richard will have a more in depth 
review, but figured this might save a round or two.




Comment at: lib/Frontend/CompilerInvocation.cpp:1561-1565
+  if (File.empty())
+  {
+Diags.Report(diag::err_drv_invalid_value) << Arg << Val;
+return;
+  }

Run the whole change through clang-format (there are tools to help do this over 
a patch or git revision range) so it matches LLVM coding conventions



Comment at: lib/Frontend/CompilerInvocation.cpp:1576
+  StringRef Str = Buf.get()->getBuffer();
+  for (size_t B(0), E(B); B < Str.size(); B = E + 1)
+  {

Prefer copy init over direct init:

  for (size_t B = 0, E = 0;

& probably != rather than < is more canonical/common in the LLVM codebase.



Comment at: lib/Frontend/CompilerInvocation.cpp:1586
+// [B, E) is our line. Compare and skip the prefix, if any.
+StringRef Line(Str.data() + B, E - B);
+if (!Prefix.empty())

There's probably a StringRef-y way to do this rather than going back through 
raw pointers? (StringRef::substr or the like)


https://reviews.llvm.org/D37299



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

[libclc] r312491 - Fixup clc.h comment

[Jan Vesely via cfe-commits]

Author: jvesely
Date: Mon Sep  4 08:52:03 2017
New Revision: 312491

URL: http://llvm.org/viewvc/llvm-project?rev=312491=rev
Log:
Fixup clc.h comment

Signed-off-by: Jan Vesely 

Modified:
libclc/trunk/generic/include/clc/clc.h

Modified: libclc/trunk/generic/include/clc/clc.h
URL: 
http://llvm.org/viewvc/llvm-project/libclc/trunk/generic/include/clc/clc.h?rev=312491=312490=312491=diff
==
--- libclc/trunk/generic/include/clc/clc.h (original)
+++ libclc/trunk/generic/include/clc/clc.h Mon Sep  4 08:52:03 2017
@@ -235,12 +235,11 @@
 #include 
 #include 
 
-/* 6.11.13 Image Read and Write Functions */
-
 /* 6.12.12 Miscellaneous Vector Functions */
 #include 
 #include 
 
+/* 6.11.13 Image Read and Write Functions */
 #include 
 #include 
 


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

[libclc] r312492 - r600: Cleanup barrier implementation.

[Jan Vesely via cfe-commits]

Author: jvesely
Date: Mon Sep  4 08:52:05 2017
New Revision: 312492

URL: http://llvm.org/viewvc/llvm-project?rev=312492=rev
Log:
r600: Cleanup barrier implementation.

We don't have memory fences for r600 so just call group barrier directly
Make sure that barrier is called even with 0 flags

Signed-off-by: Jan Vesely 
Reviewed-by: Aaron Watry 

Removed:
libclc/trunk/amdgpu/lib/synchronization/barrier.cl
Modified:
libclc/trunk/amdgpu/lib/SOURCES
libclc/trunk/r600/lib/synchronization/barrier_impl.ll

Modified: libclc/trunk/amdgpu/lib/SOURCES
URL: 
http://llvm.org/viewvc/llvm-project/libclc/trunk/amdgpu/lib/SOURCES?rev=312492=312491=312492=diff
==
--- libclc/trunk/amdgpu/lib/SOURCES (original)
+++ libclc/trunk/amdgpu/lib/SOURCES Mon Sep  4 08:52:05 2017
@@ -1,7 +1,6 @@
 atomic/atomic.cl
 math/nextafter.cl
 math/sqrt.cl
-synchronization/barrier.cl
 image/get_image_width.cl
 image/get_image_height.cl
 image/get_image_depth.cl

Removed: libclc/trunk/amdgpu/lib/synchronization/barrier.cl
URL: 
http://llvm.org/viewvc/llvm-project/libclc/trunk/amdgpu/lib/synchronization/barrier.cl?rev=312491=auto
==
--- libclc/trunk/amdgpu/lib/synchronization/barrier.cl (original)
+++ libclc/trunk/amdgpu/lib/synchronization/barrier.cl (removed)
@@ -1,10 +0,0 @@
-
-#include 
-
-_CLC_DEF int __clc_clk_local_mem_fence() {
-  return CLK_LOCAL_MEM_FENCE;
-}
-
-_CLC_DEF int __clc_clk_global_mem_fence() {
-  return CLK_GLOBAL_MEM_FENCE;
-}

Modified: libclc/trunk/r600/lib/synchronization/barrier_impl.ll
URL: 
http://llvm.org/viewvc/llvm-project/libclc/trunk/r600/lib/synchronization/barrier_impl.ll?rev=312492=312491=312492=diff
==
--- libclc/trunk/r600/lib/synchronization/barrier_impl.ll (original)
+++ libclc/trunk/r600/lib/synchronization/barrier_impl.ll Mon Sep  4 08:52:05 
2017
@@ -1,32 +1,11 @@
-declare i32 @__clc_clk_local_mem_fence() #1
-declare i32 @__clc_clk_global_mem_fence() #1
 declare void @llvm.r600.group.barrier() #0
 
-define void @barrier(i32 %flags) #2 {
-barrier_local_test:
-  %CLK_LOCAL_MEM_FENCE = call i32 @__clc_clk_local_mem_fence()
-  %0 = and i32 %flags, %CLK_LOCAL_MEM_FENCE
-  %1 = icmp ne i32 %0, 0
-  br i1 %1, label %barrier_local, label %barrier_global_test
-
-barrier_local:
-  call void @llvm.r600.group.barrier()
-  br label %barrier_global_test
-
-barrier_global_test:
-  %CLK_GLOBAL_MEM_FENCE = call i32 @__clc_clk_global_mem_fence()
-  %2 = and i32 %flags, %CLK_GLOBAL_MEM_FENCE
-  %3 = icmp ne i32 %2, 0
-  br i1 %3, label %barrier_global, label %done
-
-barrier_global:
-  call void @llvm.r600.group.barrier()
-  br label %done
-
-done:
+define void @barrier(i32 %flags) #1 {
+entry:
+  ; We should call mem_fence here, but that is not implemented for r600 yet
+  tail call void @llvm.r600.group.barrier()
   ret void
 }
 
 attributes #0 = { nounwind convergent }
-attributes #1 = { nounwind alwaysinline }
-attributes #2 = { nounwind convergent alwaysinline }
+attributes #1 = { nounwind convergent alwaysinline }


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

[libclc] r312493 - amdgcn,waitcnt: Add datalayout info

[Jan Vesely via cfe-commits]

Author: jvesely
Date: Mon Sep  4 08:52:07 2017
New Revision: 312493

URL: http://llvm.org/viewvc/llvm-project?rev=312493=rev
Log:
amdgcn,waitcnt: Add datalayout info

This file is only compiled for GCN which all share the same layout

Signed-off-by: Jan Vesely 
Reviewed-by: Aaron Watry 

Modified:
libclc/trunk/amdgcn/lib/mem_fence/waitcnt.ll

Modified: libclc/trunk/amdgcn/lib/mem_fence/waitcnt.ll
URL: 
http://llvm.org/viewvc/llvm-project/libclc/trunk/amdgcn/lib/mem_fence/waitcnt.ll?rev=312493=312492=312493=diff
==
--- libclc/trunk/amdgcn/lib/mem_fence/waitcnt.ll (original)
+++ libclc/trunk/amdgcn/lib/mem_fence/waitcnt.ll Mon Sep  4 08:52:07 2017
@@ -1,5 +1,7 @@
 declare void @llvm.amdgcn.s.waitcnt(i32) #0
 
+target datalayout = 
"e-p:32:32-p1:64:64-p2:64:64-p3:32:32-p4:64:64-p5:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64"
+
 ; Export waitcnt intrinsic for clang < 5
 define void @__clc_amdgcn_s_waitcnt(i32 %flags) #1 {
 entry:


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

Re: [PATCH] D37428: Debug info: Fixed faulty debug locations for attributed statements

[David Blaikie via cfe-commits]

Seems like the test case should/could be simplified a bit, maybe? (could it
be something really simple like "void f(bool b) { #pragma ... while (b) ;
}"?)

Also, is it relying on LLVM optimizations to run (add
"-disable-llvm-passes" to the command line to be sure it isn't, perhaps?)?

On Sun, Sep 3, 2017 at 10:35 AM Karl-Johan Karlsson via Phabricator via
cfe-commits  wrote:

> Ka-Ka created this revision.
> Herald added a subscriber: aprantl.
>
> As no stoppoint was generated the attributed statements got faulty debug
> locations.
>
>
> https://reviews.llvm.org/D37428
>
> Files:
>   lib/CodeGen/CGStmt.cpp
>   test/CodeGen/debug-info-attributed-stmt.c
>
>
> Index: test/CodeGen/debug-info-attributed-stmt.c
> ===
> --- /dev/null
> +++ test/CodeGen/debug-info-attributed-stmt.c
> @@ -0,0 +1,19 @@
> +// RUN: %clang_cc1 -triple x86_64-unk-unk -debug-info-kind=limited
> -emit-llvm %s -o - | FileCheck %s
> +int data[50] = { 0 };
> +
> +void foo()
> +{
> +int i = 0;
> +int x = 7;
> +#pragma nounroll
> +while (i < 50)
> +{
> +data[i] = i;
> +++i;
> +}
> +
> +// CHECK: br label %while.cond, !dbg ![[NUM:[0-9]+]]
> +// CHECK: br i1 %cmp, label %while.body, label %while.end, !dbg ![[NUM]]
> +// CHECK: br label %while.cond, !dbg ![[NUM]], !llvm.loop
> +// CHECK: ![[NUM]] = !DILocation(line: 9, scope: !14)
> +}
> Index: lib/CodeGen/CGStmt.cpp
> ===
> --- lib/CodeGen/CGStmt.cpp
> +++ lib/CodeGen/CGStmt.cpp
> @@ -556,6 +556,10 @@
>
>  void CodeGenFunction::EmitAttributedStmt(const AttributedStmt ) {
>const Stmt *SubStmt = S.getSubStmt();
> +
> +  // Generate a stoppoint if we are emitting debug info.
> +  EmitStopPoint(SubStmt);
> +
>switch (SubStmt->getStmtClass()) {
>case Stmt::DoStmtClass:
>  EmitDoStmt(cast(*SubStmt), S.getAttrs());
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37437: [analyzer] Fix SimpleStreamChecker's output plist not containing the checker name

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Cool. Thanks!

> In the future probably it would be better to alter the signature of the 
> checkers' constructor to set the name in the constructor so it is possible to 
> create the BugType eagerly.

Still, should we add an assertion so that we could be sure that every bug type 
contains a checker name?


Repository:
  rL LLVM

https://reviews.llvm.org/D37437



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

[PATCH] D36347: Add new script to launch lldb and set breakpoints for diagnostics all diagnostics seen.

[Don Hinton via Phabricator via cfe-commits]

hintonda added a comment.

ping...


https://reviews.llvm.org/D36347



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

[PATCH] D37437: [analyzer] Fix SimpleStreamChecker's output plist not containing the checker name

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun created this revision.
Herald added subscribers: baloghadamsoftware, whisperity.

Unfortunately, currently, the analyzer core sets the checker name after the 
constructor was already run. So if we set the BugType in the constructor, the 
output plist will not contain a checker name.  Right now the idiomatic solution 
is to create the BugType lazily. This patch updates the SimpleStreamChecker to 
follow this idiom.

In the future probably it would be better to alter the signature of the 
checkers and set the checker name in the constructor so it is possible to 
create the BugType eagerly.


Repository:
  rL LLVM

https://reviews.llvm.org/D37437

Files:
  lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp


Index: lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
+++ lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
@@ -54,8 +54,8 @@
check::PointerEscape> {
   CallDescription OpenFn, CloseFn;
 
-  std::unique_ptr DoubleCloseBugType;
-  std::unique_ptr LeakBugType;
+  mutable std::unique_ptr DoubleCloseBugType;
+  mutable std::unique_ptr LeakBugType;
 
   void reportDoubleClose(SymbolRef FileDescSym,
  const CallEvent ,
@@ -104,16 +104,7 @@
 } // end anonymous namespace
 
 SimpleStreamChecker::SimpleStreamChecker()
-: OpenFn("fopen"), CloseFn("fclose", 1) {
-  // Initialize the bug types.
-  DoubleCloseBugType.reset(
-  new BugType(this, "Double fclose", "Unix Stream API Error"));
-
-  LeakBugType.reset(
-  new BugType(this, "Resource Leak", "Unix Stream API Error"));
-  // Sinks are higher importance bugs as well as calls to assert() or exit(0).
-  LeakBugType->setSuppressOnSink(true);
-}
+: OpenFn("fopen"), CloseFn("fclose", 1) {}
 
 void SimpleStreamChecker::checkPostCall(const CallEvent ,
 CheckerContext ) const {
@@ -206,6 +197,10 @@
   if (!ErrNode)
 return;
 
+  if (!DoubleCloseBugType)
+DoubleCloseBugType.reset(
+new BugType(this, "Double fclose", "Unix Stream API Error"));
+
   // Generate the report.
   auto R = llvm::make_unique(*DoubleCloseBugType,
   "Closing a previously closed file stream", ErrNode);
@@ -217,6 +212,13 @@
 void SimpleStreamChecker::reportLeaks(ArrayRef LeakedStreams,
   CheckerContext ,
   ExplodedNode *ErrNode) const {
+
+  if (!LeakBugType) {
+LeakBugType.reset(
+new BugType(this, "Resource Leak", "Unix Stream API Error"));
+// Sinks are higher importance bugs as well as calls to assert() or 
exit(0).
+LeakBugType->setSuppressOnSink(true);
+  }
   // Attach bug reports to the leak node.
   // TODO: Identify the leaked file descriptor.
   for (SymbolRef LeakedStream : LeakedStreams) {


Index: lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
+++ lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
@@ -54,8 +54,8 @@
check::PointerEscape> {
   CallDescription OpenFn, CloseFn;
 
-  std::unique_ptr DoubleCloseBugType;
-  std::unique_ptr LeakBugType;
+  mutable std::unique_ptr DoubleCloseBugType;
+  mutable std::unique_ptr LeakBugType;
 
   void reportDoubleClose(SymbolRef FileDescSym,
  const CallEvent ,
@@ -104,16 +104,7 @@
 } // end anonymous namespace
 
 SimpleStreamChecker::SimpleStreamChecker()
-: OpenFn("fopen"), CloseFn("fclose", 1) {
-  // Initialize the bug types.
-  DoubleCloseBugType.reset(
-  new BugType(this, "Double fclose", "Unix Stream API Error"));
-
-  LeakBugType.reset(
-  new BugType(this, "Resource Leak", "Unix Stream API Error"));
-  // Sinks are higher importance bugs as well as calls to assert() or exit(0).
-  LeakBugType->setSuppressOnSink(true);
-}
+: OpenFn("fopen"), CloseFn("fclose", 1) {}
 
 void SimpleStreamChecker::checkPostCall(const CallEvent ,
 CheckerContext ) const {
@@ -206,6 +197,10 @@
   if (!ErrNode)
 return;
 
+  if (!DoubleCloseBugType)
+DoubleCloseBugType.reset(
+new BugType(this, "Double fclose", "Unix Stream API Error"));
+
   // Generate the report.
   auto R = llvm::make_unique(*DoubleCloseBugType,
   "Closing a previously closed file stream", ErrNode);
@@ -217,6 +212,13 @@
 void SimpleStreamChecker::reportLeaks(ArrayRef LeakedStreams,
   CheckerContext ,
   ExplodedNode *ErrNode) const {
+
+  if (!LeakBugType) {
+LeakBugType.reset(
+new BugType(this, "Resource Leak", "Unix Stream API Error"));
+// Sinks are higher importance bugs as well as calls to assert() or exit(0).
+LeakBugType->setSuppressOnSink(true);
+  

[PATCH] D37436: Initial implementation of C attributes (WG14 N2137)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman created this revision.
Herald added a subscriber: javed.absar.

WG14 has demonstrated sincere interest in adding C++-style attributes to C for 
C2x, and have added the proposal to their SD-3 document tracking features which 
are possibly desired for the next version of the standard. The proposal has 
*not* been formally adopted into the working draft (C17 needs to be finalized 
first), but it is at the stage where having implementation experience is 
important to the committee.

This patch implements the feature as proposed, but it might make sense to split 
the patch up, depending on what we think is the correct way to proceed. This 
patch adds `c2x` as a new language standard as well as `-fc-attributes` and 
`-fno-c-attributes` flags. The intention is that `-std=c2x` will enable 
`-fc-attributes` once the feature is formally adopted by WG14, but 
`-fc-attributes` can be used to enable the functionality in earlier standard 
modes (but is currently required in order to enable the functionality). 
`fno-c-attributes` is provided on the chance that the syntax proposed causes 
parsing issues for Objective-C and users need a way to work around that; 
however, such cases should be considered bugs that we need to work around 
similar to Objective-C++ and C++11-style attributes. The patch also adds 
support for the `deprecated` attribute (WG14 N2050), mostly so that I would 
have an attribute handy to test the semantic functionality. Once the basic 
syntax is committed, I plan to implement the other attributes: `nodiscard` 
(WG14 N2051), `fallthrough` (WG14 N2052), and `maybe_unused` (WG14 N2053) in 
separate patches.


https://reviews.llvm.org/D37436

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/Attributes.h
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticParseKinds.td
  include/clang/Basic/LangOptions.def
  include/clang/Driver/Options.td
  include/clang/Frontend/LangStandard.h
  include/clang/Frontend/LangStandards.def
  include/clang/Parse/Parser.h
  include/clang/Sema/AttributeList.h
  lib/Frontend/CompilerInvocation.cpp
  lib/Lex/Lexer.cpp
  lib/Parse/ParseDecl.cpp
  lib/Parse/ParseDeclCXX.cpp
  lib/Parse/ParseExpr.cpp
  lib/Parse/ParseExprCXX.cpp
  lib/Parse/ParseObjc.cpp
  lib/Parse/ParseOpenMP.cpp
  lib/Parse/ParsePragma.cpp
  lib/Parse/ParseStmt.cpp
  lib/Parse/ParseTemplate.cpp
  lib/Parse/ParseTentative.cpp
  lib/Parse/Parser.cpp
  lib/Sema/AttributeList.cpp
  test/CXX/dcl.dcl/dcl.attr/dcl.align/p6.cpp
  test/Driver/unknown-std.c
  test/Misc/ast-dump-c-attr.c
  test/Parser/c2x-attributes.c
  test/Parser/cxx0x-attributes.cpp
  test/Sema/attr-deprecated-c2x.c
  test/Sema/c11-compat.c
  utils/TableGen/ClangAttrEmitter.cpp

Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -58,7 +58,7 @@
 
 assert(V != "GCC" && "Given a GCC spelling, which means this hasn't been"
"flattened!");
-if (V == "CXX11" || V == "Pragma")
+if (V == "CXX11" || V == "C2x" || V == "Pragma")
   NS = Spelling.getValueAsString("Namespace");
 bool Unset;
 K = Spelling.getValueAsBitOrUnset("KnownToGCC", Unset);
@@ -1326,7 +1326,7 @@
 if (Variety == "GNU") {
   Prefix = " __attribute__((";
   Suffix = "))";
-} else if (Variety == "CXX11") {
+} else if (Variety == "CXX11" || Variety == "C2x") {
   Prefix = " [[";
   Suffix = "]]";
   std::string Namespace = Spellings[I].nameSpace();
@@ -2716,10 +2716,14 @@
   // If this is the C++11 variety, also add in the LangOpts test.
   if (Variety == "CXX11")
 Test += " && LangOpts.CPlusPlus11";
+  else if (Variety == "C2x")
+Test += " && LangOpts.C2x && LangOpts.CAttributes";
 } else if (Variety == "CXX11")
   // C++11 mode should be checked against LangOpts, which is presumed to be
   // present in the caller.
   Test = "LangOpts.CPlusPlus11";
+else if (Variety == "C2x")
+  Test = "LangOpts.C2x && LangOpts.CAttributes";
 
 std::string TestStr =
 !Test.empty() ? Test + " ? " + llvm::itostr(Version) + " : 0" : "1";
@@ -2740,7 +2744,7 @@
   // and declspecs. Then generate a big switch statement for each of them.
   std::vector Attrs = Records.getAllDerivedDefinitions("Attr");
   std::vector Declspec, Microsoft, GNU, Pragma;
-  std::map CXX;
+  std::map CXX, C2x;
 
   // Walk over the list of all attributes, and split them out based on the
   // spelling variety.
@@ -2756,6 +2760,8 @@
 Microsoft.push_back(R);
   else if (Variety == "CXX11")
 CXX[SI.nameSpace()].push_back(R);
+  else if (Variety == "C2x")
+C2x[SI.nameSpace()].push_back(R);
   else if (Variety == "Pragma")
 Pragma.push_back(R);
 }
@@ -2775,20 +2781,25 @@
   OS << "case AttrSyntax::Pragma:\n";
   OS << "  return 

r312484 - clang-format: Fix indentation of macros in include guards (after r312125).

[Daniel Jasper via cfe-commits]

Author: djasper
Date: Mon Sep  4 06:33:52 2017
New Revision: 312484

URL: http://llvm.org/viewvc/llvm-project?rev=312484=rev
Log:
clang-format: Fix indentation of macros in include guards (after r312125).

Before:
  #ifndef A_H
  #define A_H

  #define A() \
  int i;\
  int j;

  #endif // A_H

After:
  #ifndef A_H
  #define A_H

  #define A() \
int i;\
int j;

  #endif // A_H

Modified:
cfe/trunk/lib/Format/UnwrappedLineParser.cpp
cfe/trunk/unittests/Format/FormatTest.cpp

Modified: cfe/trunk/lib/Format/UnwrappedLineParser.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineParser.cpp?rev=312484=312483=312484=diff
==
--- cfe/trunk/lib/Format/UnwrappedLineParser.cpp (original)
+++ cfe/trunk/lib/Format/UnwrappedLineParser.cpp Mon Sep  4 06:33:52 2017
@@ -742,12 +742,11 @@ void UnwrappedLineParser::parsePPEndIf()
   // preprocessor indent.
   unsigned TokenPosition = Tokens->getPosition();
   FormatToken *PeekNext = AllTokens[TokenPosition];
-  if (FoundIncludeGuardStart && PPBranchLevel == -1 && PeekNext->is(tok::eof)) 
{
-for (auto  : Lines) {
+  if (FoundIncludeGuardStart && PPBranchLevel == -1 && PeekNext->is(tok::eof) 
&&
+  Style.IndentPPDirectives != FormatStyle::PPDIS_None)
+for (auto  : Lines)
   if (Line.InPPDirective && Line.Level > 0)
 --Line.Level;
-}
-  }
 }
 
 void UnwrappedLineParser::parsePPDefine() {

Modified: cfe/trunk/unittests/Format/FormatTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=312484=312483=312484=diff
==
--- cfe/trunk/unittests/Format/FormatTest.cpp (original)
+++ cfe/trunk/unittests/Format/FormatTest.cpp Mon Sep  4 06:33:52 2017
@@ -2332,7 +2332,6 @@ TEST_F(FormatTest, IndentPreprocessorDir
"#define A 1\n"
"#endif",
Style);
-
   Style.IndentPPDirectives = FormatStyle::PPDIS_AfterHash;
   verifyFormat("#ifdef _WIN32\n"
"#  define A 0\n"
@@ -2493,6 +2492,15 @@ TEST_F(FormatTest, IndentPreprocessorDir
"#\tdefine A 1\n"
"#endif",
Style);
+
+  // Regression test: Multiline-macro inside include guards.
+  verifyFormat("#ifndef HEADER_H\n"
+   "#define HEADER_H\n"
+   "#define A()\\\n"
+   "  int i;   \\\n"
+   "  int j;\n"
+   "#endif // HEADER_H",
+   getLLVMStyleWithColumns(20));
 }
 
 TEST_F(FormatTest, FormatHashIfNotAtStartOfLine) {


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

[PATCH] D37282: clangd: Tolerate additional headers

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In https://reviews.llvm.org/D37282#860239, @puremourning wrote:

> No I don't have commit rights. Can you land for me? Thanks!


Done.


Repository:
  rL LLVM

https://reviews.llvm.org/D37282



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

[clang-tools-extra] r312483 - clangd: Tolerate additional headers

[Ilya Biryukov via cfe-commits]

Author: ibiryukov
Date: Mon Sep  4 05:28:15 2017
New Revision: 312483

URL: http://llvm.org/viewvc/llvm-project?rev=312483=rev
Log:
clangd: Tolerate additional headers

Summary:
The language server protocol specified 2 headers (Content-Length and 
Content-Type), but does not specify their sequence. It specifies that an empty 
line ends
headers. Clangd has been updated to handle arbitrary sequences of headers, 
extracting only the content length.

Patch by puremourning (Ben Jackson).

Reviewers: bkramer, klimek, ilya-biryukov

Reviewed By: ilya-biryukov

Subscribers: cfe-commits, ilya-biryukov

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

Added:
clang-tools-extra/trunk/test/clangd/protocol.test
Modified:
clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp

Modified: clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp?rev=312483=312482=312483=diff
==
--- clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp (original)
+++ clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp Mon Sep  4 05:28:15 
2017
@@ -136,46 +136,68 @@ void clangd::runLanguageServerLoop(std::
JSONRPCDispatcher ,
bool ) {
   while (In.good()) {
-// A Language Server Protocol message starts with a HTTP header, delimited
-// by \r\n.
-std::string Line;
-std::getline(In, Line);
-if (!In.good() && errno == EINTR) {
-  In.clear();
-  continue;
+// A Language Server Protocol message starts with a set of HTTP headers,
+// delimited  by \r\n, and terminated by an empty line (\r\n).
+unsigned long long ContentLength = 0;
+while (In.good()) {
+  std::string Line;
+  std::getline(In, Line);
+  if (!In.good() && errno == EINTR) {
+In.clear();
+continue;
+  }
+
+  llvm::StringRef LineRef(Line);
+
+  // We allow YAML-style comments in headers. Technically this isn't part
+  // of the LSP specification, but makes writing tests easier.
+  if (LineRef.startswith("#"))
+continue;
+
+  // Content-Type is a specified header, but does nothing.
+  // Content-Length is a mandatory header. It specifies the length of the
+  // following JSON.
+  // It is unspecified what sequence headers must be supplied in, so we
+  // allow any sequence.
+  // The end of headers is signified by an empty line.
+  if (LineRef.consume_front("Content-Length: ")) {
+if (ContentLength != 0) {
+  Out.log("Warning: Duplicate Content-Length header received. "
+  "The previous value for this message ("
+  + std::to_string(ContentLength)
+  + ") was ignored.\n");
+}
+
+llvm::getAsUnsignedInteger(LineRef.trim(), 0, ContentLength);
+continue;
+  } else if (!LineRef.trim().empty()) {
+// It's another header, ignore it.
+continue;
+  } else {
+// An empty line indicates the end of headers.
+// Go ahead and read the JSON.
+break;
+  }
 }
 
-// Skip empty lines.
-llvm::StringRef LineRef(Line);
-if (LineRef.trim().empty())
-  continue;
-
-// We allow YAML-style comments. Technically this isn't part of the
-// LSP specification, but makes writing tests easier.
-if (LineRef.startswith("#"))
-  continue;
-
-unsigned long long Len = 0;
-// FIXME: Content-Type is a specified header, but does nothing.
-// Content-Length is a mandatory header. It specifies the length of the
-// following JSON.
-if (LineRef.consume_front("Content-Length: "))
-  llvm::getAsUnsignedInteger(LineRef.trim(), 0, Len);
-
-// Check if the next line only contains \r\n. If not this is another 
header,
-// which we ignore.
-char NewlineBuf[2];
-In.read(NewlineBuf, 2);
-if (std::memcmp(NewlineBuf, "\r\n", 2) != 0)
-  continue;
-
-// Now read the JSON. Insert a trailing null byte as required by the YAML
-// parser.
-std::vector JSON(Len + 1, '\0');
-In.read(JSON.data(), Len);
+if (ContentLength > 0) {
+  // Now read the JSON. Insert a trailing null byte as required by the YAML
+  // parser.
+  std::vector JSON(ContentLength + 1, '\0');
+  In.read(JSON.data(), ContentLength);
+
+  // If the stream is aborted before we read ContentLength bytes, In
+  // will have eofbit and failbit set.
+  if (!In) {
+Out.log("Input was aborted. Read only "
++ std::to_string(In.gcount())
++ " bytes of expected "
++ std::to_string(ContentLength)
++ ".\n");
+break;
+  }
 
-if (Len > 0) {
-  llvm::StringRef JSONRef(JSON.data(), Len);
+  llvm::StringRef JSONRef(JSON.data(), ContentLength);
   // Log the 

[PATCH] D37282: clangd: Tolerate additional headers

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL312483: clangd: Tolerate additional headers (authored by 
ibiryukov).

Repository:
  rL LLVM

https://reviews.llvm.org/D37282

Files:
  clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp
  clang-tools-extra/trunk/test/clangd/protocol.test

Index: clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp
===
--- clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp
+++ clang-tools-extra/trunk/clangd/JSONRPCDispatcher.cpp
@@ -136,46 +136,68 @@
JSONRPCDispatcher ,
bool ) {
   while (In.good()) {
-// A Language Server Protocol message starts with a HTTP header, delimited
-// by \r\n.
-std::string Line;
-std::getline(In, Line);
-if (!In.good() && errno == EINTR) {
-  In.clear();
-  continue;
+// A Language Server Protocol message starts with a set of HTTP headers,
+// delimited  by \r\n, and terminated by an empty line (\r\n).
+unsigned long long ContentLength = 0;
+while (In.good()) {
+  std::string Line;
+  std::getline(In, Line);
+  if (!In.good() && errno == EINTR) {
+In.clear();
+continue;
+  }
+
+  llvm::StringRef LineRef(Line);
+
+  // We allow YAML-style comments in headers. Technically this isn't part
+  // of the LSP specification, but makes writing tests easier.
+  if (LineRef.startswith("#"))
+continue;
+
+  // Content-Type is a specified header, but does nothing.
+  // Content-Length is a mandatory header. It specifies the length of the
+  // following JSON.
+  // It is unspecified what sequence headers must be supplied in, so we
+  // allow any sequence.
+  // The end of headers is signified by an empty line.
+  if (LineRef.consume_front("Content-Length: ")) {
+if (ContentLength != 0) {
+  Out.log("Warning: Duplicate Content-Length header received. "
+  "The previous value for this message ("
+  + std::to_string(ContentLength)
+  + ") was ignored.\n");
+}
+
+llvm::getAsUnsignedInteger(LineRef.trim(), 0, ContentLength);
+continue;
+  } else if (!LineRef.trim().empty()) {
+// It's another header, ignore it.
+continue;
+  } else {
+// An empty line indicates the end of headers.
+// Go ahead and read the JSON.
+break;
+  }
 }
 
-// Skip empty lines.
-llvm::StringRef LineRef(Line);
-if (LineRef.trim().empty())
-  continue;
-
-// We allow YAML-style comments. Technically this isn't part of the
-// LSP specification, but makes writing tests easier.
-if (LineRef.startswith("#"))
-  continue;
-
-unsigned long long Len = 0;
-// FIXME: Content-Type is a specified header, but does nothing.
-// Content-Length is a mandatory header. It specifies the length of the
-// following JSON.
-if (LineRef.consume_front("Content-Length: "))
-  llvm::getAsUnsignedInteger(LineRef.trim(), 0, Len);
-
-// Check if the next line only contains \r\n. If not this is another header,
-// which we ignore.
-char NewlineBuf[2];
-In.read(NewlineBuf, 2);
-if (std::memcmp(NewlineBuf, "\r\n", 2) != 0)
-  continue;
-
-// Now read the JSON. Insert a trailing null byte as required by the YAML
-// parser.
-std::vector JSON(Len + 1, '\0');
-In.read(JSON.data(), Len);
+if (ContentLength > 0) {
+  // Now read the JSON. Insert a trailing null byte as required by the YAML
+  // parser.
+  std::vector JSON(ContentLength + 1, '\0');
+  In.read(JSON.data(), ContentLength);
+
+  // If the stream is aborted before we read ContentLength bytes, In
+  // will have eofbit and failbit set.
+  if (!In) {
+Out.log("Input was aborted. Read only "
++ std::to_string(In.gcount())
++ " bytes of expected "
++ std::to_string(ContentLength)
++ ".\n");
+break;
+  }
 
-if (Len > 0) {
-  llvm::StringRef JSONRef(JSON.data(), Len);
+  llvm::StringRef JSONRef(JSON.data(), ContentLength);
   // Log the message.
   Out.log("<-- " + JSONRef + "\n");
 
@@ -186,6 +208,9 @@
   // If we're done, exit the loop.
   if (IsDone)
 break;
+} else {
+  Out.log( "Warning: Missing Content-Length header, or message has zero "
+   "length.\n" );
 }
   }
 }
Index: clang-tools-extra/trunk/test/clangd/protocol.test
===
--- clang-tools-extra/trunk/test/clangd/protocol.test
+++ clang-tools-extra/trunk/test/clangd/protocol.test
@@ -0,0 +1,82 @@
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# RUN: clangd -run-synchronously < %s 2>&1 | FileCheck -check-prefix=STDERR %s
+# 

[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action

[Manuel Klimek via Phabricator via cfe-commits]

klimek added inline comments.



Comment at: test/Refactor/LocalRename/Field.cpp:4
+class Baz {
+  int /*range=*/Foo; // CHECK: symbol [[@LINE]]:17 -> [[@LINE]]:20
+public:

arphaman wrote:
> klimek wrote:
> > arphaman wrote:
> > > klimek wrote:
> > > > Does this just test the selection?
> > > No, this is the moved `clang-rename/Field.cpp` test that tests 
> > > local-rename. I will move the other tests when this patch is accepted.
> > Ok, then I find this test really hard to read - where does it check what 
> > the symbol was replaced with?
> I thought I should check for occurrences, but you're right, it's probably 
> better to always check the source replacements (we can introduce options that 
> apply non-default occurrences in the future which would allow us to check 
> replacements for occurrences in comments). 
> 
> Would something like `//CHECK: "newName" [[@LINE]]:24 -> [[]]:27` work when 
> checking for replacements?
Yep, I think that'd be better.
Or we could just apply the replacement and check on the replaced code? (I think 
that's what we do in fixits for clang and clang-tidy)


Repository:
  rL LLVM

https://reviews.llvm.org/D36574



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:82
+size_t Pos = Line.find(" ");
+StringRef LineRef{Line};
+if (Pos > 0 && Pos != std::string::npos) {

danielmarjamaki wrote:
> LineRef can be const
StringRef is an immutable reference to a string, I think it is not idiomatic in 
LLVM codebase to make it const. 



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:85
+  FunctionName = LineRef.substr(0, Pos);
+  if (Result.count(FunctionName))
+return llvm::make_error(

danielmarjamaki wrote:
> I would like to see some FunctionName validation. For instance "123" should 
> not be a valid function name.
This is not a real function name but an USR. I updated the name of the variable 
to reflect that this name is only used for lookup. 



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:89
+  FileName = LineRef.substr(Pos + 1);
+  SmallString<256> FilePath = CrossTUDir;
+  llvm::sys::path::append(FilePath, FileName);

danielmarjamaki wrote:
> Stupid question .. how will this work if the path is longer than 256 bytes?
If the path is shorter than 256 bytes it will be stored on stack, otherwise 
`SmallString` will allocate on the heap.



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:125
+CrossTranslationUnitContext::findFunctionInDeclContext(const DeclContext *DC,
+   StringRef LookupFnName) 
{
+  assert(DC && "Declaration Context must not be null");

danielmarjamaki wrote:
> LookupFnName could be const right?
In LLVM we usually do not mark StringRefs as consts, they represent a const 
reference to a string.



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:223
+assert(ToDecl->hasBody());
+assert(FD->hasBody() && "Functions already imported should have body.");
+return ToDecl;

danielmarjamaki wrote:
> sorry I am probably missing something here.. you first assert !FD->hasBody() 
> and here you assert FD->hasBody(). Is it changed in this function somewhere?
Yes, after the importer imports the new declaration with a body we should be 
able to find the body through the original declaration. The importer modifies 
the AST and the supporting data structures. 


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 113740.
xazax.hun marked 19 inline comments as done.
xazax.hun added a comment.

- Make the API capable of using custom lookup strategies for CTU
- Fix review comments


https://reviews.llvm.org/D34512

Files:
  include/clang/Basic/AllDiagnostics.h
  include/clang/Basic/CMakeLists.txt
  include/clang/Basic/Diagnostic.td
  include/clang/Basic/DiagnosticCrossTUKinds.td
  include/clang/Basic/DiagnosticIDs.h
  include/clang/CrossTU/CrossTUDiagnostic.h
  include/clang/CrossTU/CrossTranslationUnit.h
  lib/AST/ASTImporter.cpp
  lib/Basic/DiagnosticIDs.cpp
  lib/CMakeLists.txt
  lib/CrossTU/CMakeLists.txt
  lib/CrossTU/CrossTranslationUnit.cpp
  test/Analysis/func-mapping-test.cpp
  test/CMakeLists.txt
  test/lit.cfg
  tools/CMakeLists.txt
  tools/clang-func-mapping/CMakeLists.txt
  tools/clang-func-mapping/ClangFnMapGen.cpp
  tools/diagtool/DiagnosticNames.cpp
  unittests/CMakeLists.txt
  unittests/CrossTU/CMakeLists.txt
  unittests/CrossTU/CrossTranslationUnitTest.cpp

Index: unittests/CrossTU/CrossTranslationUnitTest.cpp
===
--- /dev/null
+++ unittests/CrossTU/CrossTranslationUnitTest.cpp
@@ -0,0 +1,124 @@
+//===- unittest/Tooling/CrossTranslationUnitTest.cpp - Tooling unit tests -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/CrossTU/CrossTranslationUnit.h"
+#include "clang/AST/ASTConsumer.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/Config/llvm-config.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+#include 
+
+namespace clang {
+namespace cross_tu {
+
+namespace {
+StringRef IndexFileName = "index.txt";
+StringRef ASTFileName = "f.ast";
+StringRef DefinitionFileName = "input.cc";
+
+class CTUASTConsumer : public clang::ASTConsumer {
+public:
+  explicit CTUASTConsumer(clang::CompilerInstance , bool *Success)
+  : CTU(CI), Success(Success) {}
+
+  void HandleTranslationUnit(ASTContext ) {
+const TranslationUnitDecl *TU = Ctx.getTranslationUnitDecl();
+const FunctionDecl *FD = nullptr;
+for (const Decl *D : TU->decls()) {
+  FD = dyn_cast(D);
+  if (FD && FD->getName() == "f")
+break;
+}
+assert(FD && FD->getName() == "f");
+bool OrigFDHasBody = FD->hasBody();
+
+// Prepare the index file and the AST file.
+std::error_code EC;
+llvm::raw_fd_ostream OS(IndexFileName, EC, llvm::sys::fs::F_Text);
+OS << "c:@F@f#I# " << ASTFileName << "\n";
+OS.flush();
+StringRef SourceText = "int f(int) { return 0; }\n";
+// This file must exist since the saved ASTFile will reference it.
+llvm::raw_fd_ostream OS2(DefinitionFileName, EC, llvm::sys::fs::F_Text);
+OS2 << SourceText;
+OS2.flush();
+std::unique_ptr ASTWithDefinition =
+tooling::buildASTFromCode(SourceText);
+ASTWithDefinition->Save(ASTFileName);
+
+// Load the definition from the AST file.
+llvm::Expected NewFDorError =
+CTU.getCrossTUDefinition(FD, ".", IndexFileName);
+assert(NewFDorError);
+const FunctionDecl *NewFD = *NewFDorError;
+
+*Success = NewFD && NewFD->hasBody() && !OrigFDHasBody;
+  }
+
+private:
+  CrossTranslationUnitContext CTU;
+  bool *Success;
+};
+
+class CTUAction : public clang::ASTFrontendAction {
+public:
+  CTUAction(bool *Success) : Success(Success) {}
+
+protected:
+  std::unique_ptr
+  CreateASTConsumer(clang::CompilerInstance , StringRef) override {
+return llvm::make_unique(CI, Success);
+  }
+
+private:
+  bool *Success;
+};
+
+} // end namespace
+
+TEST(CrossTranslationUnit, CanLoadFunctionDefinition) {
+  bool Success = false;
+  EXPECT_TRUE(tooling::runToolOnCode(new CTUAction(), "int f(int);"));
+  EXPECT_TRUE(Success);
+  EXPECT_TRUE(llvm::sys::fs::exists(IndexFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(IndexFileName));
+  EXPECT_TRUE(llvm::sys::fs::exists(ASTFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(ASTFileName));
+  EXPECT_TRUE(llvm::sys::fs::exists(DefinitionFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(DefinitionFileName));
+}
+
+TEST(CrossTranslationUnit, IndexFormatCanBeParsed) {
+  llvm::StringMap Index;
+  Index["a"] = "b";
+  Index["c"] = "d";
+  Index["e"] = "f";
+  std::string IndexText = createCrossTUIndexString(Index);
+  std::error_code EC;
+  llvm::raw_fd_ostream OS(IndexFileName, EC, llvm::sys::fs::F_Text);
+  OS << IndexText;
+  OS.flush();
+  EXPECT_TRUE(llvm::sys::fs::exists(IndexFileName));
+  llvm::Expected IndexOrErr =
+  parseCrossTUIndex(IndexFileName, "");
+  EXPECT_TRUE((bool)IndexOrErr);
+  llvm::StringMap ParsedIndex = IndexOrErr.get();
+  for (const auto  : Index) {
+

[PATCH] D37282: clangd: Tolerate additional headers

[Ben Jackson via Phabricator via cfe-commits]

puremourning added a comment.

In https://reviews.llvm.org/D37282#860200, @ilya-biryukov wrote:

> Looks good and ready to land. Thanks for this change.
>  Do you have commit rights to llvm repo?


Thanks for the review!

No I don't have commit rights. Can you land for me? Thanks!


https://reviews.llvm.org/D37282



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

[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

In https://reviews.llvm.org/D36574#860203, @klimek wrote:

> In https://reviews.llvm.org/D36574#860197, @arphaman wrote:
>
> > In https://reviews.llvm.org/D36574#858763, @klimek wrote:
> >
> > > One of my main concerns is still that I don't see the need for all the 
> > > template magic yet :) Why doesn't everybody use the RefactoringResult we 
> > > define here?
> >
> >
> > @klimek, are you talking about the template usage in this patch or the 
> > whole approach in general? I can probably think of some way to reduce the 
> > template boilerplate if you are talking about the general approach.
>
>
> Talking about the approach in general :)


I'll work on a patch for a simpler solution as well then. It'll probably be 
dependent on this one though. I'm pretty sure I need some template magic to 
figure out how to connect inputs like selection and options and entities that 
produce source replacements , but I could probably get rid of most of 
RefactoringActionRule template magic.




Comment at: include/clang/Tooling/Refactoring/SourceSelectionConstraints.h:42
 
+/// This constraint is satisfied when
+template  struct Decl {

ioeric wrote:
> when?
Oops, this isn't supposed to be in this patch, I will remove.



Comment at: test/Refactor/LocalRename/Field.cpp:4
+class Baz {
+  int /*range=*/Foo; // CHECK: symbol [[@LINE]]:17 -> [[@LINE]]:20
+public:

klimek wrote:
> arphaman wrote:
> > klimek wrote:
> > > Does this just test the selection?
> > No, this is the moved `clang-rename/Field.cpp` test that tests 
> > local-rename. I will move the other tests when this patch is accepted.
> Ok, then I find this test really hard to read - where does it check what the 
> symbol was replaced with?
I thought I should check for occurrences, but you're right, it's probably 
better to always check the source replacements (we can introduce options that 
apply non-default occurrences in the future which would allow us to check 
replacements for occurrences in comments). 

Would something like `//CHECK: "newName" [[@LINE]]:24 -> [[]]:27` work when 
checking for replacements?



Comment at: tools/clang-refactor/ClangRefactor.cpp:60
+  : SubCommand(Name, Description), Action() {
+Sources = llvm::make_unique(
+cl::Positional, cl::ZeroOrMore, cl::desc(" [... ]"),

ioeric wrote:
> I think you would get a conflict of positional args when you have multiple 
> sub-command instances.
> 
> Any reason not to use `clang::tooling::CommonOptionsParser` which takes care 
> of sources and compilation options for you? 
Not from my experience, I've tried multiple actions it seems like the right 
arguments are parsed for the right subcommand. It looks like the cl option 
parser looks is smart enough to handle identical options across multiple 
subcommands.



Comment at: tools/clang-refactor/ClangRefactor.cpp:312
+  [](const std::unique_ptr ) {
+return !!(*SubCommand);
+  });

ioeric wrote:
> Do we need a `FIXME` here?  It seems that this simply finds the first command 
> that is available? What if there are more than one commands that satisfy the 
> requirements?
I don't quite follow, when would multiple subcommands be satisfied? The 
subcommand corresponds to the action that the user wants to do, there should be 
no ambiguity.


Repository:
  rL LLVM

https://reviews.llvm.org/D36574



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

[PATCH] D37101: [clangd] Add support for snippet completions

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Looks good overall, just a bunch of comments before accepting the revision. 
Mostly minor code style issues, sorry for spamming you with those.




Comment at: clangd/ClangdLSPServer.h:30
   ClangdLSPServer(JSONOutput , unsigned AsyncThreadsCount,
+  const bool SnippetCompletions,
   llvm::Optional ResourceDir);

Code style: we don't use `const` for non-reference parameters.



Comment at: clangd/ClangdUnit.cpp:275
 
+std::string SnippetEscape(const llvm::StringRef Text) {
+  std::string Result;

Code style: function names are `lowerCamelCase`.
Also maybe rename to `escapeSnippet` to follow the style guide: "function names 
should be verb phrases, e.g. `openFile()` or `isFoo()`" (see 
https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly)



Comment at: clangd/ClangdUnit.cpp:315
+protected:
+  std::vector 
+  std::shared_ptr Allocator;

Code style: we put all field definitions at the bottom of the class in clangd.
Also: why not make the fields private?




Comment at: clangd/ClangdUnit.cpp:376
+
+CompletionItem Item{InsertTextFormat::PlainText};
+

Implementations of this function in `PlainTextCompletionItemsCollector` and 
`SnippetCompletionItemsCollector` are the same.
Maybe make `ProcessChunks` virtual instead?

Or maybe consider replacing inheritance with a `bool` flag. Inheritance does 
not seem to buy us much here. This looks simpler:
```
if (EnableSnippets)
  ProcessChunksWithSnippets(CCS, Item);
else
  ProcessChunksWithoutSnippets(CCS, Item);

```





Comment at: clangd/ClangdUnit.cpp:454
+// a declarator or macro.
+Item.filterText = Chunk.Text;
+  case CodeCompletionString::CK_Text:

Maybe add a comment that fallthrough to the next case is intended?
It's very easy to miss that there's no break intentionally when reading the 
code.



Comment at: clangd/ClangdUnit.cpp:533
+Item.insertText += Chunk.Text;
+// Don't even add a space to the label.
+  }

Maybe add `break` here?
To avoid unintentional fallthrough if new `case` statement gets added.



Comment at: clangd/Protocol.h:393
+  CompletionItem() = default;
+  CompletionItem(const InsertTextFormat Format);
+

Maybe remove these constructors?
It seems weird that `insertTextFormat` is initialized on construction, while 
all other fields are filled up later.
Having to write all the fields explicitly is pretty low-level, but it's 
consistent and is done in all other structs from `Protocol.h`.



Comment at: clangd/tool/ClangdMain.cpp:31
+static llvm::cl::opt EnableSnippets(
+"enable-snippets",
+llvm::cl::desc("Present completions with embedded snippets instead of "

I've managed to get snippet completion working in VSCode. 
The problem is that we're using outdated version of `vscode-languageclient` 
dependency in our npm module.
Simply bumping versions of dependencies in 
`clangd\clients\clangd-vscode\package.json` worked.
Had to change
```
"dependencies": {
"vscode-languageclient": "^2.6.3",
"vscode-languageserver": "^2.6.2"
},
```
to 
```
"dependencies": {
"vscode-languageclient": "^3.3.0",
"vscode-languageserver": "^3.3.0"
},
```

Maybe include this in this commit and make `-enable-snippets` true by default?


https://reviews.llvm.org/D37101



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

r312479 - Fix MSVC narrowing conversion warning.

[Simon Pilgrim via cfe-commits]

Author: rksimon
Date: Mon Sep  4 03:54:39 2017
New Revision: 312479

URL: http://llvm.org/viewvc/llvm-project?rev=312479=rev
Log:
Fix MSVC narrowing conversion warning.

Modified:
cfe/trunk/lib/Serialization/ASTWriter.cpp

Modified: cfe/trunk/lib/Serialization/ASTWriter.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriter.cpp?rev=312479=312478=312479=diff
==
--- cfe/trunk/lib/Serialization/ASTWriter.cpp (original)
+++ cfe/trunk/lib/Serialization/ASTWriter.cpp Mon Sep  4 03:54:39 2017
@@ -2816,7 +2816,7 @@ void ASTWriter::WriteSubmodules(Module *
   RecordData::value_type Record[] = {SUBMODULE_DEFINITION,
  ID,
  ParentID,
- Mod->Kind,
+ (RecordData::value_type)Mod->Kind,
  Mod->IsFramework,
  Mod->IsExplicit,
  Mod->IsSystem,


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

[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: include/clang/Tooling/Refactoring/RefactoringAction.h:20
+
+class RefactoringAction {
+public:

Please add documentation for the class. 

I appreciate your RFC that explains all these concepts; it would be nice if you 
could move some key concepts into the code documentation since we couldn't 
expect future readers to refer to the RFC. Thanks! ;)



Comment at: include/clang/Tooling/Refactoring/SourceSelectionConstraints.h:42
 
+/// This constraint is satisfied when
+template  struct Decl {

when?



Comment at: include/clang/Tooling/Refactoring/SourceSelectionConstraints.h:55
   /// Subclasses must implement 'T evaluateSelection(SelectionConstraint) 
const'
-  /// member function. \c T is used to determine the return type that is
-  /// passed to the refactoring rule's function.
+  /// or 'T evaluateSelection(const ASTRefactoringContext &,
+  /// SelectionConstraint) const' member function. \c T is used to determine

Can we pass `RefactoringRuleContext` instead of `ASTRefactoringContext` and get 
rules can get AST context from the rule context? I'm a bit nervous about having 
different contexts.

Maybe we should have just one interface `T 
evaluateSelection(RefactoringRuleContext, SelectionConstraint) const`. I think 
the rule context can be useful for all rules.



Comment at: lib/Tooling/Refactoring/Rename/RenamingAction.cpp:35
 
 using namespace llvm;
+using namespace clang;

nit: It's a common practice to wrap implementations in namespaces instead of 
using `using namespace`. It becomes hard to manage scopes/namespaces when there 
are `using namespace`s.



Comment at: tools/clang-refactor/ClangRefactor.cpp:60
+  : SubCommand(Name, Description), Action() {
+Sources = llvm::make_unique(
+cl::Positional, cl::ZeroOrMore, cl::desc(" [... ]"),

I think you would get a conflict of positional args when you have multiple 
sub-command instances.

Any reason not to use `clang::tooling::CommonOptionsParser` which takes care of 
sources and compilation options for you? 



Comment at: tools/clang-refactor/ClangRefactor.cpp:312
+  [](const std::unique_ptr ) {
+return !!(*SubCommand);
+  });

Do we need a `FIXME` here?  It seems that this simply finds the first command 
that is available? What if there are more than one commands that satisfy the 
requirements?



Comment at: tools/clang-refactor/ClangRefactor.cpp:72
+if (HasSelection) {
+  Selection = llvm::make_unique(
+  "selection", cl::desc("Source selection range"), cl::cat(Category),

Same here. There could be potential conflict among subcommands.

Also please provide more detailed description of this option; it is not obvious 
to users what a `selection` is.


Repository:
  rL LLVM

https://reviews.llvm.org/D36574



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

[PATCH] D37282: clangd: Tolerate additional headers

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/JSONRPCDispatcher.cpp:167
+  // It's another header, ignore it.
   continue;
+} else {

puremourning wrote:
> ilya-biryukov wrote:
> > Maybe log the skipped header?
> I think this would just be noise for conforming clients which send the 
> (legitimate) Content-Type header. We could of course special case this, but 
> I'm not sure the extra logging would be all that useful.
You're right. Logging `Content-Type` headers doesn't make any sense.


https://reviews.llvm.org/D37282



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

[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In https://reviews.llvm.org/D36574#860197, @arphaman wrote:

> In https://reviews.llvm.org/D36574#858763, @klimek wrote:
>
> > One of my main concerns is still that I don't see the need for all the 
> > template magic yet :) Why doesn't everybody use the RefactoringResult we 
> > define here?
>
>
> @klimek, are you talking about the template usage in this patch or the whole 
> approach in general? I can probably think of some way to reduce the template 
> boilerplate if you are talking about the general approach.


Talking about the approach in general :)




Comment at: test/Refactor/LocalRename/Field.cpp:4
+class Baz {
+  int /*range=*/Foo; // CHECK: symbol [[@LINE]]:17 -> [[@LINE]]:20
+public:

arphaman wrote:
> klimek wrote:
> > Does this just test the selection?
> No, this is the moved `clang-rename/Field.cpp` test that tests local-rename. 
> I will move the other tests when this patch is accepted.
Ok, then I find this test really hard to read - where does it check what the 
symbol was replaced with?


Repository:
  rL LLVM

https://reviews.llvm.org/D36574



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

[PATCH] D37282: clangd: Tolerate additional headers

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

Looks good and ready to land. Thanks for this change.
Do you have commit rights to llvm repo?


https://reviews.llvm.org/D37282



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

[PATCH] D37390: [diagtool] Change default tree behavior to print only flags

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere updated this revision to Diff 113737.
JDevlieghere added a comment.

Thanks Alex!


https://reviews.llvm.org/D37390

Files:
  test/Misc/warning-flags-tree.c
  tools/diagtool/TreeView.cpp

Index: tools/diagtool/TreeView.cpp
===
--- tools/diagtool/TreeView.cpp
+++ tools/diagtool/TreeView.cpp
@@ -32,10 +32,10 @@
 public:
   llvm::raw_ostream 
   const bool ShowColors;
-  bool FlagsOnly;
+  bool Internal;
 
   TreePrinter(llvm::raw_ostream )
-  : out(out), ShowColors(hasColors(out)), FlagsOnly(false) {}
+  : out(out), ShowColors(hasColors(out)), Internal(false) {}
 
   void setColor(llvm::raw_ostream::Colors Color) {
 if (ShowColors)
@@ -54,23 +54,41 @@
 return Diags.isIgnored(DiagID, SourceLocation());
   }
 
+  static bool enabledByDefault(const GroupRecord ) {
+for (auto I = Group.diagnostics_begin(), E = Group.diagnostics_end();
+ I != E; ++I) {
+  if (isIgnored(I->DiagID))
+return false;
+}
+
+for (auto I = Group.subgroup_begin(), E = Group.subgroup_end(); I != E;
+ ++I) {
+  if (!enabledByDefault(*I))
+return false;
+}
+
+return true;
+  }
+
   void printGroup(const GroupRecord , unsigned Indent = 0) {
 out.indent(Indent * 2);
 
-setColor(llvm::raw_ostream::YELLOW);
+if (enabledByDefault(Group))
+  setColor(llvm::raw_ostream::GREEN);
+else
+  setColor(llvm::raw_ostream::YELLOW);
+
 out << "-W" << Group.getName() << "\n";
 resetColor();
 
 ++Indent;
-for (GroupRecord::subgroup_iterator I = Group.subgroup_begin(),
-E = Group.subgroup_end();
- I != E; ++I) {
+for (auto I = Group.subgroup_begin(), E = Group.subgroup_end(); I != E;
+ ++I) {
   printGroup(*I, Indent);
 }
 
-if (!FlagsOnly) {
-  for (GroupRecord::diagnostics_iterator I = Group.diagnostics_begin(),
- E = Group.diagnostics_end();
+if (Internal) {
+  for (auto I = Group.diagnostics_begin(), E = Group.diagnostics_end();
I != E; ++I) {
 if (ShowColors && !isIgnored(I->DiagID))
   setColor(llvm::raw_ostream::GREEN);
@@ -107,12 +125,9 @@
 ArrayRef AllGroups = getDiagnosticGroups();
 llvm::DenseSet NonRootGroupIDs;
 
-for (ArrayRef::iterator I = AllGroups.begin(),
- E = AllGroups.end();
- I != E; ++I) {
-  for (GroupRecord::subgroup_iterator SI = I->subgroup_begin(),
-  SE = I->subgroup_end();
-   SI != SE; ++SI) {
+for (auto I = AllGroups.begin(), E = AllGroups.end(); I != E; ++I) {
+  for (auto SI = I->subgroup_begin(), SE = I->subgroup_end(); SI != SE;
+   ++SI) {
 NonRootGroupIDs.insert((unsigned)SI.getID());
   }
 }
@@ -139,16 +154,16 @@
 };
 
 static void printUsage() {
-  llvm::errs() << "Usage: diagtool tree [--flags-only] []\n";
+  llvm::errs() << "Usage: diagtool tree [--internal] []\n";
 }
 
 int TreeView::run(unsigned int argc, char **argv, llvm::raw_ostream ) {
   // First check our one flag (--flags-only).
-  bool FlagsOnly = false;
+  bool Internal = false;
   if (argc > 0) {
 StringRef FirstArg(*argv);
-if (FirstArg.equals("--flags-only")) {
-  FlagsOnly = true;
+if (FirstArg.equals("--internal")) {
+  Internal = true;
   --argc;
   ++argv;
 }
@@ -175,7 +190,7 @@
   }
 
   TreePrinter TP(out);
-  TP.FlagsOnly = FlagsOnly;
+  TP.Internal = Internal;
   TP.showKey();
   return ShowAll ? TP.showAll() : TP.showGroup(RootGroup);
 }
Index: test/Misc/warning-flags-tree.c
===
--- test/Misc/warning-flags-tree.c
+++ test/Misc/warning-flags-tree.c
@@ -1,6 +1,6 @@
-// RUN: diagtool tree | FileCheck -strict-whitespace %s
-// RUN: diagtool tree -Weverything | FileCheck -strict-whitespace %s
-// RUN: diagtool tree everything | FileCheck -strict-whitespace %s
+// RUN: diagtool tree --internal | FileCheck -strict-whitespace %s
+// RUN: diagtool tree --internal -Weverything | FileCheck -strict-whitespace %s
+// RUN: diagtool tree --internal everything | FileCheck -strict-whitespace %s
 //
 // These three ways of running diagtool tree are the same:
 // they produce a tree for every top-level diagnostic flag.
@@ -30,7 +30,7 @@
 // RUN: not diagtool tree -Wthis-is-not-a-valid-flag
 
 
-// RUN: diagtool tree -Wgnu | FileCheck -strict-whitespace -check-prefix CHECK-GNU %s
+// RUN: diagtool tree --internal -Wgnu | FileCheck -strict-whitespace -check-prefix CHECK-GNU %s
 // CHECK-GNU: -Wgnu
 // CHECK-GNU:   -Wgnu-designator
 // CHECK-GNU: ext_gnu_array_range
@@ -40,7 +40,7 @@
 // CHECK-GNU: ext_vla
 // There are more GNU extensions but we don't need to check them all.
 
-// RUN: diagtool tree --flags-only -Wgnu | FileCheck -check-prefix CHECK-FLAGS-ONLY %s
+// RUN: diagtool 

[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

In https://reviews.llvm.org/D36574#858763, @klimek wrote:

> One of my main concerns is still that I don't see the need for all the 
> template magic yet :) Why doesn't everybody use the RefactoringResult we 
> define here?


@klimek, are you talking about the template usage in this patch or the whole 
approach in general? I can probably think of some way to reduce the template 
boilerplate if you are talking about the general approach.


Repository:
  rL LLVM

https://reviews.llvm.org/D36574



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

[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action

[Alex Lorenz via Phabricator via cfe-commits]

arphaman updated this revision to Diff 113736.
arphaman marked an inline comment as done.
arphaman added a comment.

Get rid of inheritance of `RefactoringEngine` by getting rid of the class 
itself.


Repository:
  rL LLVM

https://reviews.llvm.org/D36574

Files:
  include/clang/Tooling/Refactoring/RefactoringAction.h
  include/clang/Tooling/Refactoring/RefactoringActionRegistry.def
  include/clang/Tooling/Refactoring/RefactoringActionRule.h
  include/clang/Tooling/Refactoring/RefactoringActionRuleRequirementsInternal.h
  include/clang/Tooling/Refactoring/RefactoringActionRules.h
  include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h
  include/clang/Tooling/Refactoring/RefactoringRuleContext.h
  include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
  include/clang/Tooling/Refactoring/Rename/USRFindingAction.h
  include/clang/Tooling/Refactoring/SourceSelectionConstraints.h
  include/clang/module.modulemap
  lib/Tooling/Refactoring/CMakeLists.txt
  lib/Tooling/Refactoring/RefactoringActions.cpp
  lib/Tooling/Refactoring/Rename/RenamingAction.cpp
  lib/Tooling/Refactoring/Rename/USRFindingAction.cpp
  test/CMakeLists.txt
  test/Refactor/LocalRename/Field.cpp
  test/Refactor/tool-common-options.c
  test/Refactor/tool-test-support.c
  test/clang-rename/Field.cpp
  tools/CMakeLists.txt
  tools/clang-refactor/CMakeLists.txt
  tools/clang-refactor/ClangRefactor.cpp
  tools/clang-refactor/DumpRefactoringResult.cpp
  tools/clang-refactor/RefactoringResult.h
  tools/clang-refactor/TestSupport.cpp
  tools/clang-refactor/TestSupport.h

Index: tools/clang-refactor/TestSupport.h
===
--- /dev/null
+++ tools/clang-refactor/TestSupport.h
@@ -0,0 +1,105 @@
+//===--- TestSupport.h - Clang-based refactoring tool ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+///
+/// \file
+/// \brief Declares datatypes and routines that are used by test-specific code
+/// in clang-refactor.
+///
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_CLANG_REFACTOR_TEST_SUPPORT_H
+#define LLVM_CLANG_TOOLS_CLANG_REFACTOR_TEST_SUPPORT_H
+
+#include "RefactoringResult.h"
+#include "clang/Basic/LLVM.h"
+#include "clang/Basic/SourceLocation.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/Support/Error.h"
+#include 
+#include 
+
+namespace clang {
+
+class SourceManager;
+
+namespace clang_refactor {
+
+/// A source selection range that's specified in a test file using an inline
+/// command in the comment. These commands can take the following forms:
+///
+/// - /*range=*/ will create an empty selection range in the default group
+///   right after the comment.
+/// - /*range a=*/ will create an empty selection range in the 'a' group right
+///   after the comment.
+/// - /*range = +1*/ will create an empty selection range at a location that's
+///   right after the comment with one offset to the column.
+/// - /*range= -> +2:3*/ will create a selection range that starts at the
+///   location right after the comment, and ends at column 3 of the 2nd line
+///   after the line of the starting location.
+///
+/// Clang-refactor will expected all ranges in one test group to produce
+/// identical results.
+struct TestSelectionRange {
+  unsigned Begin, End;
+};
+
+/// A set of test selection ranges specified in one file.
+struct TestSelectionRangesInFile {
+  std::string Filename;
+  std::map> GroupedRanges;
+
+  TestSelectionRangesInFile(TestSelectionRangesInFile &&) = default;
+  TestSelectionRangesInFile =(TestSelectionRangesInFile &&) = default;
+
+  bool dispatch(
+  const SourceManager ,
+  llvm::function_ref<
+  Expected(SourceRange)>
+  Producer) const;
+
+  void dump(llvm::raw_ostream ) const;
+};
+
+/// Extracts the grouped selection ranges from the file that's specified in
+/// the -selection=test: option.
+///
+/// The grouped ranges are specified in comments using the following syntax:
+/// "range" [ group-name ] "=" [ "+" starting-column-offset ] [ "->"
+///  "+" ending-line-offset ":"
+///  ending-column-position ]
+///
+/// The selection range is then computed from this command by taking the ending
+/// location of the comment, and adding 'starting-column-offset' to the column
+/// for that location. That location in turns becomes the whole selection range,
+/// unless 'ending-line-offset' and 'ending-column-position' are specified. If
+/// they are specified, then the ending location of the selection range is
+/// the starting location's line + 'ending-line-offset' 

[PATCH] D37390: [diagtool] Change default tree behavior to print only flags

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

The diagtool tests should go to `test/Misc`, and there's an existing `diagtool 
tree` test there that has to be updated as well.


Repository:
  rL LLVM

https://reviews.llvm.org/D37390



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

[PATCH] D37390: [diagtool] Change default tree behavior to print only flags

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere updated this revision to Diff 113731.
JDevlieghere added a comment.
Herald added subscribers: aheejin, klimek.

Thanks for the review Adrian!

I somehow completely overlooked the existing `--flags-only` option in `diagtool 
tree`. Obviously it makes much more sense to implement printing flags here and 
make it the default behavior. I've also added some code to print the flags with 
the appropriate color, i.e. defaults are printed in green while the others 
remain printed in yellow as before.

I've updated the diff to reflect this and added a test case.


Repository:
  rL LLVM

https://reviews.llvm.org/D37390

Files:
  test/Tooling/diatool.test
  tools/diagtool/TreeView.cpp

Index: tools/diagtool/TreeView.cpp
===
--- tools/diagtool/TreeView.cpp
+++ tools/diagtool/TreeView.cpp
@@ -32,10 +32,10 @@
 public:
   llvm::raw_ostream 
   const bool ShowColors;
-  bool FlagsOnly;
+  bool Internal;
 
   TreePrinter(llvm::raw_ostream )
-  : out(out), ShowColors(hasColors(out)), FlagsOnly(false) {}
+  : out(out), ShowColors(hasColors(out)), Internal(false) {}
 
   void setColor(llvm::raw_ostream::Colors Color) {
 if (ShowColors)
@@ -54,23 +54,41 @@
 return Diags.isIgnored(DiagID, SourceLocation());
   }
 
+  static bool enabledByDefault(const GroupRecord ) {
+for (auto I = Group.diagnostics_begin(), E = Group.diagnostics_end();
+ I != E; ++I) {
+  if (isIgnored(I->DiagID))
+return false;
+}
+
+for (auto I = Group.subgroup_begin(), E = Group.subgroup_end(); I != E;
+ ++I) {
+  if (!enabledByDefault(*I))
+return false;
+}
+
+return true;
+  }
+
   void printGroup(const GroupRecord , unsigned Indent = 0) {
 out.indent(Indent * 2);
 
-setColor(llvm::raw_ostream::YELLOW);
+if (enabledByDefault(Group))
+  setColor(llvm::raw_ostream::GREEN);
+else
+  setColor(llvm::raw_ostream::YELLOW);
+
 out << "-W" << Group.getName() << "\n";
 resetColor();
 
 ++Indent;
-for (GroupRecord::subgroup_iterator I = Group.subgroup_begin(),
-E = Group.subgroup_end();
- I != E; ++I) {
+for (auto I = Group.subgroup_begin(), E = Group.subgroup_end(); I != E;
+ ++I) {
   printGroup(*I, Indent);
 }
 
-if (!FlagsOnly) {
-  for (GroupRecord::diagnostics_iterator I = Group.diagnostics_begin(),
- E = Group.diagnostics_end();
+if (Internal) {
+  for (auto I = Group.diagnostics_begin(), E = Group.diagnostics_end();
I != E; ++I) {
 if (ShowColors && !isIgnored(I->DiagID))
   setColor(llvm::raw_ostream::GREEN);
@@ -107,12 +125,9 @@
 ArrayRef AllGroups = getDiagnosticGroups();
 llvm::DenseSet NonRootGroupIDs;
 
-for (ArrayRef::iterator I = AllGroups.begin(),
- E = AllGroups.end();
- I != E; ++I) {
-  for (GroupRecord::subgroup_iterator SI = I->subgroup_begin(),
-  SE = I->subgroup_end();
-   SI != SE; ++SI) {
+for (auto I = AllGroups.begin(), E = AllGroups.end(); I != E; ++I) {
+  for (auto SI = I->subgroup_begin(), SE = I->subgroup_end(); SI != SE;
+   ++SI) {
 NonRootGroupIDs.insert((unsigned)SI.getID());
   }
 }
@@ -139,16 +154,16 @@
 };
 
 static void printUsage() {
-  llvm::errs() << "Usage: diagtool tree [--flags-only] []\n";
+  llvm::errs() << "Usage: diagtool tree [--internal] []\n";
 }
 
 int TreeView::run(unsigned int argc, char **argv, llvm::raw_ostream ) {
   // First check our one flag (--flags-only).
-  bool FlagsOnly = false;
+  bool Internal = false;
   if (argc > 0) {
 StringRef FirstArg(*argv);
-if (FirstArg.equals("--flags-only")) {
-  FlagsOnly = true;
+if (FirstArg.equals("--internal")) {
+  Internal = true;
   --argc;
   ++argv;
 }
@@ -175,7 +190,7 @@
   }
 
   TreePrinter TP(out);
-  TP.FlagsOnly = FlagsOnly;
+  TP.Internal = Internal;
   TP.showKey();
   return ShowAll ? TP.showAll() : TP.showGroup(RootGroup);
 }
Index: test/Tooling/diatool.test
===
--- /dev/null
+++ test/Tooling/diatool.test
@@ -0,0 +1,816 @@
+RUN: diagtool tree | FileCheck %s
+
+CHECK: -W
+CHECK:   -Wextra
+CHECK: -Wmissing-field-initializers
+CHECK: -Wignored-qualifiers
+CHECK: -Winitializer-overrides
+CHECK: -Wsemicolon-before-method-body
+CHECK: -Wmissing-method-return-type
+CHECK: -Wsign-compare
+CHECK: -Wunused-parameter
+CHECK: -W#pragma-messages
+CHECK: -WCFString-literal
+CHECK: -WCL4
+CHECK:   -Wall
+CHECK: -Wmost
+CHECK:   -Wchar-subscripts
+CHECK:   -Wcomment
+CHECK:   -Wdelete-non-virtual-dtor
+CHECK:   -Wfor-loop-analysis
+CHECK:   -Wformat
+CHECK: -Wformat-extra-args
+CHECK: -Wformat-zero-length
+CHECK:

[PATCH] D36390: Fix overloaded static functions in SemaCodeComplete

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan added a comment.

ping


https://reviews.llvm.org/D36390



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

Re: [PATCH] D36562: [Bitfield] Make the bitfield a separate location if it has width of legal integer type and its bit offset is naturally aligned for the type

[Chandler Carruth via cfe-commits]

On Sun, Sep 3, 2017 at 10:41 PM Hal Finkel via llvm-commits <
llvm-comm...@lists.llvm.org> wrote:

> Nevertheless, I think that you've convinced me that this is a least-bad
> solution. I'll want a flag preserving the old behavior. Something like
> -fwiden-bitfield-accesses (modulo bikeshedding).
>
Several different approaches have been discussed in this thread, I'm not
sure what you mean about "least-bad solution"...

I remain completely unconvinced that we should change the default behavior.
At most, I'm not strongly opposed to adding an attribute that indicates
"please try to use narrow loads for this bitfield member" and is an error
if applied to a misaligned or non-byte-sized bitfield member. But I remain
strongly opposed to changing the default behavior. We have one or two cases
that regress and are easily addressed by source changes (either to not use
bitfields or to use an attribute). I don't think that is cause to change
the lowering Clang has used for years and potentially regress many other
use cases.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36998: [AST] Traverse templates in LexicallyOrderedRecursiveASTVisitor

[Alex Lorenz via Phabricator via cfe-commits]

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

LGTM


https://reviews.llvm.org/D36998



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

[PATCH] D37383: [AST] Add TableGen for StmtDataCollectors

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

LGTM with one inline comment below:




Comment at: utils/TableGen/TableGen.cpp:151
 clEnumValN(GenOptDocs, "gen-opt-docs", "Generate option 
documentation"),
+clEnumValN(GenDataCollectors, "gen-clang-data-collectors", "Generate 
data collectors for AST nodes"),
 clEnumValN(GenTestPragmaAttributeSupportedAttributes,

Looks like an 80 column violation, please reformat this line.


https://reviews.llvm.org/D37383



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

[PATCH] D37383: [AST] Add TableGen for StmtDataCollectors

[Alex Lorenz via Phabricator via cfe-commits]

arphaman accepted this revision.
arphaman added a comment.

`def`s could work, but I think that we should stick with TableGen. As you've 
said, we'd be able to introduce some sort of structure instead of just code in 
the future which will be better (Ideally we'd try to do it now, but given the 
fact that the GSoC is done the current approach is acceptable). I would also 
like to see this analyzed and tested better in the future to ensure a complete 
coverage of AST, and I think the use of TableGen can potentially help with that.


https://reviews.llvm.org/D37383



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

[PATCH] D37231: Add half load and store builtins

[Jan Vesely via Phabricator via cfe-commits]

jvesely requested review of this revision.
jvesely added a comment.

please let me know if your accept still stands for the modified version.


Repository:
  rL LLVM

https://reviews.llvm.org/D37231



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

[PATCH] D37433: [cxx_status] Mark coroutine TS support as clang 5.0 and change class from svn to full for P0096R4

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

rsmith added a comment.

Part of this change (marking the coroutines TS as "Clang 5") is already done, 
and we will change all the `class="svn"` entries on this page to `class="full"` 
when Clang 5 releases (we keep them in the yellow "SVN" color until they're in 
a released version of Clang).

Changing the paper number from N4663 to N4680 seems fine to me, though :)


https://reviews.llvm.org/D37433



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