date:20201117

[PATCH] D69094: [WIP] Add override relationship.

2020-11-17 Thread Haojian Wu via Phabricator via cfe-commits

hokein abandoned this revision.
hokein added a comment.
Herald added a subscriber: yaxunl.

closing it in favor of https://reviews.llvm.org/D91610.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69094

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

[PATCH] D91519: [AST][Mach0] Fix unused-variable warnings

2020-11-17 Thread Gabriel Hjort Åkerlund via Phabricator via cfe-commits

ehjogab updated this revision to Diff 305984.
ehjogab added a comment.

Replace dyn_cast with isa


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91519

Files:
  clang/lib/AST/APValue.cpp
  lld/MachO/SymbolTable.cpp


Index: lld/MachO/SymbolTable.cpp
===
--- lld/MachO/SymbolTable.cpp
+++ lld/MachO/SymbolTable.cpp
@@ -134,7 +134,7 @@
 // FIXME: Make every symbol (including absolute symbols) contain a
 // reference to their originating file, then add that file name to this
 // error message.
-if (auto *defined = dyn_cast(s))
+if (isa(s))
   error("found defined symbol with illegal name " + DSOHandle::name);
   }
   replaceSymbol(s, header);
Index: clang/lib/AST/APValue.cpp
===
--- clang/lib/AST/APValue.cpp
+++ clang/lib/AST/APValue.cpp
@@ -1062,7 +1062,7 @@
   }
 
   case APValue::Union:
-if (const auto *FD = V.getUnionField())
+if (V.getUnionField())
   Merge(V.getUnionValue());
 break;
 


Index: lld/MachO/SymbolTable.cpp
===
--- lld/MachO/SymbolTable.cpp
+++ lld/MachO/SymbolTable.cpp
@@ -134,7 +134,7 @@
 // FIXME: Make every symbol (including absolute symbols) contain a
 // reference to their originating file, then add that file name to this
 // error message.
-if (auto *defined = dyn_cast(s))
+if (isa(s))
   error("found defined symbol with illegal name " + DSOHandle::name);
   }
   replaceSymbol(s, header);
Index: clang/lib/AST/APValue.cpp
===
--- clang/lib/AST/APValue.cpp
+++ clang/lib/AST/APValue.cpp
@@ -1062,7 +1062,7 @@
   }
 
   case APValue::Union:
-if (const auto *FD = V.getUnionField())
+if (V.getUnionField())
   Merge(V.getUnionValue());
 break;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72184: [BPF] support atomic instructions

2020-11-17 Thread Yonghong Song via Phabricator via cfe-commits

yonghong-song added a comment.

make sense. will add and/or/xor support in the next revision. will also think 
about how to support hardware-level barrier. Totally agree that we should have 
adequate atomic op support in cpu=v4.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72184

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

[PATCH] D91327: [NewPM] Redesign of PreserveCFG Checker

2020-11-17 Thread Serguei Katkov via Phabricator via cfe-commits

skatkov added inline comments.



Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:769
+auto *F = any_cast(IR);
+if (auto *Result = FAM.getCachedResult(
+*const_cast(F)))

Result -> GraphBefore 



Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:771
+*const_cast(F)))
+  checkCFG(P, F->getName(), *Result, CFG(F, false /* TrackBBLifetime */));
 else

why do you check before pass runs?



Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:778
   [this](StringRef P, const PreservedAnalyses &PassPA) {
-auto Before = GraphStackBefore.pop_back_val();
-assert(Before.first == P &&
+assert(PassStack.pop_back_val() == P &&
"Before and After callbacks must correspond");

you should not PassStack.pop_back_val() for product build?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91327

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

[PATCH] D91610: [clangd] Add OverriddenBy Relation to index.

2020-11-17 Thread Utkarsh Saxena via Phabricator via cfe-commits

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG4bc085f5b3ed: [clangd] Add OverridenBy Relation to index. 
(authored by usaxena95).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91610

Files:
  clang-tools-extra/clangd/index/Relation.cpp
  clang-tools-extra/clangd/index/Relation.h
  clang-tools-extra/clangd/index/Serialization.cpp
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/test/index-serialization/Inputs/sample.cpp
  clang-tools-extra/clangd/test/index-serialization/Inputs/sample.h
  clang-tools-extra/clangd/test/index-serialization/Inputs/sample.idx
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -97,6 +97,9 @@
   const Range &Range = ::testing::get<1>(arg);
   return rangesMatch(Pos.Location, Range);
 }
+MATCHER_P2(OverriddenBy, Subject, Object, "") {
+  return arg == Relation{Subject.ID, RelationKind::OverriddenBy, Object.ID};
+}
 ::testing::Matcher &>
 HaveRanges(const std::vector Ranges) {
   return ::testing::UnorderedPointwise(RefRange(), Ranges);
@@ -1031,7 +1034,7 @@
   HaveRanges(Header.ranges("macro");
 }
 
-TEST_F(SymbolCollectorTest, Relations) {
+TEST_F(SymbolCollectorTest, BaseOfRelations) {
   std::string Header = R"(
   class Base {};
   class Derived : public Base {};
@@ -1043,6 +1046,77 @@
   Contains(Relation{Base.ID, RelationKind::BaseOf, Derived.ID}));
 }
 
+TEST_F(SymbolCollectorTest, OverrideRelationsSimpleInheritance) {
+  std::string Header = R"cpp(
+class A {
+  virtual void foo();
+};
+class B : public A {
+  void foo() override;  // A::foo
+  virtual void bar();
+};
+class C : public B {
+  void bar() override;  // B::bar
+};
+class D: public C {
+  void foo() override;  // B::foo
+  void bar() override;  // C::bar
+};
+  )cpp";
+  runSymbolCollector(Header, /*Main=*/"");
+  const Symbol &AFoo = findSymbol(Symbols, "A::foo");
+  const Symbol &BFoo = findSymbol(Symbols, "B::foo");
+  const Symbol &DFoo = findSymbol(Symbols, "D::foo");
+
+  const Symbol &BBar = findSymbol(Symbols, "B::bar");
+  const Symbol &CBar = findSymbol(Symbols, "C::bar");
+  const Symbol &DBar = findSymbol(Symbols, "D::bar");
+
+  std::vector Result;
+  for (const Relation &R : Relations)
+if (R.Predicate == RelationKind::OverriddenBy)
+  Result.push_back(R);
+  EXPECT_THAT(Result, UnorderedElementsAre(
+  OverriddenBy(AFoo, BFoo), OverriddenBy(BBar, CBar),
+  OverriddenBy(BFoo, DFoo), OverriddenBy(CBar, DBar)));
+}
+
+TEST_F(SymbolCollectorTest, OverrideRelationsMultipleInheritance) {
+  std::string Header = R"cpp(
+class A {
+  virtual void foo();
+};
+class B {
+  virtual void bar();
+};
+class C : public B {
+  void bar() override;  // B::bar
+  virtual void baz();
+}
+class D : public A, C {
+  void foo() override;  // A::foo
+  void bar() override;  // C::bar
+  void baz() override;  // C::baz
+};
+  )cpp";
+  runSymbolCollector(Header, /*Main=*/"");
+  const Symbol &AFoo = findSymbol(Symbols, "A::foo");
+  const Symbol &BBar = findSymbol(Symbols, "B::bar");
+  const Symbol &CBar = findSymbol(Symbols, "C::bar");
+  const Symbol &CBaz = findSymbol(Symbols, "C::baz");
+  const Symbol &DFoo = findSymbol(Symbols, "D::foo");
+  const Symbol &DBar = findSymbol(Symbols, "D::bar");
+  const Symbol &DBaz = findSymbol(Symbols, "D::baz");
+
+  std::vector Result;
+  for (const Relation &R : Relations)
+if (R.Predicate == RelationKind::OverriddenBy)
+  Result.push_back(R);
+  EXPECT_THAT(Result, UnorderedElementsAre(
+  OverriddenBy(BBar, CBar), OverriddenBy(AFoo, DFoo),
+  OverriddenBy(CBar, DBar), OverriddenBy(CBaz, DBaz)));
+}
+
 TEST_F(SymbolCollectorTest, CountReferences) {
   const std::string Header = R"(
 class W;
Index: clang-tools-extra/clangd/test/index-serialization/Inputs/sample.h
===
--- clang-tools-extra/clangd/test/index-serialization/Inputs/sample.h
+++ clang-tools-extra/clangd/test/index-serialization/Inputs/sample.h
@@ -1,4 +1,6 @@
 #pragma once
 
 // Introduce a symbol.
-struct Foo {};
+struct Foo {
+  virtual void Func() {}
+};
Index: clang-tools-extra/clangd/test/index-serialization/Inputs/sample.cpp
===
--- clang-tools-extra/clangd/test/index-serialization/Inputs/sample.cpp
+++ clang-tools-extr

[clang-tools-extra] 4bc085f - [clangd] Add OverridenBy Relation to index.

2020-11-17 Thread Utkarsh Saxena via cfe-commits


Author: Utkarsh Saxena
Date: 2020-11-18T06:59:49+01:00
New Revision: 4bc085f5b3eda5273721fd787ffa65e2f155fc45

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

LOG: [clangd] Add OverridenBy Relation to index.

This was previously explored in reviews.llvm.org/D69094.

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

Added: 


Modified: 
clang-tools-extra/clangd/index/Relation.cpp
clang-tools-extra/clangd/index/Relation.h
clang-tools-extra/clangd/index/Serialization.cpp
clang-tools-extra/clangd/index/SymbolCollector.cpp
clang-tools-extra/clangd/test/index-serialization/Inputs/sample.cpp
clang-tools-extra/clangd/test/index-serialization/Inputs/sample.h
clang-tools-extra/clangd/test/index-serialization/Inputs/sample.idx
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

Removed: 




diff  --git a/clang-tools-extra/clangd/index/Relation.cpp 
b/clang-tools-extra/clangd/index/Relation.cpp
index 6daa10a6f95e..2e5ae33939b0 100644
--- a/clang-tools-extra/clangd/index/Relation.cpp
+++ b/clang-tools-extra/clangd/index/Relation.cpp
@@ -13,6 +13,20 @@
 namespace clang {
 namespace clangd {
 
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const RelationKind R) {
+  switch (R) {
+  case RelationKind::BaseOf:
+return OS << "BaseOf";
+  case RelationKind::OverriddenBy:
+return OS << "OverriddenBy";
+  }
+  llvm_unreachable("Unhandled RelationKind enum.");
+}
+
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Relation &R) {
+  return OS << R.Subject << " " << R.Predicate << " " << R.Object;
+}
+
 llvm::iterator_range
 RelationSlab::lookup(const SymbolID &Subject, RelationKind Predicate) const {
   auto IterPair = std::equal_range(Relations.begin(), Relations.end(),

diff  --git a/clang-tools-extra/clangd/index/Relation.h 
b/clang-tools-extra/clangd/index/Relation.h
index 36319e70..b0326ac6eae0 100644
--- a/clang-tools-extra/clangd/index/Relation.h
+++ b/clang-tools-extra/clangd/index/Relation.h
@@ -21,11 +21,16 @@ namespace clangd {
 
 enum class RelationKind : uint8_t {
   BaseOf,
+  OverriddenBy,
 };
 
 /// Represents a relation between two symbols.
-/// For an example "A is a base class of B" may be represented
-/// as { Subject = A, Predicate = BaseOf, Object = B }.
+/// For an example:
+///   - "A is a base class of B" is represented as
+/// { Subject = A, Predicate = BaseOf, Object = B }.
+///   - "Derived::Foo overrides Base::Foo" is represented as
+/// { Subject = Base::Foo, Predicate = OverriddenBy, Object = Derived::Foo
+/// }.
 struct Relation {
   SymbolID Subject;
   RelationKind Predicate;
@@ -41,6 +46,8 @@ struct Relation {
std::tie(Other.Subject, Other.Predicate, Other.Object);
   }
 };
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const RelationKind R);
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const Relation &R);
 
 class RelationSlab {
 public:

diff  --git a/clang-tools-extra/clangd/index/Serialization.cpp 
b/clang-tools-extra/clangd/index/Serialization.cpp
index 8b0ae3925a2f..5d48edf81343 100644
--- a/clang-tools-extra/clangd/index/Serialization.cpp
+++ b/clang-tools-extra/clangd/index/Serialization.cpp
@@ -443,7 +443,7 @@ readCompileCommand(Reader CmdReader, 
llvm::ArrayRef Strings) {
 // The current versioning scheme is simple - non-current versions are rejected.
 // If you make a breaking change, bump this version number to invalidate stored
 // data. Later we may want to support some backward compatibility.
-constexpr static uint32_t Version = 14;
+constexpr static uint32_t Version = 15;
 
 llvm::Expected readRIFF(llvm::StringRef Data) {
   auto RIFF = riff::readFile(Data);

diff  --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp 
b/clang-tools-extra/clangd/index/SymbolCollector.cpp
index a7a6bd992f8b..94ab54be54b0 100644
--- a/clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -15,6 +15,7 @@
 #include "SourceCode.h"
 #include "SymbolLocation.h"
 #include "URI.h"
+#include "index/Relation.h"
 #include "index/SymbolID.h"
 #include "support/Logger.h"
 #include "clang/AST/Decl.h"
@@ -187,9 +188,12 @@ RefKind toRefKind(index::SymbolRoleSet Roles, bool Spelled 
= false) {
   return Result;
 }
 
-bool shouldIndexRelation(const index::SymbolRelation &R) {
-  // We currently only index BaseOf relations, for type hierarchy subtypes.
-  return R.Roles & static_cast(index::SymbolRole::RelationBaseOf);
+llvm::Optional indexableRelation(const index::SymbolRelation &R) 
{
+  if (R.Roles & static_cast(index::SymbolRole::RelationBaseOf))
+return RelationKind::BaseOf;
+  if (R.Roles & static_cast(index::SymbolRole::RelationOverrideOf))
+return RelationKind::OverriddenBy;
+  return None;
 }
 
 }

[PATCH] D87981: [X86] AMX programming model prototype.

2020-11-17 Thread LuoYuanke via Phabricator via cfe-commits

LuoYuanke updated this revision to Diff 305978.
LuoYuanke added a comment.

Address Wei's comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87981

Files:
  clang/include/clang/Basic/BuiltinsX86_64.def
  clang/lib/Headers/amxintrin.h
  clang/test/CodeGen/X86/amx_api.c
  llvm/include/llvm/CodeGen/LiveIntervalUnion.h
  llvm/include/llvm/CodeGen/LiveRegMatrix.h
  llvm/include/llvm/CodeGen/Passes.h
  llvm/include/llvm/CodeGen/TileShapeInfo.h
  llvm/include/llvm/CodeGen/VirtRegMap.h
  llvm/include/llvm/IR/Intrinsics.td
  llvm/include/llvm/IR/IntrinsicsX86.td
  llvm/lib/CodeGen/InlineSpiller.cpp
  llvm/lib/CodeGen/LiveIntervalUnion.cpp
  llvm/lib/CodeGen/LiveRegMatrix.cpp
  llvm/lib/CodeGen/VirtRegMap.cpp
  llvm/lib/Target/X86/CMakeLists.txt
  llvm/lib/Target/X86/X86.h
  llvm/lib/Target/X86/X86ExpandPseudo.cpp
  llvm/lib/Target/X86/X86FrameLowering.cpp
  llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
  llvm/lib/Target/X86/X86ISelLowering.cpp
  llvm/lib/Target/X86/X86InstrAMX.td
  llvm/lib/Target/X86/X86InstrInfo.cpp
  llvm/lib/Target/X86/X86LowerAMXType.cpp
  llvm/lib/Target/X86/X86PreTileConfig.cpp
  llvm/lib/Target/X86/X86RegisterInfo.cpp
  llvm/lib/Target/X86/X86RegisterInfo.h
  llvm/lib/Target/X86/X86RegisterInfo.td
  llvm/lib/Target/X86/X86Subtarget.h
  llvm/lib/Target/X86/X86TargetMachine.cpp
  llvm/lib/Target/X86/X86TileConfig.cpp
  llvm/test/CodeGen/X86/AMX/amx-across-func.ll
  llvm/test/CodeGen/X86/AMX/amx-config.ll
  llvm/test/CodeGen/X86/AMX/amx-spill.ll
  llvm/test/CodeGen/X86/AMX/amx-type.ll
  llvm/test/CodeGen/X86/O0-pipeline.ll
  llvm/test/CodeGen/X86/ipra-reg-usage.ll
  llvm/test/CodeGen/X86/opt-pipeline.ll
  llvm/test/CodeGen/X86/statepoint-fixup-invoke.mir
  llvm/test/CodeGen/X86/statepoint-fixup-shared-ehpad.mir

Index: llvm/test/CodeGen/X86/statepoint-fixup-shared-ehpad.mir
===
--- llvm/test/CodeGen/X86/statepoint-fixup-shared-ehpad.mir
+++ llvm/test/CodeGen/X86/statepoint-fixup-shared-ehpad.mir
@@ -108,7 +108,7 @@
   ; CHECK:   ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
   ; CHECK:   MOV64mr [[STACK0:%stack.[0-9]+]], 1, $noreg, 0, $noreg, killed $rbx :: (store 8 into [[STACK0]])
   ; CHECK:   MOV64mr [[STACK1:%stack.[0-9]+]], 1, $noreg, 0, $noreg, killed $r14 :: (store 8 into [[STACK1]])
-  ; CHECK:   STATEPOINT 0, 0, 0, @foo, 2, 0, 2, 0, 2, 0, 2, 2, 1, 8, [[STACK0]], 0, 1, 8, [[STACK1]], 0, 2, 0, 2, 2, 0, 0, 1, 1, csr_64, implicit-def $rsp, implicit-def $ssp :: (load store 8 on [[STACK0]]), (load store 8 on [[STACK1]])
+  ; CHECK:   STATEPOINT 0, 0, 0, @foo, 2, 0, 2, 0, 2, 0, 2, 2, 1, 8, [[STACK0]], 0, 1, 8, [[STACK1]], 0, 2, 0, 2, 2, 0, 0, 1, 1, csr_64, implicit-def $rsp, implicit-def $ssp :: (load store 8 on [[STACK1]]), (load store 8 on [[STACK0]])
   ; CHECK-DAG:   $rbx = MOV64rm [[STACK0]], 1, $noreg, 0, $noreg :: (load 8 from [[STACK0]])
   ; CHECK-DAG:   $r14 = MOV64rm [[STACK1]], 1, $noreg, 0, $noreg :: (load 8 from [[STACK1]])
   ; CHECK:   ADJCALLSTACKUP64 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
@@ -121,7 +121,7 @@
   ; CHECK:   ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
   ; CHECK-DAG:   MOV64mr [[STACK0]], 1, $noreg, 0, $noreg, killed $rbx :: (store 8 into [[STACK0]])
   ; CHECK-DAG:   MOV64mr [[STACK1]], 1, $noreg, 0, $noreg, killed $r14 :: (store 8 into [[STACK1]])
-  ; CHECK:   STATEPOINT 0, 0, 0, @bar, 2, 0, 2, 0, 2, 0, 2, 2, 1, 8, %stack.0, 0, 1, 8, [[STACK1]], 0, 2, 0, 2, 2, 0, 0, 1, 1, csr_64, implicit-def $rsp, implicit-def $ssp :: (load store 8 on [[STACK0]]), (load store 8 on [[STACK1]])
+  ; CHECK:   STATEPOINT 0, 0, 0, @bar, 2, 0, 2, 0, 2, 0, 2, 2, 1, 8, %stack.0, 0, 1, 8, [[STACK1]], 0, 2, 0, 2, 2, 0, 0, 1, 1, csr_64, implicit-def $rsp, implicit-def $ssp :: (load store 8 on [[STACK1]]), (load store 8 on [[STACK0]])
   ; CHECK-DAG:   $rbx = MOV64rm [[STACK0]], 1, $noreg, 0, $noreg :: (load 8 from [[STACK0]])
   ; CHECK-DAG:   $r14 = MOV64rm [[STACK1]], 1, $noreg, 0, $noreg :: (load 8 from [[STACK1]])
   ; CHECK:   ADJCALLSTACKUP64 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
Index: llvm/test/CodeGen/X86/statepoint-fixup-invoke.mir
===
--- llvm/test/CodeGen/X86/statepoint-fixup-invoke.mir
+++ llvm/test/CodeGen/X86/statepoint-fixup-invoke.mir
@@ -91,7 +91,7 @@
   ; CHECK-DAG:   MOV64mr %stack.1, 1, $noreg, 0, $noreg, $rdi :: (store 8 into %stack.1)
   ; CHECK:   EH_LABEL 
   ; CHECK:   ADJCALLSTACKDOWN64 0, 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp
-  ; CHECK:   STATEPOINT 0, 0, 1, @som

[PATCH] D91610: [clangd] Add OverriddenBy Relation to index.

2020-11-17 Thread Utkarsh Saxena via Phabricator via cfe-commits

usaxena95 updated this revision to Diff 305976.
usaxena95 marked an inline comment as done.
usaxena95 added a comment.

Addressed final comments. Ready to land.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91610

Files:
  clang-tools-extra/clangd/index/Relation.cpp
  clang-tools-extra/clangd/index/Relation.h
  clang-tools-extra/clangd/index/Serialization.cpp
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/test/index-serialization/Inputs/sample.cpp
  clang-tools-extra/clangd/test/index-serialization/Inputs/sample.h
  clang-tools-extra/clangd/test/index-serialization/Inputs/sample.idx
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -97,6 +97,9 @@
   const Range &Range = ::testing::get<1>(arg);
   return rangesMatch(Pos.Location, Range);
 }
+MATCHER_P2(OverriddenBy, Subject, Object, "") {
+  return arg == Relation{Subject.ID, RelationKind::OverriddenBy, Object.ID};
+}
 ::testing::Matcher &>
 HaveRanges(const std::vector Ranges) {
   return ::testing::UnorderedPointwise(RefRange(), Ranges);
@@ -1031,7 +1034,7 @@
   HaveRanges(Header.ranges("macro");
 }
 
-TEST_F(SymbolCollectorTest, Relations) {
+TEST_F(SymbolCollectorTest, BaseOfRelations) {
   std::string Header = R"(
   class Base {};
   class Derived : public Base {};
@@ -1043,6 +1046,77 @@
   Contains(Relation{Base.ID, RelationKind::BaseOf, Derived.ID}));
 }
 
+TEST_F(SymbolCollectorTest, OverrideRelationsSimpleInheritance) {
+  std::string Header = R"cpp(
+class A {
+  virtual void foo();
+};
+class B : public A {
+  void foo() override;  // A::foo
+  virtual void bar();
+};
+class C : public B {
+  void bar() override;  // B::bar
+};
+class D: public C {
+  void foo() override;  // B::foo
+  void bar() override;  // C::bar
+};
+  )cpp";
+  runSymbolCollector(Header, /*Main=*/"");
+  const Symbol &AFoo = findSymbol(Symbols, "A::foo");
+  const Symbol &BFoo = findSymbol(Symbols, "B::foo");
+  const Symbol &DFoo = findSymbol(Symbols, "D::foo");
+
+  const Symbol &BBar = findSymbol(Symbols, "B::bar");
+  const Symbol &CBar = findSymbol(Symbols, "C::bar");
+  const Symbol &DBar = findSymbol(Symbols, "D::bar");
+
+  std::vector Result;
+  for (const Relation &R : Relations)
+if (R.Predicate == RelationKind::OverriddenBy)
+  Result.push_back(R);
+  EXPECT_THAT(Result, UnorderedElementsAre(
+  OverriddenBy(AFoo, BFoo), OverriddenBy(BBar, CBar),
+  OverriddenBy(BFoo, DFoo), OverriddenBy(CBar, DBar)));
+}
+
+TEST_F(SymbolCollectorTest, OverrideRelationsMultipleInheritance) {
+  std::string Header = R"cpp(
+class A {
+  virtual void foo();
+};
+class B {
+  virtual void bar();
+};
+class C : public B {
+  void bar() override;  // B::bar
+  virtual void baz();
+}
+class D : public A, C {
+  void foo() override;  // A::foo
+  void bar() override;  // C::bar
+  void baz() override;  // C::baz
+};
+  )cpp";
+  runSymbolCollector(Header, /*Main=*/"");
+  const Symbol &AFoo = findSymbol(Symbols, "A::foo");
+  const Symbol &BBar = findSymbol(Symbols, "B::bar");
+  const Symbol &CBar = findSymbol(Symbols, "C::bar");
+  const Symbol &CBaz = findSymbol(Symbols, "C::baz");
+  const Symbol &DFoo = findSymbol(Symbols, "D::foo");
+  const Symbol &DBar = findSymbol(Symbols, "D::bar");
+  const Symbol &DBaz = findSymbol(Symbols, "D::baz");
+
+  std::vector Result;
+  for (const Relation &R : Relations)
+if (R.Predicate == RelationKind::OverriddenBy)
+  Result.push_back(R);
+  EXPECT_THAT(Result, UnorderedElementsAre(
+  OverriddenBy(BBar, CBar), OverriddenBy(AFoo, DFoo),
+  OverriddenBy(CBar, DBar), OverriddenBy(CBaz, DBaz)));
+}
+
 TEST_F(SymbolCollectorTest, CountReferences) {
   const std::string Header = R"(
 class W;
Index: clang-tools-extra/clangd/test/index-serialization/Inputs/sample.h
===
--- clang-tools-extra/clangd/test/index-serialization/Inputs/sample.h
+++ clang-tools-extra/clangd/test/index-serialization/Inputs/sample.h
@@ -1,4 +1,6 @@
 #pragma once
 
 // Introduce a symbol.
-struct Foo {};
+struct Foo {
+  virtual void Func() {}
+};
Index: clang-tools-extra/clangd/test/index-serialization/Inputs/sample.cpp
===
--- clang-tools-extra/clangd/test/index-serialization/Inputs/sample.cpp
+++ clang-tools-extra/clangd/test/index-serialization/Inputs/sample.cpp
@@ -2,4 +2,7 @@
 #i

[PATCH] D91659: Allow anonymous enum typedefs to be referenced with the 'enum' specifier under MSVC compat mode

2020-11-17 Thread Shivanshu Goyal via Phabricator via cfe-commits

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

Thank you very much for the review @rsmith!




Comment at: clang/lib/Sema/SemaDecl.cpp:15752-15754
+  bool AnonymousEnumEligible = getLangOpts().MSVCCompat &&
+   (Kind == TTK_Enum) &&
+   Tag->getDeclName().isEmpty();

rsmith wrote:
> In the anonymous union case, we also need to check for qualifiers on the 
> typedef type, and more broadly perhaps we should be using 
> `getAnonDeclWithTypedefName` here too. Presumably the new case should also 
> only apply for `TUK_Reference`.
I added a check for TUK_Reference. But I don't fully understand your other 
comment about checking the qualifiers on the typedef type. Could you please 
explain a little more?



Comment at: clang/test/Sema/enum-typedef-msvc.c:15
+  (void)foo;
+}

rsmith wrote:
> Please also add a testcase for the situation where the use of `enum Foo` 
> occurs in the same scope as the typedef and anonymous `enum` declaration. 
> (MSVC has weird behavior here that we don't necessarily need to follow, but 
> we should make sure we at least do something defensible.)
Oh interesting! I didn't realize MSVC produces an error in that case. Clang 
does not produce an error with these changes in that case. Is that OK?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91659

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

[PATCH] D91626: [clangd] Implement textDocument/implementation.

2020-11-17 Thread Utkarsh Saxena via Phabricator via cfe-commits

usaxena95 updated this revision to Diff 305974.
usaxena95 added a comment.

Remove unintended change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91626

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/test/initialize-params.test
  clang-tools-extra/clangd/unittests/XRefsTests.cpp

Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -1464,6 +1464,44 @@
   }
 }
 
+TEST(FindImplementations, Inheritance) {
+  llvm::StringRef Test = R"cpp(
+struct Base {
+  virtual void F$1^oo();
+};
+struct Child1 : Base {
+  void $1[[Fo$3^o]]() override;
+  virtual void B$2^ar();
+};
+struct Child2 : Child1 {
+  void $3[[Foo]]() override;
+  void $2[[Bar]]() override;
+};
+void FromReference() {
+  Base* B;
+  B->Fo$1^o();
+  &Base::Fo$1^o;
+  Child1 * C1;
+  C1->B$2^ar();
+  C1->Fo$3^o();
+}
+  )cpp";
+
+  Annotations Code(Test);
+  auto TU = TestTU::withCode(Code.code());
+  auto AST = TU.build();
+  for (const std::string &Label : {"1", "2", "3"}) {
+std::vector> ExpectedLocations;
+for (const auto &R : Code.ranges(Label))
+  ExpectedLocations.push_back(RangeIs(R));
+for (const auto &Point : Code.points(Label)) {
+  EXPECT_THAT(findImplementations(AST, Point, TU.index().get()).References,
+  UnorderedElementsAreArray(ExpectedLocations))
+  << Code.code() << " at " << Point;
+}
+  }
+}
+
 TEST(FindReferences, WithinAST) {
   const char *Tests[] = {
   R"cpp(// Local variable
Index: clang-tools-extra/clangd/test/initialize-params.test
===
--- clang-tools-extra/clangd/test/initialize-params.test
+++ clang-tools-extra/clangd/test/initialize-params.test
@@ -66,7 +66,8 @@
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "hoverProvider": true,
-# CHECK-NEXT:  "memoryUsageProvider": true
+# CHECK-NEXT:  "implementationProvider": true,
+# CHECK-NEXT:  "memoryUsageProvider": true,
 # CHECK-NEXT:  "referencesProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "selectionRangeProvider": true,
Index: clang-tools-extra/clangd/XRefs.h
===
--- clang-tools-extra/clangd/XRefs.h
+++ clang-tools-extra/clangd/XRefs.h
@@ -82,6 +82,11 @@
   std::vector References;
   bool HasMore = false;
 };
+
+/// Returns implementations of the virtual function at a specified \p Pos.
+ReferencesResult findImplementations(ParsedAST &AST, Position Pos,
+ const SymbolIndex *Index);
+
 /// Returns references of the symbol at a specified \p Pos.
 /// \p Limit limits the number of results returned (0 means no limit).
 ReferencesResult findReferences(ParsedAST &AST, Position Pos, uint32_t Limit,
Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -1124,6 +1124,38 @@
   return Result;
 }
 
+ReferencesResult findImplementations(ParsedAST &AST, Position Pos,
+ const SymbolIndex *Index) {
+  ReferencesResult Results;
+  // We rely on index to find the implementations in subclasses.
+  if (!Index)
+return Results;
+  const SourceManager &SM = AST.getSourceManager();
+  auto MainFilePath =
+  getCanonicalPath(SM.getFileEntryForID(SM.getMainFileID()), SM);
+  if (!MainFilePath) {
+elog("Failed to get a path for the main file, so no implementations.");
+return Results;
+  }
+  auto CurLoc = sourceLocationInMainFile(SM, Pos);
+
+  DeclRelationSet Relations =
+  DeclRelation::TemplatePattern | DeclRelation::Alias;
+  RelationsRequest Req;
+  Req.Predicate = RelationKind::OverriddenBy;
+  for (const NamedDecl *ND : getDeclAtPosition(AST, *CurLoc, Relations))
+if (llvm::isa(ND))
+  Req.Subjects.insert(getSymbolID(ND));
+
+  if (Req.Subjects.empty())
+return Results;
+  Index->relations(Req, [&](const SymbolID &Subject, const Symbol &Object) {
+if (auto Loc = symbolToLocation(Object, *MainFilePath))
+  Results.References.push_back(*Loc);
+  });
+  return Results;
+}
+
 ReferencesResult findReferences(ParsedAST &AST, Position Pos, uint32_t Limit,
 const SymbolIndex *Index) {
   if (!Limit)
Index: clang-tools-ext

[PATCH] D91626: [clangd] Implement textDocument/implementation.

2020-11-17 Thread Utkarsh Saxena via Phabricator via cfe-commits

usaxena95 updated this revision to Diff 305972.
usaxena95 added a comment.

Addressed final comments. Ready to land.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91626

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Protocol.cpp
  clang-tools-extra/clangd/Protocol.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/XRefs.h
  clang-tools-extra/clangd/index/Relation.h
  clang-tools-extra/clangd/test/initialize-params.test
  clang-tools-extra/clangd/unittests/XRefsTests.cpp

Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -1464,6 +1464,44 @@
   }
 }
 
+TEST(FindImplementations, Inheritance) {
+  llvm::StringRef Test = R"cpp(
+struct Base {
+  virtual void F$1^oo();
+};
+struct Child1 : Base {
+  void $1[[Fo$3^o]]() override;
+  virtual void B$2^ar();
+};
+struct Child2 : Child1 {
+  void $3[[Foo]]() override;
+  void $2[[Bar]]() override;
+};
+void FromReference() {
+  Base* B;
+  B->Fo$1^o();
+  &Base::Fo$1^o;
+  Child1 * C1;
+  C1->B$2^ar();
+  C1->Fo$3^o();
+}
+  )cpp";
+
+  Annotations Code(Test);
+  auto TU = TestTU::withCode(Code.code());
+  auto AST = TU.build();
+  for (const std::string &Label : {"1", "2", "3"}) {
+std::vector> ExpectedLocations;
+for (const auto &R : Code.ranges(Label))
+  ExpectedLocations.push_back(RangeIs(R));
+for (const auto &Point : Code.points(Label)) {
+  EXPECT_THAT(findImplementations(AST, Point, TU.index().get()).References,
+  UnorderedElementsAreArray(ExpectedLocations))
+  << Code.code() << " at " << Point;
+}
+  }
+}
+
 TEST(FindReferences, WithinAST) {
   const char *Tests[] = {
   R"cpp(// Local variable
Index: clang-tools-extra/clangd/test/initialize-params.test
===
--- clang-tools-extra/clangd/test/initialize-params.test
+++ clang-tools-extra/clangd/test/initialize-params.test
@@ -66,7 +66,8 @@
 # CHECK-NEXT:]
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "hoverProvider": true,
-# CHECK-NEXT:  "memoryUsageProvider": true
+# CHECK-NEXT:  "implementationProvider": true,
+# CHECK-NEXT:  "memoryUsageProvider": true,
 # CHECK-NEXT:  "referencesProvider": true,
 # CHECK-NEXT:  "renameProvider": true,
 # CHECK-NEXT:  "selectionRangeProvider": true,
Index: clang-tools-extra/clangd/index/Relation.h
===
--- clang-tools-extra/clangd/index/Relation.h
+++ clang-tools-extra/clangd/index/Relation.h
@@ -28,8 +28,9 @@
 /// For an example:
 ///   - "A is a base class of B" is represented as
 /// { Subject = A, Predicate = BaseOf, Object = B }.
-///   - "A::Foo is overriden by B::Foo" is represented as
-/// { Subject = A::Foo, Predicate = OverriddenBy, Object = B::Foo }.
+///   - "Derived::Foo overrides Base::Foo" is represented as
+/// { Subject = Base::Foo, Predicate = OverriddenBy, Object = Derived::Foo
+/// }.
 struct Relation {
   SymbolID Subject;
   RelationKind Predicate;
Index: clang-tools-extra/clangd/XRefs.h
===
--- clang-tools-extra/clangd/XRefs.h
+++ clang-tools-extra/clangd/XRefs.h
@@ -82,6 +82,11 @@
   std::vector References;
   bool HasMore = false;
 };
+
+/// Returns implementations of the virtual function at a specified \p Pos.
+ReferencesResult findImplementations(ParsedAST &AST, Position Pos,
+ const SymbolIndex *Index);
+
 /// Returns references of the symbol at a specified \p Pos.
 /// \p Limit limits the number of results returned (0 means no limit).
 ReferencesResult findReferences(ParsedAST &AST, Position Pos, uint32_t Limit,
Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -1124,6 +1124,38 @@
   return Result;
 }
 
+ReferencesResult findImplementations(ParsedAST &AST, Position Pos,
+ const SymbolIndex *Index) {
+  ReferencesResult Results;
+  // We rely on index to find the implementations in subclasses.
+  if (!Index)
+return Results;
+  const SourceManager &SM = AST.getSourceManager();
+  auto MainFilePath =
+  getCanonicalPath(SM.getFileEntryForID(SM.getMainFileID()), SM);
+  if (!MainFilePath) {
+elog("Failed to get a path for the main file, so no implementations.");
+return Results;
+  }
+  auto CurLoc = sourceLocation

[PATCH] D91659: Allow anonymous enum typedefs to be referenced with the 'enum' specifier under MSVC compat mode

2020-11-17 Thread Shivanshu Goyal via Phabricator via cfe-commits

shivanshu3 updated this revision to Diff 305971.
shivanshu3 added a reviewer: rsmith.
shivanshu3 added a comment.

Addressing some of rsmith's comments:

- Add a test case where an enum variable is declared in the same scope as the 
enum definition
- Fix a style issue with parentheses
- Simplify the if condition for the C case
- The C++ case should only apply for TUK_Reference


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91659

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/Sema/enum-typedef-msvc.c
  clang/test/Sema/enum-typedef-msvc.cpp


Index: clang/test/Sema/enum-typedef-msvc.cpp
===
--- /dev/null
+++ clang/test/Sema/enum-typedef-msvc.cpp
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility %s
+
+typedef enum {
+  First,
+  Second
+} MyEnum;
+
+class AMyInterface {
+  virtual void MyFunc(enum MyEnum *param) = 0;
+};
+
+class MyImpl : public AMyInterface {
+  virtual void MyFunc(enum MyEnum *param) override {}
+};
+
+enum MyEnum myEnum;
+
+// expected-no-diagnostics
Index: clang/test/Sema/enum-typedef-msvc.c
===
--- /dev/null
+++ clang/test/Sema/enum-typedef-msvc.c
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility -DUSE_MSVC_COMPAT 
%s
+
+typedef enum {
+  A
+} Foo;
+
+void func() {
+#ifdef USE_MSVC_COMPAT
+  enum Foo foo; // expected-no-diagnostics
+#else
+  enum Foo foo; // expected-error {{variable has incomplete type 'enum Foo'}} 
// expected-note {{forward declaration of 'enum Foo'}}
+#endif
+  (void)foo;
+}
+
+#ifdef USE_MSVC_COMPAT
+enum Foo foo2; // expected-no-diagnostics
+#else
+enum Foo foo2;  // expected-error {{tentative definition has type 'enum Foo' 
that is never completed}}
+// expected-note@-1 {{forward declaration of 'enum Foo'}}
+#endif
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -15539,6 +15539,21 @@
 // shouldn't be diagnosing.
 LookupName(Previous, S);
 
+// Under MSVC, the 'enum' specifier can be used for typedef'd enums.
+// Note that lookup only fails in C, not C++, so this if condition
+// is only used for C code.
+if (getLangOpts().MSVCCompat && Kind == TTK_Enum && Previous.empty() &&
+TUK == TUK_Reference) {
+  LookupResult TypedefEnumLookup(*this, Name, NameLoc, LookupOrdinaryName,
+ Redecl);
+  LookupName(TypedefEnumLookup, S);
+
+  if (auto *TD = TypedefEnumLookup.getAsSingle())
+if (auto *ED = dyn_cast_or_null(
+TD->getAnonDeclWithTypedefName(true)))
+  Previous.addDecl(ED);
+}
+
 // When declaring or defining a tag, ignore ambiguities introduced
 // by types using'ed into this scope.
 if (Previous.isAmbiguous() &&
@@ -15721,9 +15736,12 @@
   if (TypedefNameDecl *TD = dyn_cast(PrevDecl)) {
 if (const TagType *TT = TD->getUnderlyingType()->getAs()) {
   TagDecl *Tag = TT->getDecl();
-  if (Tag->getDeclName() == Name &&
-  Tag->getDeclContext()->getRedeclContext()
-  ->Equals(TD->getDeclContext()->getRedeclContext())) {
+  bool AnonymousEnumEligible =
+  getLangOpts().MSVCCompat && TUK == TUK_Reference &&
+  Kind == TTK_Enum && Tag->getDeclName().isEmpty();
+  if ((Tag->getDeclName() == Name || AnonymousEnumEligible) &&
+  Tag->getDeclContext()->getRedeclContext()->Equals(
+  TD->getDeclContext()->getRedeclContext())) {
 PrevDecl = Tag;
 Previous.clear();
 Previous.addDecl(Tag);


Index: clang/test/Sema/enum-typedef-msvc.cpp
===
--- /dev/null
+++ clang/test/Sema/enum-typedef-msvc.cpp
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility %s
+
+typedef enum {
+  First,
+  Second
+} MyEnum;
+
+class AMyInterface {
+  virtual void MyFunc(enum MyEnum *param) = 0;
+};
+
+class MyImpl : public AMyInterface {
+  virtual void MyFunc(enum MyEnum *param) override {}
+};
+
+enum MyEnum myEnum;
+
+// expected-no-diagnostics
Index: clang/test/Sema/enum-typedef-msvc.c
===
--- /dev/null
+++ clang/test/Sema/enum-typedef-msvc.c
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility -DUSE_MSVC_COMPAT %s
+
+typedef enum {
+  A
+} Foo;
+
+void func() {
+#ifdef USE_MSVC_COMPAT
+  enum Foo foo; // expected-no-diagnostics
+#else
+  enum Foo foo; // expected-error {{variable has incomplete type 'enum Foo'}} // expected-note {{forward declara

[PATCH] D90441: [X86] Add support for vex, vex2, vex3, and evex for MASM

2020-11-17 Thread Pengfei Wang via Phabricator via cfe-commits

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

LGTM. Thanks.
You'd better wait one or two days to see if other people objects.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90441

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

[PATCH] D90441: [X86] Add support for vex, vex2, vex3, and evex for MASM

2020-11-17 Thread LiuChen via Phabricator via cfe-commits

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

> It allows more than two, right? like `{vex}{vex2}{vex3} instruction`. I think 
> it should be a bug for att.

Yes, My previous statement is incorrect, it should be ‘two more’. Thanks for 
your correction.
We might need another patch to fix it.




Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:3079
+  if (ForcedVEXEncoding != VEXEncoding_Default) {
+if (getLexer().isNot(AsmToken::Identifier))
+  return Error(Parser.getTok().getLoc(), "Expected identifier");

pengfei wrote:
> Do you need to eat the prefix here?
No. The prefix has been eat.
For example: vex vcvtps2pd xmm0, xmm1 .
Current token is 'vex' and the rest is 'vcvtps2pd xmm0, xmm1'. 'vcvtps2pd' is 
the next token which will be eat in line 3082.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90441

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

[PATCH] D91651: [clang] Add a warning (à la gcc) for too small enum bitfields

2020-11-17 Thread Faisal Vali via Phabricator via cfe-commits

faisalv planned changes to this revision.
faisalv marked 3 inline comments as done.
faisalv added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:16443-16446
+  const unsigned BitsNeeded =
+  IsSignedEnum
+  ? std::max(ED->getNumPositiveBits() + 1, 
ED->getNumNegativeBits())
+  : ED->getNumPositiveBits();

rsmith wrote:
> Please add a member function to `EnumDecl` to compute this. SemaChecking.cpp 
> does the same calculation twice, and CGExpr.cpp does it once too, so it seems 
> past time to factor out this calculation.
Done.  Will try and commit a separate patch that replaces those uses, once this 
one has been approved.



Comment at: clang/lib/Sema/SemaDecl.cpp:16448
+  
+  if (BitsNeeded > Value.getZExtValue()) {
+// TODO: Should we be emitting diagnostics for unnamed bitfields that

rsmith wrote:
> > When comparing an APSInt to an unsigned value - should i prefer using the 
> > overloaded operator (i.e. Value < BitsNeeded) or stick with BitsNeeded > 
> > Value.getZExtValue().
> 
> Never use `get*ExtValue` unless you've already checked that the integer fits 
> in 64 bits (and even then, it's better to avoid it when you can). For 
> example, due to the incorrect pre-existing use of `getZExtValue` in this 
> function, we asserted on:
> 
> ```
> enum E {};
> struct X { E e : (__int128)0x7fff''' * 
> (__int128)0x7fff'''; };
> ```
> 
> I've fixed that in rG8e923ec2a803d54154aaa0079c1cfcf146b7a22f, but we should 
> avoid reintroducing uses of `getZExtValue` here.
Aah!  I'm glad I asked.  Thanks!



Comment at: clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp:62
+
+#ifdef FV_UNNAMED_BITFIELD
+#define NAME(x)

rsmith wrote:
> Please remove the `FV_` prefix here -- authorship information is in the 
> version control history if we need it.
> 
> It would also be better to write out the test twice (once with named and once 
> with unnamed bit-fields) instead of using a macro and running the entire test 
> file an additional time. Each `clang` invocation adds to our total test time 
> much more substantially than a few more lines of testcase do, and 
> non-macro-expanded testcases are easier to understand and maintain.
> Please remove the `FV_` prefix here -- authorship information is in the 
> version control history if we need it.
> 

Hah - that wasn't a signifier of authorship but rather employment of a well 
known acronym in military circles (Field Verification :  
https://www.acronymfinder.com/Field-Verification-(Census)-(FV).html )  Lol - 
can't slip anything by you Richard ;)

> It would also be better to write out the test twice (once with named and once 
> with unnamed bit-fields) instead of using a macro and running the entire test 
> file an additional time. Each `clang` invocation adds to our total test time 
> much more substantially than a few more lines of testcase do, and 
> non-macro-expanded testcases are easier to understand and maintain.

Good to know.   Thanks. Suppression of the warning for unnamed bitfields made 
that test case even simpler.  



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91651

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

[PATCH] D91651: [clang] Add a warning (à la gcc) for too small enum bitfields

2020-11-17 Thread Faisal Vali via Phabricator via cfe-commits

faisalv updated this revision to Diff 305970.
faisalv added a comment.

Based on Richards Feedback, this update includes the following changes:

- avoids calling the fragile getZextValue() for comparing against bit-field 
width, and uses APSInt's comparison operator overload
- suppresses/avoids the warning for unnamed bit-fields
- simplifies the test case and avoids preprocessor cleverness (and thus an 
extra pass).

Thank you for taking the time to review this Richard!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91651

Files:
  clang/include/clang/AST/Decl.h
  clang/lib/Sema/SemaDecl.cpp
  clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp


Index: clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp
===
--- clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp
+++ clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp
@@ -57,3 +57,18 @@
   f.two_bits = (unsigned)three_bits_signed;
   f.two_bits = static_cast(three_bits_signed);
 }
+
+enum class E_UNSIGNED_LONG_LONG : long long { PLUS_MAX = (1LL << (sizeof(long 
long) * 8 - 2)) };
+enum class E_SIGNED_LONG_LONG : long long { PLUS_MAX = (1LL << (sizeof(long 
long) * 8 - 2)), MINUS = -1 };
+
+struct E64 {
+  E_UNSIGNED_LONG_LONG b3 : (sizeof(long long) * 8) - 1; // OK.
+  E_SIGNED_LONG_LONG   b4 : (sizeof(long long) * 8); // OK.
+
+  E_UNSIGNED_LONG_LONG b1 : 5; //expected-warning {{bit-field 'b1' is not wide 
enough}} expected-note {{widen this field to 63}}
+  E_SIGNED_LONG_LONG   b2 : 5; //expected-warning {{bit-field 'b2' is not wide 
enough}} expected-note {{widen this field to 64}}
+  
+  E_UNSIGNED_LONG_LONG : 5; // OK.  no warning for unnamed bit fields.
+  E_SIGNED_LONG_LONG   : 5; // OK.  no warning for unnamed bit fields.
+  
+};
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -16430,6 +16430,22 @@
   }
 
   if (!FieldTy->isDependentType()) {
+// Check if a named enum bit-field has enough bits to accomodate all its
+// enumerators. We can ignore unnamed enum bit-fields since they do not
+// represent values.
+if (FieldName && FieldTy->isEnumeralType()) {
+  const EnumDecl *const ED = FieldTy->castAs()->getDecl();
+
+  const auto BitsNeeded = ED->getTotalBitsNeeded();
+  if (BitsNeeded > Value) {
+Diag(FieldLoc, diag::warn_bitfield_too_small_for_enum)
+<< FieldName << ED;
+
+Diag(BitWidth->getExprLoc(), diag::note_widen_bitfield)
+<< BitsNeeded << ED << BitWidth->getSourceRange();
+  }
+}
+
 uint64_t TypeStorageSize = Context.getTypeSize(FieldTy);
 uint64_t TypeWidth = Context.getIntWidth(FieldTy);
 bool BitfieldIsOverwide = Value.ugt(TypeWidth);
Index: clang/include/clang/AST/Decl.h
===
--- clang/include/clang/AST/Decl.h
+++ clang/include/clang/AST/Decl.h
@@ -3723,6 +3723,14 @@
   /// -101 1001011 8
   unsigned getNumNegativeBits() const { return EnumDeclBits.NumNegativeBits; }
 
+  /// Returns the least width in bits required to store all the enumerators.
+  unsigned getTotalBitsNeeded() const {
+// If the enum has negative values, we need one more bit than the normal
+// number of positive bits to represent the sign bit.
+return getNumNegativeBits() > 0
+   ? std::max(getNumPositiveBits() + 1, getNumNegativeBits())
+   : getNumPositiveBits();
+  }
   /// Returns true if this is a C++11 scoped enumeration.
   bool isScoped() const { return EnumDeclBits.IsScoped; }
 


Index: clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp
===
--- clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp
+++ clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp
@@ -57,3 +57,18 @@
   f.two_bits = (unsigned)three_bits_signed;
   f.two_bits = static_cast(three_bits_signed);
 }
+
+enum class E_UNSIGNED_LONG_LONG : long long { PLUS_MAX = (1LL << (sizeof(long long) * 8 - 2)) };
+enum class E_SIGNED_LONG_LONG : long long { PLUS_MAX = (1LL << (sizeof(long long) * 8 - 2)), MINUS = -1 };
+
+struct E64 {
+  E_UNSIGNED_LONG_LONG b3 : (sizeof(long long) * 8) - 1; // OK.
+  E_SIGNED_LONG_LONG   b4 : (sizeof(long long) * 8); // OK.
+
+  E_UNSIGNED_LONG_LONG b1 : 5; //expected-warning {{bit-field 'b1' is not wide enough}} expected-note {{widen this field to 63}}
+  E_SIGNED_LONG_LONG   b2 : 5; //expected-warning {{bit-field 'b2' is not wide enough}} expected-note {{widen this field to 64}}
+  
+  E_UNSIGNED_LONG_LONG : 5; // OK.  no warning for unnamed bit fields.
+  E_SIGNED_LONG_LONG   : 5; // OK.  no warning for unnamed bit fields.
+  
+};
Index: clang/lib/Sema/SemaDecl.cpp

[PATCH] D90441: [X86] Add support for vex, vex2, vex3, and evex for MASM

2020-11-17 Thread Pengfei Wang via Phabricator via cfe-commits

pengfei added a comment.

> 2. Delete IsPrefix parameter, and delete 'break', so that we won't check 
> prefix again. I am not sure if this is right. Att format can allow two prefix 
> and using the last one as the finally encoding prefix. I think this may not 
> be the original intention of the design.

It allows more than two, right? like `{vex}{vex2}{vex3} instruction`. I think 
it should be a bug for att.




Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:3079
+  if (ForcedVEXEncoding != VEXEncoding_Default) {
+if (getLexer().isNot(AsmToken::Identifier))
+  return Error(Parser.getTok().getLoc(), "Expected identifier");

Do you need to eat the prefix here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90441

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

[PATCH] D87981: [X86] AMX programming model prototype.

2020-11-17 Thread LuoYuanke via Phabricator via cfe-commits

LuoYuanke added inline comments.



Comment at: llvm/include/llvm/CodeGen/LiveRegMatrix.h:44
   VirtRegMap *VRM;
+  MachineRegisterInfo *MRI;
 

wxiao3 wrote:
> what's the purpose of this member?
It is useless. Thanks.



Comment at: llvm/lib/Target/X86/X86RegisterInfo.cpp:917
+  }
+  for (MCPhysReg PhysReg : Order) {
+if (!MRI->isReserved(PhysReg))

wxiao3 wrote:
> Don't need to add PhysReg again if PhysReg is already in Hints.
You are right. I will revise the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87981

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

[PATCH] D90441: [X86] Add support for vex, vex2, vex3, and evex for MASM

2020-11-17 Thread LiuChen via Phabricator via cfe-commits

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

> 2. Delete IsPrefix parameter, and delete 'break'

It should be 'continue'. Sorry for this mistake.




Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:3083
+  }
+  if (IsPrefix) {
+NameLoc = Parser.getTok().getLoc();

pengfei wrote:
> You just need to check `ForcedVEXEncoding != VEXEncoding_Default`.
I think this is better. Multi vex/evex prefix doesn't make sense.



Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:3084
+  if (IsPrefix) {
+NameLoc = Parser.getTok().getLoc();
+if (getLexer().isNot(AsmToken::Identifier))

pengfei wrote:
> Unused assignment. It may suppose to be used on line 3086.
This would be used later. However, this should only be updated when there is 
prefix. I put it in wrong place.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90441

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

[PATCH] D72184: [BPF] support atomic instructions

2020-11-17 Thread Alexei Starovoitov via Phabricator via cfe-commits

ast added a comment.

looks good. Before landing we need to agree on the full set of instructions 
that -mcpu=v4 will support.
atomic_fetch_or|xor|and are probably needed as instructions. The kernel JIT 
will generate x86 cmpxchg for them.
Because if llvm generates bpf cmpxchg insn then we'd need to teach the verifier 
to recognize infinite loops.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72184

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

[PATCH] D91269: [Clang][CodeGen][RISCV] Add hard float ABI tests with empty struct

2020-11-17 Thread John McCall via Phabricator via cfe-commits

rjmccall accepted this revision.
rjmccall added a comment.

We don't usually add known-broken tests like this before a fix, as opposed to 
just landing them as part of the fix, but if you have a good reason to do so 
it's okay.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91269

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

[PATCH] D91673: [PGO] Enable preinline and cleanup when optimize for size

2020-11-17 Thread Rong Xu via Phabricator via cfe-commits

xur added a comment.

This is probably OK for -Os (SizeLevel == 1), but we need to be careful with Oz 
(SizeLevel == 2).
We already know that enabling preinliner in general will reduce size -- as the 
preinliner is pretty conservative. But there will be cases size will be 
increased.

I would like more test results (like bootstrap clang) before committing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91673

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

[PATCH] D91676: Avoid redundant work when computing vtable vcall visibility

2020-11-17 Thread Teresa Johnson via Phabricator via cfe-commits

tejohnson created this revision.
tejohnson added reviewers: pcc, ostannard.
Herald added a project: clang.
tejohnson requested review of this revision.

Add a Visited set to avoid repeatedly processing the same base classes
in complex class hierarchies. This cut down the compile time of one
source file from >12min to ~1min.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91676

Files:
  clang/lib/CodeGen/CGVTables.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/MicrosoftCXXABI.cpp


Index: clang/lib/CodeGen/MicrosoftCXXABI.cpp
===
--- clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1649,8 +1649,9 @@
   // TODO: Should VirtualFunctionElimination also be supported here?
   // See similar handling in CodeGenModule::EmitVTableTypeMetadata.
   if (CGM.getCodeGenOpts().WholeProgramVTables) {
+llvm::DenseSet Visited;
 llvm::GlobalObject::VCallVisibility TypeVis =
-CGM.GetVCallVisibilityLevel(RD);
+CGM.GetVCallVisibilityLevel(RD, Visited);
 if (TypeVis != llvm::GlobalObject::VCallVisibilityPublic)
   VTable->setVCallVisibilityMetadata(TypeVis);
   }
Index: clang/lib/CodeGen/CodeGenModule.h
===
--- clang/lib/CodeGen/CodeGenModule.h
+++ clang/lib/CodeGen/CodeGenModule.h
@@ -1333,8 +1333,11 @@
   /// a virtual function call could be made which ends up being dispatched to a
   /// member function of this class. This scope can be wider than the 
visibility
   /// of the class itself when the class has a more-visible dynamic base class.
+  /// The client should pass in an empty Visited set, which is used to prevent
+  /// redundant recursive processing.
   llvm::GlobalObject::VCallVisibility
-  GetVCallVisibilityLevel(const CXXRecordDecl *RD);
+  GetVCallVisibilityLevel(const CXXRecordDecl *RD,
+  llvm::DenseSet &Visited);
 
   /// Emit type metadata for the given vtable using the given layout.
   void EmitVTableTypeMetadata(const CXXRecordDecl *RD,
Index: clang/lib/CodeGen/CGVTables.cpp
===
--- clang/lib/CodeGen/CGVTables.cpp
+++ clang/lib/CodeGen/CGVTables.cpp
@@ -1294,8 +1294,13 @@
   return !HasLTOVisibilityPublicStd(RD);
 }
 
-llvm::GlobalObject::VCallVisibility
-CodeGenModule::GetVCallVisibilityLevel(const CXXRecordDecl *RD) {
+llvm::GlobalObject::VCallVisibility CodeGenModule::GetVCallVisibilityLevel(
+const CXXRecordDecl *RD, llvm::DenseSet &Visited) {
+  // If we have already visited this RD, return the max visibility, so that it
+  // has no effect on the min visibility computed below by the recursive 
caller.
+  if (!Visited.insert(RD).second)
+return llvm::GlobalObject::VCallVisibilityTranslationUnit;
+
   LinkageInfo LV = RD->getLinkageAndVisibility();
   llvm::GlobalObject::VCallVisibility TypeVis;
   if (!isExternallyVisible(LV.getLinkage()))
@@ -1307,13 +1312,15 @@
 
   for (auto B : RD->bases())
 if (B.getType()->getAsCXXRecordDecl()->isDynamicClass())
-  TypeVis = std::min(TypeVis,
-
GetVCallVisibilityLevel(B.getType()->getAsCXXRecordDecl()));
+  TypeVis = std::min(
+  TypeVis,
+  GetVCallVisibilityLevel(B.getType()->getAsCXXRecordDecl(), Visited));
 
   for (auto B : RD->vbases())
 if (B.getType()->getAsCXXRecordDecl()->isDynamicClass())
-  TypeVis = std::min(TypeVis,
-
GetVCallVisibilityLevel(B.getType()->getAsCXXRecordDecl()));
+  TypeVis = std::min(
+  TypeVis,
+  GetVCallVisibilityLevel(B.getType()->getAsCXXRecordDecl(), Visited));
 
   return TypeVis;
 }
@@ -1382,7 +1389,9 @@
 
   if (getCodeGenOpts().VirtualFunctionElimination ||
   getCodeGenOpts().WholeProgramVTables) {
-llvm::GlobalObject::VCallVisibility TypeVis = GetVCallVisibilityLevel(RD);
+llvm::DenseSet Visited;
+llvm::GlobalObject::VCallVisibility TypeVis =
+GetVCallVisibilityLevel(RD, Visited);
 if (TypeVis != llvm::GlobalObject::VCallVisibilityPublic)
   VTable->setVCallVisibilityMetadata(TypeVis);
   }


Index: clang/lib/CodeGen/MicrosoftCXXABI.cpp
===
--- clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1649,8 +1649,9 @@
   // TODO: Should VirtualFunctionElimination also be supported here?
   // See similar handling in CodeGenModule::EmitVTableTypeMetadata.
   if (CGM.getCodeGenOpts().WholeProgramVTables) {
+llvm::DenseSet Visited;
 llvm::GlobalObject::VCallVisibility TypeVis =
-CGM.GetVCallVisibilityLevel(RD);
+CGM.GetVCallVisibilityLevel(RD, Visited);
 if (TypeVis != llvm::GlobalObject::VCallVisibilityPublic)
   VTable->setVCallVisibilityMetadata(TypeVis);
   }
Index: clang/lib/CodeGen/CodeGenModule.h
==

[PATCH] D90441: [X86] Add support for vex, vex2, vex3, and evex for MASM

2020-11-17 Thread LiuChen via Phabricator via cfe-commits

LiuChen3 updated this revision to Diff 305959.
LiuChen3 added a comment.

1. Check prefix, ignoring case
2. Delete IsPrefix parameter, and delete 'break', so that we won't check prefix 
again. I am not sure if this is right. Att format can allow two prefix and 
using the last one as the finally encoding prefix. I think this may not be the 
original intention of the design.
3. Change the test: checking the IR istead of checking the assembly.
4. Made some format adjustments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90441

Files:
  clang/lib/AST/Stmt.cpp
  clang/test/CodeGen/X86/ms-inline-asm-prefix.c
  llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

Index: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
===
--- llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
+++ llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
@@ -3064,7 +3064,26 @@
   }
   continue;
 }
+// Parse MASM style pseudo prefixes.
+if (isParsingMSInlineAsm()) {
+  if (Name.equals_lower("vex"))
+ForcedVEXEncoding = VEXEncoding_VEX;
+  else if (Name.equals_lower("vex2"))
+ForcedVEXEncoding = VEXEncoding_VEX2;
+  else if (Name.equals_lower("vex3"))
+ForcedVEXEncoding = VEXEncoding_VEX3;
+  else if (Name.equals_lower("evex"))
+ForcedVEXEncoding = VEXEncoding_EVEX;
 
+  if (ForcedVEXEncoding != VEXEncoding_Default) {
+if (getLexer().isNot(AsmToken::Identifier))
+  return Error(Parser.getTok().getLoc(), "Expected identifier");
+// FIXME: The mnemonic won't match correctly if its not in lower case.
+Name = Parser.getTok().getString();
+NameLoc = Parser.getTok().getLoc();
+Parser.Lex();
+  }
+}
 break;
   }
 
@@ -4370,10 +4389,16 @@
 
   MCInst Inst;
 
-  // If VEX3 encoding is forced, we need to pass the USE_VEX3 flag to the
-  // encoder.
-  if (ForcedVEXEncoding == VEXEncoding_VEX3)
+  // If VEX/EVEX encoding is forced, we need to pass the USE_* flag to the
+  // encoder and printer.
+  if (ForcedVEXEncoding == VEXEncoding_VEX)
+Prefixes |= X86::IP_USE_VEX;
+  else if (ForcedVEXEncoding == VEXEncoding_VEX2)
+Prefixes |= X86::IP_USE_VEX2;
+  else if (ForcedVEXEncoding == VEXEncoding_VEX3)
 Prefixes |= X86::IP_USE_VEX3;
+  else if (ForcedVEXEncoding == VEXEncoding_EVEX)
+Prefixes |= X86::IP_USE_EVEX;
 
   // Set encoded flags for {disp8} and {disp32}.
   if (ForcedDispEncoding == DispEncoding_Disp8)
Index: clang/test/CodeGen/X86/ms-inline-asm-prefix.c
===
--- /dev/null
+++ clang/test/CodeGen/X86/ms-inline-asm-prefix.c
@@ -0,0 +1,14 @@
+// REQUIRES: x86-registered-target
+// RUN:%clang_cc1 %s -ferror-limit 0 -triple=x86_64-pc-windows-msvc -target-feature +avx512f -target-feature +avx2 -target-feature +avx512vl -fasm-blocks -mllvm -x86-asm-syntax=intel -S -emit-llvm -o -  | FileCheck %s -check-prefix=INTEL
+// RUN:%clang_cc1 %s -ferror-limit 0 -triple=x86_64-pc-windows-msvc -target-feature +avx512f -target-feature +avx2 -target-feature +avx512vl -fasm-blocks -mllvm -x86-asm-syntax=att -S -emit-llvm -o -  | FileCheck %s -check-prefix=ATT
+
+void check_inline_prefix(void) {
+  __asm {
+// INTEL: call void asm sideeffect inteldialect "{vex} vcvtps2pd xmm0, xmm1\0A\09{vex2} vcvtps2pd xmm0, xmm1\0A\09{vex3} vcvtps2pd xmm0, xmm1\0A\09{evex} vcvtps2pd xmm0, xmm1", "~{xmm0},~{dirflag},~{fpsr},~{flags}"()
+// ATT: call void asm sideeffect inteldialect "{vex} vcvtps2pd xmm0, xmm1\0A\09{vex2} vcvtps2pd xmm0, xmm1\0A\09{vex3} vcvtps2pd xmm0, xmm1\0A\09{evex} vcvtps2pd xmm0, xmm1", "~{xmm0},~{dirflag},~{fpsr},~{flags}"()
+vex vcvtps2pd xmm0, xmm1
+vex2 vcvtps2pd xmm0, xmm1
+vex3 vcvtps2pd xmm0, xmm1
+evex vcvtps2pd xmm0, xmm1
+  }
+}
Index: clang/lib/AST/Stmt.cpp
===
--- clang/lib/AST/Stmt.cpp
+++ clang/lib/AST/Stmt.cpp
@@ -791,7 +791,27 @@
 /// Assemble final IR asm string (MS-style).
 std::string MSAsmStmt::generateAsmString(const ASTContext &C) const {
   // FIXME: This needs to be translated into the IR string representation.
-  return std::string(AsmStr);
+  SmallVector Pieces;
+  AsmStr.split(Pieces, "\n\t");
+  std::string MSAsmString;
+  for (size_t I = 0, E = Pieces.size(); I < E; ++I) {
+StringRef Instruction = Pieces[I];
+// For vex/vex2/vex3/evex masm style prefix, convert it to att style
+// since we don't support masm style prefix in backend.
+if (Instruction.startswith("vex "))
+  MSAsmString += '{' + Instruction.substr(0, 3).str() + '}' +
+ Instruction.substr(3).str();
+else if (Instruction.startswith("vex2 ") ||
+ Instruction.startswith("vex3 ") || Instruction.startswith("evex "))
+  MSAsmString += '{' + Instruction.substr(0, 4).str() + '}' +
+

[clang] 6a89cb8 - Revert "Revert "Revert "[analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis."""

2020-11-17 Thread Artem Dergachev via cfe-commits


Author: Artem Dergachev
Date: 2020-11-17T18:59:21-08:00
New Revision: 6a89cb8136f3435bd977b419b683dc0acc98e61e

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

LOG: Revert "Revert "Revert "[analyzer] NFC: Move path diagnostic consumer 
implementations to libAnalysis."""

This reverts commit 41bcc05e2a4e3062eb12ac6e071bc835decc38f5.

Added: 
clang/include/clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h
clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp

Modified: 
clang/include/clang/StaticAnalyzer/Core/Analyses.def
clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
clang/include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h
clang/include/clang/module.modulemap
clang/lib/Analysis/CMakeLists.txt
clang/lib/Frontend/CompilerInvocation.cpp
clang/lib/StaticAnalyzer/Core/CMakeLists.txt
clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

Removed: 
clang/include/clang/Analysis/PathDiagnosticConsumers.def
clang/include/clang/Analysis/PathDiagnosticConsumers.h
clang/lib/Analysis/HTMLPathDiagnosticConsumer.cpp
clang/lib/Analysis/PlistHTMLPathDiagnosticConsumer.cpp
clang/lib/Analysis/PlistPathDiagnosticConsumer.cpp
clang/lib/Analysis/SarifPathDiagnosticConsumer.cpp
clang/lib/Analysis/TextPathDiagnosticConsumer.cpp



diff  --git a/clang/include/clang/Analysis/PathDiagnosticConsumers.def 
b/clang/include/clang/Analysis/PathDiagnosticConsumers.def
deleted file mode 100644
index 33d2072fcf31..
--- a/clang/include/clang/Analysis/PathDiagnosticConsumers.def
+++ /dev/null
@@ -1,50 +0,0 @@
-//===-- PathDiagnosticConsumers.def - Visualizing warnings --*- C++ 
-*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===--===//
-//
-// This file defines the set of path diagnostic consumers - objects that
-// implement 
diff erent representations of static analysis results.
-//
-//===--===//
-
-#ifndef ANALYSIS_DIAGNOSTICS
-#define ANALYSIS_DIAGNOSTICS(NAME, CMDFLAG, DESC, CREATEFN)
-#endif
-
-ANALYSIS_DIAGNOSTICS(HTML, "html", "Output analysis results using HTML",
- createHTMLDiagnosticConsumer)
-
-ANALYSIS_DIAGNOSTICS(
-HTML_SINGLE_FILE, "html-single-file",
-"Output analysis results using HTML (not allowing for multi-file bugs)",
-createHTMLSingleFileDiagnosticConsumer)
-
-ANALYSIS_DIAGNOSTICS(PLIST, "plist", "Output analysis results using Plists",
- createPlistDiagnosticConsumer)
-
-ANALYSIS_DIAGNOSTICS(
-PLIST_MULTI_FILE, "plist-multi-file",
-"Output analysis results using Plists (allowing for multi-file bugs)",
-createPlistMultiFileDiagnosticConsumer)
-
-ANALYSIS_DIAGNOSTICS(PLIST_HTML, "plist-html",
- "Output analysis results using HTML wrapped with Plists",
- createPlistHTMLDiagnosticConsumer)
-
-ANALYSIS_DIAGNOSTICS(SARIF, "sarif", "Output analysis results in a SARIF file",
- createSarifDiagnosticConsumer)
-
-ANALYSIS_DIAGNOSTICS(TEXT, "text", "Text output of analysis results to stderr",
- createTextPathDiagnosticConsumer)
-
-ANALYSIS_DIAGNOSTICS(TEXT_MINIMAL, "text-minimal",
- "Emits minimal diagnostics to stderr, stating only the "
- "warning message and the associated notes. Usually "
- "used in addition to other analysis types",
- createTextMinimalPathDiagnosticConsumer)
-
-#undef ANALYSIS_DIAGNOSTICS

diff  --git a/clang/include/clang/StaticAnalyzer/Core/Analyses.def 
b/clang/include/clang/StaticAnalyzer/Core/Analyses.def
index 2e98cbba4c9e..c4e5f5be6fd7 100644
--- a/clang/include/clang/StaticAnalyzer/Core/Analyses.def
+++ b/clang/include/clang/StaticAnalyzer/Core/Analyses.def
@@ -28,6 +28,42 @@ ANALYSIS_CONSTRAINTS(RangeConstraints, "range",
 ANALYSIS_CONSTRAINTS(Z3Constraints, "z3", "Use Z3 contraint solver",
  CreateZ3ConstraintManager)
 
+#ifndef ANALYSIS_DIAGNOSTICS
+#define ANALYSIS_DIAGNOSTICS(NAME, CMDFLAG, DESC, CREATEFN)
+#endif
+
+ANALYSIS_DIAGNOSTICS(HTML, "html", "Output analysis results using HTML",
+ createHTMLDiagnosticConsumer)
+
+ANALYSIS_DIAGNOSTICS(
+HTML_SINGLE_FILE, "html-single-file",
+"Output analysis results using HTML (not allowing for mult

[clang] 41bcc05 - Revert "Revert "[analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis.""

2020-11-17 Thread Artem Dergachev via cfe-commits


Author: Artem Dergachev
Date: 2020-11-17T18:45:09-08:00
New Revision: 41bcc05e2a4e3062eb12ac6e071bc835decc38f5

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

LOG: Revert "Revert "[analyzer] NFC: Move path diagnostic consumer 
implementations to libAnalysis.""

This reverts commit 77bb3ebebbca13f0648beb433fbd1b06ba95a19c.

Added: 
clang/include/clang/Analysis/PathDiagnosticConsumers.def
clang/include/clang/Analysis/PathDiagnosticConsumers.h
clang/lib/Analysis/HTMLPathDiagnosticConsumer.cpp
clang/lib/Analysis/PlistHTMLPathDiagnosticConsumer.cpp
clang/lib/Analysis/PlistPathDiagnosticConsumer.cpp
clang/lib/Analysis/SarifPathDiagnosticConsumer.cpp
clang/lib/Analysis/TextPathDiagnosticConsumer.cpp

Modified: 
clang/include/clang/StaticAnalyzer/Core/Analyses.def
clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
clang/include/clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h
clang/include/clang/module.modulemap
clang/lib/Analysis/CMakeLists.txt
clang/lib/Frontend/CompilerInvocation.cpp
clang/lib/StaticAnalyzer/Core/CMakeLists.txt
clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp

Removed: 
clang/include/clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h
clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
clang/lib/StaticAnalyzer/Core/SarifDiagnostics.cpp
clang/lib/StaticAnalyzer/Core/TextDiagnostics.cpp



diff  --git a/clang/include/clang/Analysis/PathDiagnosticConsumers.def 
b/clang/include/clang/Analysis/PathDiagnosticConsumers.def
new file mode 100644
index ..33d2072fcf31
--- /dev/null
+++ b/clang/include/clang/Analysis/PathDiagnosticConsumers.def
@@ -0,0 +1,50 @@
+//===-- PathDiagnosticConsumers.def - Visualizing warnings --*- C++ 
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file defines the set of path diagnostic consumers - objects that
+// implement 
diff erent representations of static analysis results.
+//
+//===--===//
+
+#ifndef ANALYSIS_DIAGNOSTICS
+#define ANALYSIS_DIAGNOSTICS(NAME, CMDFLAG, DESC, CREATEFN)
+#endif
+
+ANALYSIS_DIAGNOSTICS(HTML, "html", "Output analysis results using HTML",
+ createHTMLDiagnosticConsumer)
+
+ANALYSIS_DIAGNOSTICS(
+HTML_SINGLE_FILE, "html-single-file",
+"Output analysis results using HTML (not allowing for multi-file bugs)",
+createHTMLSingleFileDiagnosticConsumer)
+
+ANALYSIS_DIAGNOSTICS(PLIST, "plist", "Output analysis results using Plists",
+ createPlistDiagnosticConsumer)
+
+ANALYSIS_DIAGNOSTICS(
+PLIST_MULTI_FILE, "plist-multi-file",
+"Output analysis results using Plists (allowing for multi-file bugs)",
+createPlistMultiFileDiagnosticConsumer)
+
+ANALYSIS_DIAGNOSTICS(PLIST_HTML, "plist-html",
+ "Output analysis results using HTML wrapped with Plists",
+ createPlistHTMLDiagnosticConsumer)
+
+ANALYSIS_DIAGNOSTICS(SARIF, "sarif", "Output analysis results in a SARIF file",
+ createSarifDiagnosticConsumer)
+
+ANALYSIS_DIAGNOSTICS(TEXT, "text", "Text output of analysis results to stderr",
+ createTextPathDiagnosticConsumer)
+
+ANALYSIS_DIAGNOSTICS(TEXT_MINIMAL, "text-minimal",
+ "Emits minimal diagnostics to stderr, stating only the "
+ "warning message and the associated notes. Usually "
+ "used in addition to other analysis types",
+ createTextMinimalPathDiagnosticConsumer)
+
+#undef ANALYSIS_DIAGNOSTICS

diff  --git a/clang/include/clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h 
b/clang/include/clang/Analysis/PathDiagnosticConsumers.h
similarity index 89%
rename from clang/include/clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h
rename to clang/include/clang/Analysis/PathDiagnosticConsumers.h
index f40f88eb32ff..9f23bea1b4c1 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathDiagnosticConsumers.h
+++ b/clang/include/clang/Analysis/PathDiagnosticConsumers.h
@@ -18,6 +18,8 @@
 #include 
 #include 
 
+#include "clang/Analysis/PathDiagnostic.h"
+
 namespace clang {
 
 class AnalyzerOptions;
@@ -29,14 +31,14 @@ class CrossTranslationUnitContext;
 namespace ento {
 
 class PathDiagnosticConsumer;
-typedef std::vector PathDiagnosticConsumers;
+typedef std::vector PathDiagnosticConsumers;
 
 #define ANALYSIS_DIAGNOSTICS(NA

[PATCH] D17993: [CodeGen] Apply 'nonnull' and 'dereferenceable(N)' to 'this' pointer arguments.

2020-11-17 Thread John McCall via Phabricator via cfe-commits

rjmccall added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:2169
+if (!CodeGenOpts.NullPointerIsValid &&
+getContext().getTargetAddressSpace(FI.arg_begin()->type) == 0) {
+  Attrs.addAttribute(llvm::Attribute::NonNull);

rsmith wrote:
> jdoerfert wrote:
> > rjmccall wrote:
> > > rsmith wrote:
> > > > jdoerfert wrote:
> > > > > arichardson wrote:
> > > > > > Isn't the `this` pointer also nonnull in other address spaces?
> > > > > > 
> > > > > > In our CHERI fork we use AS200 for the this pointer and would quite 
> > > > > > like to have the nonnull attribute.
> > > > > > I can obviously change this line locally when I next merge from 
> > > > > > upstream, but I would like to avoid diffs and it seems to me like 
> > > > > > this restriction is unnecessary.
> > > > > I also think `NullPointerIsValid` is sufficient. 
> > > > It's my understanding that:
> > > > * The LLVM `null` value in any address space is the all-zero-bits value.
> > > > * In address space zero, the `null` value does not correspond to 
> > > > addressable memory, but this is not assumed to hold in other address 
> > > > spaces.
> > > > * An address-space-zero `null` value that is addressspacecast to a 
> > > > different address space might not be the `null` in the target address 
> > > > space.
> > > > * The `nonnull` attribute implies that the pointer value is not the 
> > > > `null` value.
> > > > * A null pointer in the frontend in a non-zero address space 
> > > > corresponds to the value produced by an addressspacecast of an 
> > > > address-space-zero `null` value to the target address space.
> > > > 
> > > > That being the case, there is simply no connection between the C and 
> > > > C++ notion of a null pointer and a `null` LLVM pointer value in a 
> > > > non-zero address space in general, so it is not correct to use the 
> > > > `nonnull` attribute in a non-zero address space in general. Only if we 
> > > > know that a C++ null pointer is actually represented by the LLVM `null` 
> > > > value in the corresponding address space can we use the `nonnull` 
> > > > attribute to expose that fact to LLVM. And we do not know that in 
> > > > general.
> > > I think all of this is correct except that the frontend does know what 
> > > the bit-pattern of the null pointer is in any particular language-level 
> > > address space, and it knows what the language-level address space of 
> > > `this` is.  So we should be able to ask whether the null value in the 
> > > `this` address space is the all-zero value and use that to decide whether 
> > > to apply `nonnull`.
> > Hm, I think the problem is that we don't couple `NullPointerIsValid` with 
> > the address space. As I said before. In LLVM-IR, if we don't have the 
> > `null-pointer-is-valid` property, then "memory access" implies 
> > `dereferenceable` implies `nonnull`. Now, we usually assume non-0 address 
> > spaces to have the above property, but that should be decoupled. The query 
> > if "null is valid" should take the function and the AS, as it does 
> > conceptually in LLVM-core, and then decide if `null-pointer-is-valid` or 
> > not. If we had that, @arichardson could define that AS200 does not have 
> > valid null pointers. If you do that right now the IR passes will at least 
> > deduce `nonnull` based on `derferenceable`.
> > I think all of this is correct except that the frontend does know what the 
> > bit-pattern of the null pointer is in any particular language-level address 
> > space
> 
> Interesting. How do we get at that? Do we ask the middle-end to constant-fold 
> `addrspacecast i8* null to i8* addrspace(N)` and see if we get a `null` back, 
> or is there a better way?
> 
> In any case, this patch follows the same pattern we use for return values of 
> reference type, parameters of reference type, and decayed array function 
> parameters with static array bounds, all of which apply `nonnull` only in 
> address space 0. If we want to use a different pattern, to consider whether 
> LLVM's `nonnull` means "not a null pointer" rather than assuming that holds 
> only in address space 0, we should make a holistic change for that throughout 
> CGCall.cpp, rather than touching only the handling of the `this` pointer.
> 
> It'd also be interesting to consider what we want `__attribute__((nonnull))` 
> to mean in address spaces where the null pointer isn't the zero pointer: 
> should it mean non-zero at the source level / non-null in LLVM IR, or should 
> it mean non-null at the source level (which might be unrepresentable in LLVM 
> IR, but static analysis etc. could still detect it)? We're currently 
> inconsistent on this: static analysis, constant evaluation, and sanitizers 
> treat the attribute as meaning non-null, but IR generation emits the 
> `nonnull` IR attribute, treating it as meaning non-zero instead.
> Interesting. How do we get at that? Do we ask the middle-end to constant-fold 
> addrspacecast i8* nu

LLVM buildmaster will be updated and restarted tonight

2020-11-17 Thread Galina Kistanova via cfe-commits

 Hello everyone,

LLVM buildmaster will be updated and restarted after 8PM PST today.

Thanks

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

[PATCH] D91673: [PGO] Enable preinline and cleanup when optimize for size

2020-11-17 Thread Zequan Wu via Phabricator via cfe-commits

zequanwu updated this revision to Diff 305948.
zequanwu added a comment.
Herald added subscribers: llvm-commits, hiraditya.
Herald added a project: LLVM.

update wrong diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91673

Files:
  llvm/lib/Transforms/IPO/PassManagerBuilder.cpp


Index: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
===
--- llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
+++ llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
@@ -330,10 +330,8 @@
 return;
 
   // Perform the preinline and cleanup passes for O1 and above.
-  // And avoid doing them if optimizing for size.
   // We will not do this inline for context sensitive PGO (when IsCS is true).
-  if (OptLevel > 0 && SizeLevel == 0 && !DisablePreInliner &&
-  PGOSampleUse.empty() && !IsCS) {
+  if (OptLevel > 0 && !DisablePreInliner && PGOSampleUse.empty() && !IsCS) {
 // Create preinline pass. We construct an InlineParams object and specify
 // the threshold here to avoid the command line options of the regular
 // inliner to influence pre-inlining. The only fields of InlineParams we
@@ -342,7 +340,7 @@
 IP.DefaultThreshold = PreInlineThreshold;
 // FIXME: The hint threshold has the same value used by the regular 
inliner.
 // This should probably be lowered after performance testing.
-IP.HintThreshold = 325;
+IP.HintThreshold = SizeLevel > 0 ? 75 : 325;
 
 MPM.add(createFunctionInliningPass(IP));
 MPM.add(createSROAPass());


Index: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
===
--- llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
+++ llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
@@ -330,10 +330,8 @@
 return;
 
   // Perform the preinline and cleanup passes for O1 and above.
-  // And avoid doing them if optimizing for size.
   // We will not do this inline for context sensitive PGO (when IsCS is true).
-  if (OptLevel > 0 && SizeLevel == 0 && !DisablePreInliner &&
-  PGOSampleUse.empty() && !IsCS) {
+  if (OptLevel > 0 && !DisablePreInliner && PGOSampleUse.empty() && !IsCS) {
 // Create preinline pass. We construct an InlineParams object and specify
 // the threshold here to avoid the command line options of the regular
 // inliner to influence pre-inlining. The only fields of InlineParams we
@@ -342,7 +340,7 @@
 IP.DefaultThreshold = PreInlineThreshold;
 // FIXME: The hint threshold has the same value used by the regular inliner.
 // This should probably be lowered after performance testing.
-IP.HintThreshold = 325;
+IP.HintThreshold = SizeLevel > 0 ? 75 : 325;
 
 MPM.add(createFunctionInliningPass(IP));
 MPM.add(createSROAPass());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91673: [PGO] Enable preinline and cleanup when optimize for size

2020-11-17 Thread Zequan Wu via Phabricator via cfe-commits

zequanwu created this revision.
zequanwu added reviewers: xur, davidxl, rnk.
Herald added subscribers: cfe-commits, wenlei.
Herald added a project: clang.
zequanwu requested review of this revision.

This is intended to reduce the binary size of both phase 1 and phase 2 builds.
By compiling chrome on Windows (using -Os in both phases), the binary size of 
phase 1 is reduced by 27.5% and binary size of phase 2 is reduced by 8.4%.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91673

Files:
  clang/lib/Frontend/InitPreprocessor.cpp


Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -403,6 +403,11 @@
 Builder.defineMacro("__STDCPP_DEFAULT_NEW_ALIGNMENT__",
 Twine(TI.getNewAlign() / TI.getCharWidth()) +
 TI.getTypeConstantSuffix(TI.getSizeType()));
+
+//   -- __STDCPP_THREADS__
+//  Defined, and has the value integer literal 1, if and only if a
+//  program can have more than one thread of execution.
+Builder.defineMacro("__STDCPP_THREADS__", "1");
   }
 
   // In C11 these are environment macros. In C++11 they are only defined


Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -403,6 +403,11 @@
 Builder.defineMacro("__STDCPP_DEFAULT_NEW_ALIGNMENT__",
 Twine(TI.getNewAlign() / TI.getCharWidth()) +
 TI.getTypeConstantSuffix(TI.getSizeType()));
+
+//   -- __STDCPP_ÂTHREADS__
+//  Defined, and has the value integer literal 1, if and only if a
+//  program can have more than one thread of execution.
+Builder.defineMacro("__STDCPP_ÂTHREADS__", "1");
   }
 
   // In C11 these are environment macros. In C++11 they are only defined
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91580: [Frontend] Add flag to allow PCM generation despite compiler errors

2020-11-17 Thread Argyrios Kyrtzidis via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG5834996fefc9: [Frontend] Add flag to allow PCM generation 
despite compiler errors (authored by bnbarham, committed by akyrtzi).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91580

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/ASTUnit.h
  clang/include/clang/Frontend/FrontendActions.h
  clang/include/clang/Frontend/FrontendOptions.h
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/test/Modules/Inputs/error.h
  clang/test/Modules/Inputs/module.map
  clang/test/Modules/load-module-with-errors.m
  clang/tools/c-index-test/core_main.cpp
  clang/tools/libclang/CIndex.cpp

Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -3476,7 +3476,7 @@
   ast_filename, CXXIdx->getPCHContainerOperations()->getRawReader(),
   ASTUnit::LoadEverything, Diags, FileSystemOpts, /*UseDebugInfo=*/false,
   CXXIdx->getOnlyLocalDecls(), None, CaptureDiagsKind::All,
-  /*AllowPCHWithCompilerErrors=*/true,
+  /*AllowASTWithCompilerErrors=*/true,
   /*UserFilesAreVolatile=*/true);
   *out_TU = MakeCXTranslationUnit(CXXIdx, std::move(AU));
   return *out_TU ? CXError_Success : CXError_Failure;
Index: clang/tools/c-index-test/core_main.cpp
===
--- clang/tools/c-index-test/core_main.cpp
+++ clang/tools/c-index-test/core_main.cpp
@@ -263,7 +263,7 @@
   std::string(modulePath), *pchRdr, ASTUnit::LoadASTOnly, Diags,
   FileSystemOpts, /*UseDebugInfo=*/false,
   /*OnlyLocalDecls=*/true, None, CaptureDiagsKind::None,
-  /*AllowPCHWithCompilerErrors=*/true,
+  /*AllowASTWithCompilerErrors=*/true,
   /*UserFilesAreVolatile=*/false);
   if (!AU) {
 errs() << "failed to create TU for: " << modulePath << '\n';
Index: clang/test/Modules/load-module-with-errors.m
===
--- /dev/null
+++ clang/test/Modules/load-module-with-errors.m
@@ -0,0 +1,25 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+
+// Write out a module with errors make sure it can be read
+// RUN: %clang_cc1 -fmodules -fallow-pcm-with-compiler-errors \
+// RUN:   -fmodules-cache-path=%t -x objective-c -emit-module \
+// RUN:   -fmodule-name=error %S/Inputs/module.map
+// RUN: %clang_cc1 -fmodules -fallow-pcm-with-compiler-errors \
+// RUN:   -fmodules-cache-path=%t -x objective-c -I %S/Inputs \
+// RUN:   -fimplicit-module-maps -ast-print %s | FileCheck %s
+
+// allow-pcm-with-compiler-errors should also allow errors in PCH
+// RUN: %clang_cc1 -fallow-pcm-with-compiler-errors -x c++ -emit-pch \
+// RUN:   -o %t/check.pch %S/Inputs/error.h
+
+@import error;
+
+void test(id x) {
+  [x method];
+}
+
+// CHECK: @interface Error
+// CHECK-NEXT: - (int)method;
+// CHECK-NEXT: @end
+// CHECK: void test(id x)
Index: clang/test/Modules/Inputs/module.map
===
--- clang/test/Modules/Inputs/module.map
+++ clang/test/Modules/Inputs/module.map
@@ -483,3 +483,4 @@
   header "template-nontrivial1.h"
   export *
 }
+module error { header "error.h" }
Index: clang/test/Modules/Inputs/error.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/error.h
@@ -0,0 +1,8 @@
+@import undefined
+
+@interface Error
+- (int)method;
+undefined
+@end
+
+undefined
Index: clang/lib/Frontend/FrontendActions.cpp
===
--- clang/lib/Frontend/FrontendActions.cpp
+++ clang/lib/Frontend/FrontendActions.cpp
@@ -177,7 +177,8 @@
   Consumers.push_back(std::make_unique(
   CI.getPreprocessor(), CI.getModuleCache(), OutputFile, Sysroot, Buffer,
   CI.getFrontendOpts().ModuleFileExtensions,
-  /*AllowASTWithErrors=*/false,
+  /*AllowASTWithErrors=*/
+  +CI.getFrontendOpts().AllowPCMWithCompilerErrors,
   /*IncludeTimestamps=*/
   +CI.getFrontendOpts().BuildingImplicitModule,
   /*ShouldCacheASTInMemory=*/
@@ -187,6 +188,11 @@
   return std::make_unique(std::move(Consumers));
 }
 
+bool GenerateModuleAction::shouldEraseOutputFiles() {
+  return !getCompilerInstance().getFrontendOpts().AllowPCMWithCompilerErrors &&
+ ASTFrontendAction::shouldEraseOutputFiles();
+}
+
 bool GenerateModuleFromModuleMapAction::BeginSourceFileAction(
 CompilerInstance &CI) {
   if (!CI.getLangOpts().Modules) {
@@ -339,7 +345,7 @@
   CI.getPCHContainerReader(), CI.getFrontendOpts().ModuleFileExtensions,
   Sysroot.empty() ? "" : Sysroot.c_str(),
   /*DisableValidation*/ false,
-  /*AllowPCHWithComp

[clang] 5834996 - [Frontend] Add flag to allow PCM generation despite compiler errors

2020-11-17 Thread Argyrios Kyrtzidis via cfe-commits


Author: Ben Barham
Date: 2020-11-17T17:27:50-08:00
New Revision: 5834996fefc937d6211dc8c8a5b200068753391a

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

LOG: [Frontend] Add flag to allow PCM generation despite compiler errors

As with precompiled headers, it's useful for indexers to be able to
continue through compiler errors in dependent modules.

Resolves rdar://69816264

Reviewed By: akyrtzi

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

Added: 
clang/test/Modules/Inputs/error.h
clang/test/Modules/load-module-with-errors.m

Modified: 
clang/include/clang/Driver/Options.td
clang/include/clang/Frontend/ASTUnit.h
clang/include/clang/Frontend/FrontendActions.h
clang/include/clang/Frontend/FrontendOptions.h
clang/lib/Frontend/ASTUnit.cpp
clang/lib/Frontend/CompilerInstance.cpp
clang/lib/Frontend/CompilerInvocation.cpp
clang/lib/Frontend/FrontendActions.cpp
clang/test/Modules/Inputs/module.map
clang/tools/c-index-test/core_main.cpp
clang/tools/libclang/CIndex.cpp

Removed: 




diff  --git a/clang/include/clang/Driver/Options.td 
b/clang/include/clang/Driver/Options.td
index 0168d7000737..feae81317047 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -4356,6 +4356,8 @@ def fno_validate_pch : Flag<["-"], "fno-validate-pch">,
   HelpText<"Disable validation of precompiled headers">;
 def fallow_pch_with_errors : Flag<["-"], "fallow-pch-with-compiler-errors">,
   HelpText<"Accept a PCH file that was created with compiler errors">;
+def fallow_pcm_with_errors : Flag<["-"], "fallow-pcm-with-compiler-errors">,
+  HelpText<"Accept a PCM file that was created with compiler errors">;
 def dump_deserialized_pch_decls : Flag<["-"], "dump-deserialized-decls">,
   HelpText<"Dump declarations that are deserialized from PCH, for testing">;
 def error_on_deserialized_pch_decl : Separate<["-"], 
"error-on-deserialized-decl">,

diff  --git a/clang/include/clang/Frontend/ASTUnit.h 
b/clang/include/clang/Frontend/ASTUnit.h
index 5595b8c7bcae..5bee57042ca6 100644
--- a/clang/include/clang/Frontend/ASTUnit.h
+++ b/clang/include/clang/Frontend/ASTUnit.h
@@ -694,7 +694,7 @@ class ASTUnit {
   const FileSystemOptions &FileSystemOpts, bool UseDebugInfo = false,
   bool OnlyLocalDecls = false, ArrayRef RemappedFiles = None,
   CaptureDiagsKind CaptureDiagnostics = CaptureDiagsKind::None,
-  bool AllowPCHWithCompilerErrors = false,
+  bool AllowASTWithCompilerErrors = false,
   bool UserFilesAreVolatile = false);
 
 private:

diff  --git a/clang/include/clang/Frontend/FrontendActions.h 
b/clang/include/clang/Frontend/FrontendActions.h
index 9ca2bfda2138..25ca95980806 100644
--- a/clang/include/clang/Frontend/FrontendActions.h
+++ b/clang/include/clang/Frontend/FrontendActions.h
@@ -117,6 +117,8 @@ class GenerateModuleAction : public ASTFrontendAction {
   }
 
   bool hasASTFileSupport() const override { return false; }
+
+  bool shouldEraseOutputFiles() override;
 };
 
 class GenerateInterfaceStubsAction : public ASTFrontendAction {

diff  --git a/clang/include/clang/Frontend/FrontendOptions.h 
b/clang/include/clang/Frontend/FrontendOptions.h
index 51dc3a0dc7f1..b06ad5203e75 100644
--- a/clang/include/clang/Frontend/FrontendOptions.h
+++ b/clang/include/clang/Frontend/FrontendOptions.h
@@ -303,6 +303,9 @@ class FrontendOptions {
   /// When using -emit-module, treat the modulemap as a system module.
   unsigned IsSystemModule : 1;
 
+  /// Output (and read) PCM files regardless of compiler errors.
+  unsigned AllowPCMWithCompilerErrors : 1;
+
   CodeCompleteOptions CodeCompleteOpts;
 
   /// Specifies the output format of the AST.
@@ -457,7 +460,8 @@ class FrontendOptions {
 UseGlobalModuleIndex(true), GenerateGlobalModuleIndex(true),
 ASTDumpDecls(false), ASTDumpLookups(false),
 BuildingImplicitModule(false), ModulesEmbedAllFiles(false),
-IncludeTimestamps(true), UseTemporary(true), TimeTraceGranularity(500) 
{}
+IncludeTimestamps(true), UseTemporary(true),
+AllowPCMWithCompilerErrors(false), TimeTraceGranularity(500) {}
 
   /// getInputKindForExtension - Return the appropriate input kind for a file
   /// extension. For example, "c" would return Language::C.

diff  --git a/clang/lib/Frontend/ASTUnit.cpp b/clang/lib/Frontend/ASTUnit.cpp
index a112117d1c06..c2aba4102361 100644
--- a/clang/lib/Frontend/ASTUnit.cpp
+++ b/clang/lib/Frontend/ASTUnit.cpp
@@ -759,7 +759,7 @@ std::unique_ptr ASTUnit::LoadFromASTFile(
 WhatToLoad ToLoad, IntrusiveRefCntPtr Diags,
 const FileSystemOptions &FileSystemOpts, bool UseDebugInfo,
 bool OnlyLocalDecls, ArrayRef RemappedFiles,
-CaptureDiagsKind CaptureDiagnostics, bool AllowPCHWithCom

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

2020-11-17 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

Thanks for the walkthroughs/help. Also stared at the code a bit. I think I get 
it now. Some of the confusion also came from having both LPM and NPM versions 
of the always inliner in the same file, though they seem to share no code.

I'll leave the more nuanced review to folks more familiar with it - sorry for 
any noise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91567

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

[PATCH] D87956: [IR] add fn attr for no_stack_protector; prevent inlining on mismatch

2020-11-17 Thread Nick Desaulniers via Phabricator via cfe-commits

nickdesaulniers added inline comments.



Comment at: llvm/test/Transforms/Inline/inline_nossp.ll:3
+; RUN: opt -passes='cgscc(inline)' %s -S | FileCheck %s
+; RUN: opt -always-inline -o - -S %s | FileCheck %s
+

nickdesaulniers wrote:
> aeubanks wrote:
> > nickdesaulniers wrote:
> > > aeubanks wrote:
> > > > This test fails with the NPM,
> > > > `opt -passes=always-inline ...`
> > > > 
> > > > Does `llvm::isInlineViable()` need to be updated?
> > > Thanks for the report.  Is there a Cmake configuration I need to 
> > > explicitly set to reproduce? `LLVM_USE_NEWPM`?
> > Just a new RUN line: `RUN: opt -passes=always-inline -o - -S %s | FileCheck 
> > %s`
> I think I'm going to back out this patch and the fix ups, and pursue a much 
> simpler approach: 
> https://github.com/ClangBuiltLinux/llvm-project/commit/501847c5239be52bbe32a9c5bd0723e4d0ad2990.
reverted in f4c6080ab820219c5bf78b0c2143e7fa194da296


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87956

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

[clang] f4c6080 - Revert "[IR] add fn attr for no_stack_protector; prevent inlining on mismatch"

2020-11-17 Thread Nick Desaulniers via cfe-commits


Author: Nick Desaulniers
Date: 2020-11-17T17:27:14-08:00
New Revision: f4c6080ab820219c5bf78b0c2143e7fa194da296

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

LOG: Revert "[IR] add fn attr for no_stack_protector; prevent inlining on 
mismatch"

This reverts commit b7926ce6d7a83cdf70c68d82bc3389c04009b841.

Going with a simpler approach.

Added: 


Modified: 
clang/include/clang/Basic/AttrDocs.td
clang/lib/CodeGen/CodeGenModule.cpp
clang/test/CodeGen/stack-protector.c
llvm/bindings/go/llvm/ir_test.go
llvm/bindings/ocaml/llvm/llvm.ml
llvm/docs/BitCodeFormat.rst
llvm/docs/LangRef.rst
llvm/include/llvm/Bitcode/LLVMBitCodes.h
llvm/include/llvm/IR/Attributes.td
llvm/lib/AsmParser/LLLexer.cpp
llvm/lib/AsmParser/LLParser.cpp
llvm/lib/AsmParser/LLToken.h
llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
llvm/lib/CodeGen/StackProtector.cpp
llvm/lib/IR/Attributes.cpp
llvm/lib/IR/Verifier.cpp
llvm/lib/Transforms/IPO/ForceFunctionAttrs.cpp
llvm/lib/Transforms/Utils/CodeExtractor.cpp
llvm/lib/Transforms/Utils/InlineFunction.cpp
llvm/test/CodeGen/X86/stack-protector-2.ll
llvm/test/Transforms/CodeExtractor/PartialInlineAttributes.ll
llvm/test/Transforms/Inline/inline_ssp.ll
llvm/utils/emacs/llvm-mode.el
llvm/utils/kate/llvm.xml
llvm/utils/llvm.grm
llvm/utils/vim/syntax/llvm.vim
llvm/utils/vscode/llvm/syntaxes/ll.tmLanguage.yaml

Removed: 
clang/test/Frontend/optimization-remark-missed-inline-stack-protectors.c
llvm/test/Transforms/Inline/inline_nossp.ll
llvm/test/Verifier/function-attribute-nossp-ssp-sspreq-sspstrong.ll



diff  --git a/clang/include/clang/Basic/AttrDocs.td 
b/clang/include/clang/Basic/AttrDocs.td
index 085d2bac1dea..8e04ea84b240 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -3938,10 +3938,6 @@ option.
 
 int bar(int y); // bar can be built with the stack protector.
 
-A callee that has a stack protector will not be inlined into a
-``__attribute__((no_stack_protector))`` caller, and vice-versa, even if the
-callee is marked ``__attribute__((always_inline))``.
-
 }];
 }
 

diff  --git a/clang/lib/CodeGen/CodeGenModule.cpp 
b/clang/lib/CodeGen/CodeGenModule.cpp
index f50aaf3e34f9..1f81faaa2c6f 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -1642,14 +1642,14 @@ void 
CodeGenModule::SetLLVMFunctionAttributesForDefinition(const Decl *D,
   if (!hasUnwindExceptions(LangOpts))
 B.addAttribute(llvm::Attribute::NoUnwind);
 
-  if (D && D->hasAttr())
-B.addAttribute(llvm::Attribute::NoStackProtect);
-  else if (LangOpts.getStackProtector() == LangOptions::SSPOn)
-B.addAttribute(llvm::Attribute::StackProtect);
-  else if (LangOpts.getStackProtector() == LangOptions::SSPStrong)
-B.addAttribute(llvm::Attribute::StackProtectStrong);
-  else if (LangOpts.getStackProtector() == LangOptions::SSPReq)
-B.addAttribute(llvm::Attribute::StackProtectReq);
+  if (!D || !D->hasAttr()) {
+if (LangOpts.getStackProtector() == LangOptions::SSPOn)
+  B.addAttribute(llvm::Attribute::StackProtect);
+else if (LangOpts.getStackProtector() == LangOptions::SSPStrong)
+  B.addAttribute(llvm::Attribute::StackProtectStrong);
+else if (LangOpts.getStackProtector() == LangOptions::SSPReq)
+  B.addAttribute(llvm::Attribute::StackProtectReq);
+  }
 
   if (!D) {
 // If we don't have a declaration to control inlining, the function isn't

diff  --git a/clang/test/CodeGen/stack-protector.c 
b/clang/test/CodeGen/stack-protector.c
index 2c637c2318d3..5037a3e0d6fe 100644
--- a/clang/test/CodeGen/stack-protector.c
+++ b/clang/test/CodeGen/stack-protector.c
@@ -36,7 +36,7 @@ void test2(const char *msg) {
 // SSPREQ: attributes #[[A]] = {{.*}} sspreq
 
 // SAFESTACK-NOSSP: attributes #[[A]] = {{.*}} safestack
-// SAFESTACK-NOSSP-NOT: attributes #[[A]] = {{.*}} ssp
+// SAFESTACK-NOSSP-NOT: ssp
 
 // SAFESTACK-SSP: attributes #[[A]] = {{.*}} safestack ssp{{ }}
 // SAFESTACK-SSPSTRONG: attributes #[[A]] = {{.*}} safestack sspstrong
@@ -47,11 +47,6 @@ void test2(const char *msg) {
 // SSPSTRONG-NOT: attributes #[[B]] = {{.*}} sspstrong
 // SSPREQ-NOT: attributes #[[B]] = {{.*}} sspreq
 
-// NOSSP: attributes #[[B]] = {{.*}} nossp
-// SSP: attributes #[[B]] = {{.*}} nossp
-// SSPSTRONG: attributes #[[B]] = {{.*}} nossp
-// SSPREQ: attributes #[[B]] = {{.*}} nossp
-
 // SAFESTACK-SSP: attributes #[[B]] = {{.*}} safestack
 // SAFESTACK-SSP-NOT: attributes #[[B]] = {{.*}} safestack ssp{{ }}
 // SAFESTACK-SSPSTRONG: attributes #[[B]] = {{.*}} safestack

diff  --git 
a/clang/test/Frontend/optimization-remark-missed-inline-stack-protectors.c 
b/clang/test/Frontend/optimization-rema

[PATCH] D90441: [X86] Add support for vex, vex2, vex3, and evex for MASM

2020-11-17 Thread LiuChen via Phabricator via cfe-commits

LiuChen3 added inline comments.



Comment at: clang/lib/AST/Stmt.cpp:795
+  SmallVector Pieces;
+  AsmStr.split(Pieces, "\n\t");
+  std::string MSAsmString;

pengfei wrote:
> Can we always assume the separator is `\n\t`?
I think so. From the code, we can see '\n\t' will be added to each end of 
statement:
```
case AOK_EndOfStatement:
  OS << "\n\t";
  break;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90441

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

[PATCH] D67112: [Sema] Introduce function reference conversion, NFC

2020-11-17 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

rsmith added a comment.

In D67112#2401110 , @aaronpuchert 
wrote:

> In D67112#2398577 , @rsmith wrote:
>
>> Looks fine as far as it goes, but it looks like we're also missing a cast in 
>> function pointer initialization via function conversion:
>> [...]
>>
>>   |-VarDecl 0x105143e8  col:8 p 'void (*)()' cinit
>>   | `-ImplicitCastExpr 0x10514498  'void (*)() noexcept' 
>> 
>>   |   `-DeclRefExpr 0x10514450  'void () noexcept' lvalue Function 
>> 0x10514240 'f' 'void () noexcept'
>
> It seems to depend on the standard, with `-std=c++17`:
>
>   |-VarDecl p 'void (*)()' cinit
>   | `-ImplicitCastExpr 'void (*)()' 
>   |   `-ImplicitCastExpr 'void (*)() noexcept' 
>   | `-DeclRefExpr 'void () noexcept' lvalue Function 'f' 'void () 
> noexcept'
>
> Perhaps because pre-C++17 `noexcept` is not part of the type, so we 
> essentially ignore it?

Oh, right, my bad. I forgot we still default to C++14 :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67112

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

[PATCH] D91669: Add ScopedTrace to verify methods in FormatTestObjC

2020-11-17 Thread Samuel Giddins via Phabricator via cfe-commits

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

Add tests from D17700 

Don’t break before nested block param when prior param is not a block


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91669

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


Index: clang/unittests/Format/FormatTestObjC.cpp
===
--- clang/unittests/Format/FormatTestObjC.cpp
+++ clang/unittests/Format/FormatTestObjC.cpp
@@ -18,6 +18,7 @@
 #define DEBUG_TYPE "format-test"
 
 using clang::tooling::ReplacementTest;
+using testing::internal::ScopedTrace;
 
 namespace clang {
 namespace format {
@@ -51,18 +52,24 @@
 return *Result;
   }
 
-  void verifyFormat(StringRef Code) {
+  void _verifyFormat(const char *File, int Line, StringRef Code) {
+ScopedTrace t(File, Line, ::testing::Message() << Code.str());
 EXPECT_EQ(Code.str(), format(Code)) << "Expected code is not stable";
 EXPECT_EQ(Code.str(), format(test::messUp(Code)));
   }
 
-  void verifyIncompleteFormat(StringRef Code) {
+  void _verifyIncompleteFormat(const char *File, int Line, StringRef Code) {
+ScopedTrace t(File, Line, ::testing::Message() << Code.str());
 EXPECT_EQ(Code.str(), format(test::messUp(Code), SC_ExpectIncomplete));
   }
 
   FormatStyle Style;
 };
 
+#define verifyIncompleteFormat(...)
\
+  _verifyIncompleteFormat(__FILE__, __LINE__, __VA_ARGS__)
+#define verifyFormat(...) _verifyFormat(__FILE__, __LINE__, __VA_ARGS__)
+
 TEST(FormatTestObjCStyle, DetectsObjCInHeaders) {
   auto Style = getStyle("LLVM", "a.h", "none",
 "@interface\n"
@@ -1453,6 +1460,39 @@
   "   callback:^(typeof(self) self, NSNumber *u, NSNumber *v) {\n"
   " u = v;\n"
   "   }]");
+
+  verifyFormat("[self block:^(void) {\n"
+   "  doStuff();\n"
+   "} completionHandler:^(void) {\n"
+   "  doStuff();\n"
+   "  [self block:^(void) {\n"
+   "doStuff();\n"
+   "  } completionHandler:^(void) {\n"
+   "doStuff();\n"
+   "  }];\n"
+   "}];");
+
+  Style.ColumnLimit = 0;
+  verifyFormat("[[SessionService sharedService] "
+   "loadWindowWithCompletionBlock:^(SessionWindow *window) {\n"
+   "  if (window) {\n"
+   "[self windowDidLoad:window];\n"
+   "  } else {\n"
+   "[self errorLoadingWindow];\n"
+   "  }\n"
+   "}];");
+  verifyFormat("[controller test:^{\n"
+   "  doStuff();\n"
+   "} withTimeout:5 completionHandler:^{\n"
+   "  doStuff();\n"
+   "}];");
+  verifyFormat(
+  "[self setupTextFieldSignals:@[\n"
+  "  self.documentWidthField,\n"
+  "  self.documentHeightField,\n"
+  "] solver:^(NSTextField *textField) {\n"
+  "  return [self.representedObject 
solveEquationForTextField:textField];\n"
+  "}];");
 }
 
 TEST_F(FormatTestObjC, IfNotUnlikely) {
Index: clang/lib/Format/ContinuationIndenter.cpp
===
--- clang/lib/Format/ContinuationIndenter.cpp
+++ clang/lib/Format/ContinuationIndenter.cpp
@@ -400,7 +400,9 @@
 return true;
   if (Current.is(TT_SelectorName) && !Previous.is(tok::at) &&
   State.Stack.back().ObjCSelectorNameFound &&
-  State.Stack.back().BreakBeforeParameter)
+  State.Stack.back().BreakBeforeParameter &&
+  (Style.ObjCBreakBeforeNestedBlockParam ||
+   !Current.startsSequence(TT_SelectorName, tok::colon, tok::caret)))
 return true;
 
   unsigned NewLineColumn = getNewLineColumn(State);


Index: clang/unittests/Format/FormatTestObjC.cpp
===
--- clang/unittests/Format/FormatTestObjC.cpp
+++ clang/unittests/Format/FormatTestObjC.cpp
@@ -18,6 +18,7 @@
 #define DEBUG_TYPE "format-test"
 
 using clang::tooling::ReplacementTest;
+using testing::internal::ScopedTrace;
 
 namespace clang {
 namespace format {
@@ -51,18 +52,24 @@
 return *Result;
   }
 
-  void verifyFormat(StringRef Code) {
+  void _verifyFormat(const char *File, int Line, StringRef Code) {
+ScopedTrace t(File, Line, ::testing::Message() << Code.str());
 EXPECT_EQ(Code.str(), format(Code)) << "Expected code is not stable";
 EXPECT_EQ(Code.str(), format(test::messUp(Code)));
   }
 
-  void verifyIncompleteFormat(StringRef Code) {
+  void _verifyIncompleteFormat(const char *File, int Line, StringRef Code) {
+ScopedTrace t(File, Line, ::testing::Message() << Code.str());
 EXPECT_EQ(Code.str(), format(test::messUp(Code), SC_ExpectIncomplete));
   }
 
   FormatStyle Style;
 };
 
+#define

[PATCH] D91659: Allow anonymous enum typedefs to be referenced with the 'enum' specifier under MSVC compat mode

2020-11-17 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:15545-15546
+// is only used for C code.
+if (getLangOpts().MSVCCompat && (Kind == TTK_Enum) && Previous.empty() &&
+(TUK == TUK_Reference)) {
+  LookupResult TypedefEnumLookup(*this, Name, NameLoc, LookupOrdinaryName,

Our coding style does not parenthesize `==` within `&&`.



Comment at: clang/lib/Sema/SemaDecl.cpp:15551-15567
+  if (!TypedefEnumLookup.empty()) {
+if (TypedefNameDecl *TD =
+dyn_cast(TypedefEnumLookup.getFoundDecl())) {
+  const Type *UnderlyingTypePtr =
+  TD->getUnderlyingType().getTypePtrOrNull();
+
+  if (UnderlyingTypePtr) {

This can be simplified substantially.



Comment at: clang/lib/Sema/SemaDecl.cpp:15752-15754
+  bool AnonymousEnumEligible = getLangOpts().MSVCCompat &&
+   (Kind == TTK_Enum) &&
+   Tag->getDeclName().isEmpty();

In the anonymous union case, we also need to check for qualifiers on the 
typedef type, and more broadly perhaps we should be using 
`getAnonDeclWithTypedefName` here too. Presumably the new case should also only 
apply for `TUK_Reference`.



Comment at: clang/lib/Sema/SemaDecl.cpp:15753
+  bool AnonymousEnumEligible = getLangOpts().MSVCCompat &&
+   (Kind == TTK_Enum) &&
+   Tag->getDeclName().isEmpty();





Comment at: clang/test/Sema/enum-typedef-msvc.c:15
+  (void)foo;
+}

Please also add a testcase for the situation where the use of `enum Foo` occurs 
in the same scope as the typedef and anonymous `enum` declaration. (MSVC has 
weird behavior here that we don't necessarily need to follow, but we should 
make sure we at least do something defensible.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91659

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

[PATCH] D87956: [IR] add fn attr for no_stack_protector; prevent inlining on mismatch

2020-11-17 Thread Nick Desaulniers via Phabricator via cfe-commits

nickdesaulniers added inline comments.



Comment at: llvm/test/Transforms/Inline/inline_nossp.ll:3
+; RUN: opt -passes='cgscc(inline)' %s -S | FileCheck %s
+; RUN: opt -always-inline -o - -S %s | FileCheck %s
+

aeubanks wrote:
> nickdesaulniers wrote:
> > aeubanks wrote:
> > > This test fails with the NPM,
> > > `opt -passes=always-inline ...`
> > > 
> > > Does `llvm::isInlineViable()` need to be updated?
> > Thanks for the report.  Is there a Cmake configuration I need to explicitly 
> > set to reproduce? `LLVM_USE_NEWPM`?
> Just a new RUN line: `RUN: opt -passes=always-inline -o - -S %s | FileCheck 
> %s`
I think I'm going to back out this patch and the fix ups, and pursue a much 
simpler approach: 
https://github.com/ClangBuiltLinux/llvm-project/commit/501847c5239be52bbe32a9c5bd0723e4d0ad2990.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87956

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

[PATCH] D88138: [NPM] Add target specific hook to add passes for New Pass Manager

2020-11-17 Thread Ankit via Phabricator via cfe-commits

quic_aankit added a comment.

In D88138#2397785 , @vtjnash wrote:

> I think this, and similar recent commits, are causing the shared library 
> builds to fail some tests if this code gets linked into libLLVM.so: 
> https://bugs.llvm.org/show_bug.cgi?id=48181. I assume it might actually a bug 
> in ld (GNU Binutils for Ubuntu 2.34), as I don't understand the linker 
> behavior there?

I'm trying to replicate the issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88138

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

[PATCH] D91605: [sanitizers] Implement GetTls on Solaris

2020-11-17 Thread Vitaly Buka via Phabricator via cfe-commits

vitalybuka added inline comments.



Comment at: compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp:455-520
+#if SANITIZER_SOLARIS
+// dlpi_tls_modid is only available since Solaris 11.4 SRU 10.  Use
+// dlinfo(RTLD_DI_LINKMAP) instead which works on both Solaris 11.3 and 
Illumos.
+
+// Beginning of declaration from OpenSolaris/Illumos
+// $SRC/cmd/sgs/include/rtld.h.
+typedef struct {

can this go into sanitizer_platform_limits_solaris.h ?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91605

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

[PATCH] D17993: [CodeGen] Apply 'nonnull' and 'dereferenceable(N)' to 'this' pointer arguments.

2020-11-17 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

rsmith added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:2169
+if (!CodeGenOpts.NullPointerIsValid &&
+getContext().getTargetAddressSpace(FI.arg_begin()->type) == 0) {
+  Attrs.addAttribute(llvm::Attribute::NonNull);

jdoerfert wrote:
> rjmccall wrote:
> > rsmith wrote:
> > > jdoerfert wrote:
> > > > arichardson wrote:
> > > > > Isn't the `this` pointer also nonnull in other address spaces?
> > > > > 
> > > > > In our CHERI fork we use AS200 for the this pointer and would quite 
> > > > > like to have the nonnull attribute.
> > > > > I can obviously change this line locally when I next merge from 
> > > > > upstream, but I would like to avoid diffs and it seems to me like 
> > > > > this restriction is unnecessary.
> > > > I also think `NullPointerIsValid` is sufficient. 
> > > It's my understanding that:
> > > * The LLVM `null` value in any address space is the all-zero-bits value.
> > > * In address space zero, the `null` value does not correspond to 
> > > addressable memory, but this is not assumed to hold in other address 
> > > spaces.
> > > * An address-space-zero `null` value that is addressspacecast to a 
> > > different address space might not be the `null` in the target address 
> > > space.
> > > * The `nonnull` attribute implies that the pointer value is not the 
> > > `null` value.
> > > * A null pointer in the frontend in a non-zero address space corresponds 
> > > to the value produced by an addressspacecast of an address-space-zero 
> > > `null` value to the target address space.
> > > 
> > > That being the case, there is simply no connection between the C and C++ 
> > > notion of a null pointer and a `null` LLVM pointer value in a non-zero 
> > > address space in general, so it is not correct to use the `nonnull` 
> > > attribute in a non-zero address space in general. Only if we know that a 
> > > C++ null pointer is actually represented by the LLVM `null` value in the 
> > > corresponding address space can we use the `nonnull` attribute to expose 
> > > that fact to LLVM. And we do not know that in general.
> > I think all of this is correct except that the frontend does know what the 
> > bit-pattern of the null pointer is in any particular language-level address 
> > space, and it knows what the language-level address space of `this` is.  So 
> > we should be able to ask whether the null value in the `this` address space 
> > is the all-zero value and use that to decide whether to apply `nonnull`.
> Hm, I think the problem is that we don't couple `NullPointerIsValid` with the 
> address space. As I said before. In LLVM-IR, if we don't have the 
> `null-pointer-is-valid` property, then "memory access" implies 
> `dereferenceable` implies `nonnull`. Now, we usually assume non-0 address 
> spaces to have the above property, but that should be decoupled. The query if 
> "null is valid" should take the function and the AS, as it does conceptually 
> in LLVM-core, and then decide if `null-pointer-is-valid` or not. If we had 
> that, @arichardson could define that AS200 does not have valid null pointers. 
> If you do that right now the IR passes will at least deduce `nonnull` based 
> on `derferenceable`.
> I think all of this is correct except that the frontend does know what the 
> bit-pattern of the null pointer is in any particular language-level address 
> space

Interesting. How do we get at that? Do we ask the middle-end to constant-fold 
`addrspacecast i8* null to i8* addrspace(N)` and see if we get a `null` back, 
or is there a better way?

In any case, this patch follows the same pattern we use for return values of 
reference type, parameters of reference type, and decayed array function 
parameters with static array bounds, all of which apply `nonnull` only in 
address space 0. If we want to use a different pattern, to consider whether 
LLVM's `nonnull` means "not a null pointer" rather than assuming that holds 
only in address space 0, we should make a holistic change for that throughout 
CGCall.cpp, rather than touching only the handling of the `this` pointer.

It'd also be interesting to consider what we want `__attribute__((nonnull))` to 
mean in address spaces where the null pointer isn't the zero pointer: should it 
mean non-zero at the source level / non-null in LLVM IR, or should it mean 
non-null at the source level (which might be unrepresentable in LLVM IR, but 
static analysis etc. could still detect it)? We're currently inconsistent on 
this: static analysis, constant evaluation, and sanitizers treat the attribute 
as meaning non-null, but IR generation emits the `nonnull` IR attribute, 
treating it as meaning non-zero instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D17993

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

[PATCH] D91664: Add a less ambiguous macro for Android version.

2020-11-17 Thread Jiyong Park via Phabricator via cfe-commits

jiyong added a comment.

This LGTM, but just FYI: for darwin targets, these are 
__ENVIRONMENT__VERSION_MIN_REQUIRED__


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91664

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

[PATCH] D91592: [ASTMatchers] Fix typo for hasAnyOverloadedOperatorName

2020-11-17 Thread Keishi Hattori via Phabricator via cfe-commits

keishi added a comment.

In D91592#2399535 , @aaron.ballman 
wrote:

> LGTM! Do you need me to commit on your behalf? If so, is `Keishi Hattori 
> ` the correct attribution you'd like me to use?

Yes. Please use `Keishi Hattori `. Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91592

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

[clang] f8f6d64 - Revert "Revert "[analyzer] NFC: Move IssueHash to libAnalysis.""

2020-11-17 Thread Artem Dergachev via cfe-commits


Author: Artem Dergachev
Date: 2020-11-17T16:01:49-08:00
New Revision: f8f6d6455f963d5924990824866c4b55ae694639

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

LOG: Revert "Revert "[analyzer] NFC: Move IssueHash to libAnalysis.""

This reverts commit 662ed9e67adace3d011f42478bc8bcb1773a2821.

Added: 
clang/include/clang/Analysis/IssueHash.h
clang/lib/Analysis/IssueHash.cpp

Modified: 
clang/lib/Analysis/CMakeLists.txt
clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
clang/lib/StaticAnalyzer/Core/CMakeLists.txt
clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp
clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp

Removed: 
clang/include/clang/StaticAnalyzer/Core/IssueHash.h
clang/lib/StaticAnalyzer/Core/IssueHash.cpp



diff  --git a/clang/include/clang/Analysis/IssueHash.h 
b/clang/include/clang/Analysis/IssueHash.h
new file mode 100644
index ..9c02b79f58f9
--- /dev/null
+++ b/clang/include/clang/Analysis/IssueHash.h
@@ -0,0 +1,49 @@
+//===-- IssueHash.h - Generate identification hashes *- C++ 
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+#ifndef LLVM_CLANG_STATICANALYZER_CORE_ISSUE_HASH_H
+#define LLVM_CLANG_STATICANALYZER_CORE_ISSUE_HASH_H
+
+#include "llvm/ADT/SmallString.h"
+
+namespace clang {
+class Decl;
+class FullSourceLoc;
+class LangOptions;
+
+/// Returns an opaque identifier for a diagnostic.
+///
+/// This opaque identifier is intended to be stable even when the source code
+/// is changed. It allows to track diagnostics in the long term, for example,
+/// find which diagnostics are "new", maintain a database of suppressed
+/// diagnostics etc.
+///
+/// We may introduce more variants of issue hashes in the future
+/// but older variants will still be available for compatibility.
+///
+/// This hash is based on the following information:
+///   - Name of the checker that emitted the diagnostic.
+///   - Warning message.
+///   - Name of the enclosing declaration.
+///   - Contents of the line of code with the issue, excluding whitespace.
+///   - Column number (but not the line number! - which makes it stable).
+llvm::SmallString<32> getIssueHash(const FullSourceLoc &IssueLoc,
+   llvm::StringRef CheckerName,
+   llvm::StringRef WarningMessage,
+   const Decl *IssueDecl,
+   const LangOptions &LangOpts);
+
+/// Get the unhashed string representation of the V1 issue hash.
+/// When hashed, it becomes the actual issue hash. Useful for testing.
+/// See GetIssueHashV1() for more information.
+std::string getIssueString(const FullSourceLoc &IssueLoc,
+   llvm::StringRef CheckerName,
+   llvm::StringRef WarningMessage,
+   const Decl *IssueDecl, const LangOptions &LangOpts);
+} // namespace clang
+
+#endif

diff  --git a/clang/include/clang/StaticAnalyzer/Core/IssueHash.h 
b/clang/include/clang/StaticAnalyzer/Core/IssueHash.h
deleted file mode 100644
index 38d5f847fc29..
--- a/clang/include/clang/StaticAnalyzer/Core/IssueHash.h
+++ /dev/null
@@ -1,50 +0,0 @@
-//===-- IssueHash.h - Generate identification hashes *- C++ 
-*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===--===//
-#ifndef LLVM_CLANG_STATICANALYZER_CORE_ISSUE_HASH_H
-#define LLVM_CLANG_STATICANALYZER_CORE_ISSUE_HASH_H
-
-#include "llvm/ADT/SmallString.h"
-
-namespace clang {
-class Decl;
-class SourceManager;
-class FullSourceLoc;
-class LangOptions;
-
-/// Get an MD5 hash to help identify bugs.
-///
-/// This function returns a hash that helps identify bugs within a source file.
-/// This identification can be utilized to 
diff  diagnostic results on 
diff erent
-/// snapshots of a projects, or maintain a database of suppressed diagnotics.
-///
-/// The hash contains the normalized text of the location associated with the
-/// diagnostic. Normalization means removing the whitespaces. The associated
-/// location is the either the last location of a diagnostic path or a 
uniqueing
-/// location. The bugtype and the name of the checker is also part of the hash.
-/// The last component is the string representation of the enclosing 
decl

[PATCH] D91311: Add new 'preferred_name' attribute.

2020-11-17 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

rsmith added a comment.

In D91311#2400998 , @dblaikie wrote:

> In D91311#2400926 , @ldionne wrote:
>
>> For instance, I can easily imagine a library that provides an API where some 
>> types shouldn't be named (for example expression templates). In that case, 
>> you might want to describe a type by a string along the lines of 
>> `decltype(some-expression)`, which could potentially be a lot more useful 
>> than the ability to refer to a typedef. Does this sort of usage ring true to 
>> someone else?
>
> A concrete/real-world example might be helpful, if you happen to have one on 
> hand.

I can imagine something like this being useful. But I think we would want a 
substantially more powerful mechanism to produce those strings. For example, 
suppose we have an expression template library for matrices:

  template detail::mul operator*(a, b);
  template detail::add operator+(a, b);

... then given `Matrix3f a, b`, we might want the type of `a * b + a` to be 
printed as something like `decltype(Matrix3f() * Matrix3f() + Matrix3f())` 
rather than as `detail::add, Matrix3f>` (and in 
practice the types of expression template intermediaries tend to be a lot more 
complex than this). If we had a way to ask for a type to be pretty-printed as a 
string, we could imagine:

  template
  struct [[clang::preferred_name(mul, "decltype(" + 
sample_expr_of_type>() + ")")]] mul { ... };

... for some suitable function `sample_expr_of_type`. I don't think we're there 
yet -- this would require substantially more compile-time power than we have 
(formatting type strings, and string concatenations resulting in something that 
we can then feed back into the attribute). So I don't think a proposal that 
lets us only include string literals as the second attribute argument really 
gets us where we'd want to be. (This would also require a new kind of delayed 
attribute parsing: we would need to delay parsing the attribute argument until 
after `mul` is declared and in scope -- or we would need to require the 
template to be declared twice as we currently do.)

While we might not be ready for this right now, going from 
`[[preferred_name(type)]]` to `[[preferred_name(type, spelling)]]` would be a 
compatible extension if we wanted to do that in the future.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91311

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

[PATCH] D17993: [CodeGen] Apply 'nonnull' and 'dereferenceable(N)' to 'this' pointer arguments.

2020-11-17 Thread Johannes Doerfert via Phabricator via cfe-commits

jdoerfert added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:2169
+if (!CodeGenOpts.NullPointerIsValid &&
+getContext().getTargetAddressSpace(FI.arg_begin()->type) == 0) {
+  Attrs.addAttribute(llvm::Attribute::NonNull);

rjmccall wrote:
> rsmith wrote:
> > jdoerfert wrote:
> > > arichardson wrote:
> > > > Isn't the `this` pointer also nonnull in other address spaces?
> > > > 
> > > > In our CHERI fork we use AS200 for the this pointer and would quite 
> > > > like to have the nonnull attribute.
> > > > I can obviously change this line locally when I next merge from 
> > > > upstream, but I would like to avoid diffs and it seems to me like this 
> > > > restriction is unnecessary.
> > > I also think `NullPointerIsValid` is sufficient. 
> > It's my understanding that:
> > * The LLVM `null` value in any address space is the all-zero-bits value.
> > * In address space zero, the `null` value does not correspond to 
> > addressable memory, but this is not assumed to hold in other address spaces.
> > * An address-space-zero `null` value that is addressspacecast to a 
> > different address space might not be the `null` in the target address space.
> > * The `nonnull` attribute implies that the pointer value is not the `null` 
> > value.
> > * A null pointer in the frontend in a non-zero address space corresponds to 
> > the value produced by an addressspacecast of an address-space-zero `null` 
> > value to the target address space.
> > 
> > That being the case, there is simply no connection between the C and C++ 
> > notion of a null pointer and a `null` LLVM pointer value in a non-zero 
> > address space in general, so it is not correct to use the `nonnull` 
> > attribute in a non-zero address space in general. Only if we know that a 
> > C++ null pointer is actually represented by the LLVM `null` value in the 
> > corresponding address space can we use the `nonnull` attribute to expose 
> > that fact to LLVM. And we do not know that in general.
> I think all of this is correct except that the frontend does know what the 
> bit-pattern of the null pointer is in any particular language-level address 
> space, and it knows what the language-level address space of `this` is.  So 
> we should be able to ask whether the null value in the `this` address space 
> is the all-zero value and use that to decide whether to apply `nonnull`.
Hm, I think the problem is that we don't couple `NullPointerIsValid` with the 
address space. As I said before. In LLVM-IR, if we don't have the 
`null-pointer-is-valid` property, then "memory access" implies 
`dereferenceable` implies `nonnull`. Now, we usually assume non-0 address 
spaces to have the above property, but that should be decoupled. The query if 
"null is valid" should take the function and the AS, as it does conceptually in 
LLVM-core, and then decide if `null-pointer-is-valid` or not. If we had that, 
@arichardson could define that AS200 does not have valid null pointers. If you 
do that right now the IR passes will at least deduce `nonnull` based on 
`derferenceable`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D17993

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

[PATCH] D17993: [CodeGen] Apply 'nonnull' and 'dereferenceable(N)' to 'this' pointer arguments.

2020-11-17 Thread John McCall via Phabricator via cfe-commits

rjmccall added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:2169
+if (!CodeGenOpts.NullPointerIsValid &&
+getContext().getTargetAddressSpace(FI.arg_begin()->type) == 0) {
+  Attrs.addAttribute(llvm::Attribute::NonNull);

rsmith wrote:
> jdoerfert wrote:
> > arichardson wrote:
> > > Isn't the `this` pointer also nonnull in other address spaces?
> > > 
> > > In our CHERI fork we use AS200 for the this pointer and would quite like 
> > > to have the nonnull attribute.
> > > I can obviously change this line locally when I next merge from upstream, 
> > > but I would like to avoid diffs and it seems to me like this restriction 
> > > is unnecessary.
> > I also think `NullPointerIsValid` is sufficient. 
> It's my understanding that:
> * The LLVM `null` value in any address space is the all-zero-bits value.
> * In address space zero, the `null` value does not correspond to addressable 
> memory, but this is not assumed to hold in other address spaces.
> * An address-space-zero `null` value that is addressspacecast to a different 
> address space might not be the `null` in the target address space.
> * The `nonnull` attribute implies that the pointer value is not the `null` 
> value.
> * A null pointer in the frontend in a non-zero address space corresponds to 
> the value produced by an addressspacecast of an address-space-zero `null` 
> value to the target address space.
> 
> That being the case, there is simply no connection between the C and C++ 
> notion of a null pointer and a `null` LLVM pointer value in a non-zero 
> address space in general, so it is not correct to use the `nonnull` attribute 
> in a non-zero address space in general. Only if we know that a C++ null 
> pointer is actually represented by the LLVM `null` value in the corresponding 
> address space can we use the `nonnull` attribute to expose that fact to LLVM. 
> And we do not know that in general.
I think all of this is correct except that the frontend does know what the 
bit-pattern of the null pointer is in any particular language-level address 
space, and it knows what the language-level address space of `this` is.  So we 
should be able to ask whether the null value in the `this` address space is the 
all-zero value and use that to decide whether to apply `nonnull`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D17993

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

[PATCH] D91580: [Frontend] Add flag to allow PCM generation despite compiler errors

2020-11-17 Thread Ben Barham via Phabricator via cfe-commits

bnbarham updated this revision to Diff 305914.
bnbarham added a comment.

Have allow-pcm also set allow-pch + test to make sure that works.


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

https://reviews.llvm.org/D91580

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/ASTUnit.h
  clang/include/clang/Frontend/FrontendActions.h
  clang/include/clang/Frontend/FrontendOptions.h
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/test/Modules/Inputs/error.h
  clang/test/Modules/Inputs/module.map
  clang/test/Modules/load-module-with-errors.m
  clang/tools/c-index-test/core_main.cpp
  clang/tools/libclang/CIndex.cpp

Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -3476,7 +3476,7 @@
   ast_filename, CXXIdx->getPCHContainerOperations()->getRawReader(),
   ASTUnit::LoadEverything, Diags, FileSystemOpts, /*UseDebugInfo=*/false,
   CXXIdx->getOnlyLocalDecls(), None, CaptureDiagsKind::All,
-  /*AllowPCHWithCompilerErrors=*/true,
+  /*AllowASTWithCompilerErrors=*/true,
   /*UserFilesAreVolatile=*/true);
   *out_TU = MakeCXTranslationUnit(CXXIdx, std::move(AU));
   return *out_TU ? CXError_Success : CXError_Failure;
Index: clang/tools/c-index-test/core_main.cpp
===
--- clang/tools/c-index-test/core_main.cpp
+++ clang/tools/c-index-test/core_main.cpp
@@ -263,7 +263,7 @@
   std::string(modulePath), *pchRdr, ASTUnit::LoadASTOnly, Diags,
   FileSystemOpts, /*UseDebugInfo=*/false,
   /*OnlyLocalDecls=*/true, None, CaptureDiagsKind::None,
-  /*AllowPCHWithCompilerErrors=*/true,
+  /*AllowASTWithCompilerErrors=*/true,
   /*UserFilesAreVolatile=*/false);
   if (!AU) {
 errs() << "failed to create TU for: " << modulePath << '\n';
Index: clang/test/Modules/load-module-with-errors.m
===
--- /dev/null
+++ clang/test/Modules/load-module-with-errors.m
@@ -0,0 +1,25 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+
+// Write out a module with errors make sure it can be read
+// RUN: %clang_cc1 -fmodules -fallow-pcm-with-compiler-errors \
+// RUN:   -fmodules-cache-path=%t -x objective-c -emit-module \
+// RUN:   -fmodule-name=error %S/Inputs/module.map
+// RUN: %clang_cc1 -fmodules -fallow-pcm-with-compiler-errors \
+// RUN:   -fmodules-cache-path=%t -x objective-c -I %S/Inputs \
+// RUN:   -fimplicit-module-maps -ast-print %s | FileCheck %s
+
+// allow-pcm-with-compiler-errors should also allow errors in PCH
+// RUN: %clang_cc1 -fallow-pcm-with-compiler-errors -x c++ -emit-pch \
+// RUN:   -o %t/check.pch %S/Inputs/error.h
+
+@import error;
+
+void test(id x) {
+  [x method];
+}
+
+// CHECK: @interface Error
+// CHECK-NEXT: - (int)method;
+// CHECK-NEXT: @end
+// CHECK: void test(id x)
Index: clang/test/Modules/Inputs/module.map
===
--- clang/test/Modules/Inputs/module.map
+++ clang/test/Modules/Inputs/module.map
@@ -483,3 +483,4 @@
   header "template-nontrivial1.h"
   export *
 }
+module error { header "error.h" }
Index: clang/test/Modules/Inputs/error.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/error.h
@@ -0,0 +1,8 @@
+@import undefined
+
+@interface Error
+- (int)method;
+undefined
+@end
+
+undefined
Index: clang/lib/Frontend/FrontendActions.cpp
===
--- clang/lib/Frontend/FrontendActions.cpp
+++ clang/lib/Frontend/FrontendActions.cpp
@@ -177,7 +177,8 @@
   Consumers.push_back(std::make_unique(
   CI.getPreprocessor(), CI.getModuleCache(), OutputFile, Sysroot, Buffer,
   CI.getFrontendOpts().ModuleFileExtensions,
-  /*AllowASTWithErrors=*/false,
+  /*AllowASTWithErrors=*/
+  +CI.getFrontendOpts().AllowPCMWithCompilerErrors,
   /*IncludeTimestamps=*/
   +CI.getFrontendOpts().BuildingImplicitModule,
   /*ShouldCacheASTInMemory=*/
@@ -187,6 +188,11 @@
   return std::make_unique(std::move(Consumers));
 }
 
+bool GenerateModuleAction::shouldEraseOutputFiles() {
+  return !getCompilerInstance().getFrontendOpts().AllowPCMWithCompilerErrors &&
+ ASTFrontendAction::shouldEraseOutputFiles();
+}
+
 bool GenerateModuleFromModuleMapAction::BeginSourceFileAction(
 CompilerInstance &CI) {
   if (!CI.getLangOpts().Modules) {
@@ -339,7 +345,7 @@
   CI.getPCHContainerReader(), CI.getFrontendOpts().ModuleFileExtensions,
   Sysroot.empty() ? "" : Sysroot.c_str(),
   /*DisableValidation*/ false,
-  /*AllowPCHWithCompilerErrors*/ false,
+  /*AllowASTWithCompilerErrors*/ false,
   /*AllowConfigurationMismatch*/ true,
   /*Va

[PATCH] D17993: [CodeGen] Apply 'nonnull' and 'dereferenceable(N)' to 'this' pointer arguments.

2020-11-17 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

rsmith added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:2169
+if (!CodeGenOpts.NullPointerIsValid &&
+getContext().getTargetAddressSpace(FI.arg_begin()->type) == 0) {
+  Attrs.addAttribute(llvm::Attribute::NonNull);

jdoerfert wrote:
> arichardson wrote:
> > Isn't the `this` pointer also nonnull in other address spaces?
> > 
> > In our CHERI fork we use AS200 for the this pointer and would quite like to 
> > have the nonnull attribute.
> > I can obviously change this line locally when I next merge from upstream, 
> > but I would like to avoid diffs and it seems to me like this restriction is 
> > unnecessary.
> I also think `NullPointerIsValid` is sufficient. 
It's my understanding that:
* The LLVM `null` value in any address space is the all-zero-bits value.
* In address space zero, the `null` value does not correspond to addressable 
memory, but this is not assumed to hold in other address spaces.
* An address-space-zero `null` value that is addressspacecast to a different 
address space might not be the `null` in the target address space.
* The `nonnull` attribute implies that the pointer value is not the `null` 
value.
* A null pointer in the frontend in a non-zero address space corresponds to the 
value produced by an addressspacecast of an address-space-zero `null` value to 
the target address space.

That being the case, there is simply no connection between the C and C++ notion 
of a null pointer and a `null` LLVM pointer value in a non-zero address space 
in general, so it is not correct to use the `nonnull` attribute in a non-zero 
address space in general. Only if we know that a C++ null pointer is actually 
represented by the LLVM `null` value in the corresponding address space can we 
use the `nonnull` attribute to expose that fact to LLVM. And we do not know 
that in general.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D17993

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

[PATCH] D67112: [Sema] Introduce function reference conversion, NFC

2020-11-17 Thread Aaron Puchert via Phabricator via cfe-commits

aaronpuchert added a comment.

In D67112#2398577 , @rsmith wrote:

> Looks fine as far as it goes, but it looks like we're also missing a cast in 
> function pointer initialization via function conversion:
> [...]
>
>   |-VarDecl 0x105143e8  col:8 p 'void (*)()' cinit
>   | `-ImplicitCastExpr 0x10514498  'void (*)() noexcept' 
> 
>   |   `-DeclRefExpr 0x10514450  'void () noexcept' lvalue Function 
> 0x10514240 'f' 'void () noexcept'

It seems to depend on the standard, with `-std=c++17`:

  |-VarDecl p 'void (*)()' cinit
  | `-ImplicitCastExpr 'void (*)()' 
  |   `-ImplicitCastExpr 'void (*)() noexcept' 
  | `-DeclRefExpr 'void () noexcept' lvalue Function 'f' 'void () noexcept'

Perhaps because pre-C++17 `noexcept` is not part of the type, so we essentially 
ignore it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67112

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

[PATCH] D91327: [NewPM] Redesign of PreserveCFG Checker

2020-11-17 Thread Jakub Kuderski via Phabricator via cfe-commits

kuhar added a comment.

Found some cosmetics, but I'm not familiar enough with the NPM to do a full 
review.




Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:99
   struct CFG {
 struct BBGuard final : public CallbackVH {
   BBGuard(const BasicBlock *BB) : CallbackVH(BB) {}

Consider pulling this out of CFG. I don't see many reasons for having 3 levels 
of class nesting.



Comment at: llvm/include/llvm/Passes/StandardInstrumentations.h:134
 
 public:
   static cl::opt VerifyPreservedCFG;

not necessary anymore



Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:728
+  Result run(Function &F, FunctionAnalysisManager &AM) {
+return Result(&F, true /* TrackBBLifetime */);
+  }

nit: move parameter name to the left?



Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:759
+CFG::printDiff(dbgs(), GraphBefore, GraphAfter);
+report_fatal_error(Twine("CFG unexpectedly changed by ", Pass));
+  };

I think this will print with `errs()`. Would it make sense to flush `dbgs()` 
ahead of printing with `errs()`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91327

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

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

2020-11-17 Thread Mircea Trofin via Phabricator via cfe-commits

mtrofin added a comment.

In D91567#2401021 , @dblaikie wrote:

> In D91567#2398637 , @mtrofin wrote:
>
>> In D91567#2398623 , @dblaikie wrote:
>>
>>> In D91567#2398461 , @mtrofin wrote:
>>>
 In D91567#2398440 , @dblaikie 
 wrote:

>> Performing the mandatory inlinings first simplifies the problem the full 
>> inliner needs to solve
>
> That confuses me a bit - is that suggesting that we don't run the 
> AlwaysInliner when we are running the Inliner (ie: we only run the 
> AlwaysInliner at -O0, and use the Inliner at higher optimization levels 
> and let the Inliner do always inlining too)?
> & sounds like this is suggesting that would change? That we would now 
> perform always inlining separately from inlining? Maybe that's an 
> orthogonal/separate change from one implementing the always inlining 
> using the Inliner being run in a separate mode?

 In the NPM, we didn't run the AlwaysInliner until D86988 
 . See also the discussion there. The 
 normal inliner pass was, and still is, taking care of the mandatory 
 inlinings if it finds them. Of course, if we completely upfronted those 
 (which this patch can do), then the normal inliner wouldn't need to. I'm 
 not suggesting changing that - meaning, it's straightforward for the 
 normal inliner to take care of mandatory and policy-driven inlinings. The 
 idea, though, is that if we upfront the mandatory inlinings, the shape of 
 the call graph the inliner operates over is simpler and the effects of 
 inlining probably more easy to glean by the decision making policy. There 
 are trade-offs, though - we can increase that "ease of gleaning" by 
 performing more function simplification passes between the mandatory 
 inlinings and the full inliner.
>>>
>>> OK, so if I understand correctly with the old Pass Manager there were two 
>>> separate passes (always inliner and inliner - they share some code though, 
>>> yeah?)
>>
>> AlwaysInlinerLegacyPass does, yes. The NPM variant doesn't.
>
> The NPM always inliner doesn't share any code with the NPM non-always 
> inliner? (though this ( https://reviews.llvm.org/D86988 ) is the patch that 
> added a separate always inliner to the NPM, right? And that patch doesn't 
> look like it adds a whole new pass implementation - so looks like it's 
> sharing some code with something?)

There was already an AlwaysInliner for the NPM, just wasn't used. So D86966 
 hooked that up in the NPM, basically. The 
implementation of that AlwaysInliner is separate from the Inliner pass. See 
Transforms/IPO/AlwaysInliner.cpp lines 36 - 114, vs Inliner.cpp, from 687 
onwards.

>>> and they were run in the pass pipeline but potentially (definitely?) not 
>>> adjacent?
>>
>> From what I can see, the legacy one was used only in the O0/O1 
>>  cases, see 
>> clang/lib/CodeGen/BackendUtil,cpp:643. The full inliner isn't.
>
> The full inliner isn't.. isn't run at -O0/-O1? So with the Legacy Pass 
> Manager one inliner (always or non-always) was used in a given compilation, 
> not both? (so I guess then the non-always inliner did the always-inlining in 
> -O2 and above in the old pass manager? But didn't have the same recursive 
> always inlining miss that the NPM non-always inliner had?)

Yup, see BackendUtil.cpp:634. Can't comment on the latter problem.

>> New pass manager survived for quite a while with only one inlining pass, 
>> that included a mandatorily strong preference for inlining always-inline 
>> functions? But still missed some recursive cases. So D86988 
>>  made the always inliner run right next 
>> to/before the inliner in the NPM.
>>
>>> Now there's tihs patch, to implement the AlwaysInliner using the inliner - 
>>> but is also changing the order of passes to improve optimization 
>>> opportunities by doing some cleanup after always inlining?
>>
>> It doesn't quite change the order D86988  
>> introduced. Specifically, D86988  ran 
>> AlwaysInliner (a module pass) first, then let the Inliner and function 
>> optimizations happen.
>> This patch keeps the order between doing mandatory inlinings and inlinings. 
>> But, in addition, if in the future we want to also perform some of the 
>> function passes that happen in the inliner case, to help the full inliner, 
>> we can more easily do so.
>
> I'm still a bit confused/trying to understand better - am I understanding 
> correctly when I say: D86988  added always 
> inlining (for the NPM) as a separate process within the non-always

[PATCH] D91664: Add a less ambiguous macro for Android version.

2020-11-17 Thread Dan Albert via Phabricator via cfe-commits

danalbert created this revision.
danalbert added reviewers: enh, srhines, jiyong.
Herald added a project: clang.
danalbert requested review of this revision.

Android has a handful of API levels relevant to developers described
here: https://developer.android.com/studio/build#module-level.
`__ANDROID_API__` is too vague and confuses a lot of people. Introduce
a new macro name that is explicit about which one it represents. Keep
the old name around because code has been using it for a decade.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91664

Files:
  clang/lib/Basic/Targets/OSTargets.h
  clang/test/Preprocessor/init.c


Index: clang/test/Preprocessor/init.c
===
--- clang/test/Preprocessor/init.c
+++ clang/test/Preprocessor/init.c
@@ -1580,6 +1580,7 @@
 //
 // RUN: %clang_cc1 -triple arm-linux-androideabi -E -dM < /dev/null | 
FileCheck -match-full-lines -check-prefix ANDROID %s
 // ANDROID-NOT:#define __ANDROID_API__
+// ANDROID-NOT:#define __ANDROID_MIN_SDK_VERSION__
 // ANDROID:#define __ANDROID__ 1
 // ANDROID-NOT:#define __gnu_linux__
 //
@@ -1590,7 +1591,8 @@
 // X86_64-ANDROID-CXX:#define __STDCPP_DEFAULT_NEW_ALIGNMENT__ 16UL
 //
 // RUN: %clang_cc1 -triple arm-linux-androideabi20 -E -dM < /dev/null | 
FileCheck -match-full-lines -check-prefix ANDROID20 %s
-// ANDROID20:#define __ANDROID_API__ 20
+// ANDROID20:#define __ANDROID_API__ __ANDROID_MIN_SDK_VERSION__
+// ANDROID20:#define __ANDROID_MIN_SDK_VERSION__ 20
 // ANDROID20:#define __ANDROID__ 1
 // ANDROID-NOT:#define __gnu_linux__
 //
Index: clang/lib/Basic/Targets/OSTargets.h
===
--- clang/lib/Basic/Targets/OSTargets.h
+++ clang/lib/Basic/Targets/OSTargets.h
@@ -383,8 +383,12 @@
   Triple.getEnvironmentVersion(Maj, Min, Rev);
   this->PlatformName = "android";
   this->PlatformMinVersion = VersionTuple(Maj, Min, Rev);
-  if (Maj)
-Builder.defineMacro("__ANDROID_API__", Twine(Maj));
+  if (Maj) {
+Builder.defineMacro("__ANDROID_MIN_SDK_VERSION__", Twine(Maj));
+// This historical but ambiguous name for the minSdkVersion macro. Keep
+// defined for compatibility.
+Builder.defineMacro("__ANDROID_API__", "__ANDROID_MIN_SDK_VERSION__");
+  }
 } else {
 Builder.defineMacro("__gnu_linux__");
 }


Index: clang/test/Preprocessor/init.c
===
--- clang/test/Preprocessor/init.c
+++ clang/test/Preprocessor/init.c
@@ -1580,6 +1580,7 @@
 //
 // RUN: %clang_cc1 -triple arm-linux-androideabi -E -dM < /dev/null | FileCheck -match-full-lines -check-prefix ANDROID %s
 // ANDROID-NOT:#define __ANDROID_API__
+// ANDROID-NOT:#define __ANDROID_MIN_SDK_VERSION__
 // ANDROID:#define __ANDROID__ 1
 // ANDROID-NOT:#define __gnu_linux__
 //
@@ -1590,7 +1591,8 @@
 // X86_64-ANDROID-CXX:#define __STDCPP_DEFAULT_NEW_ALIGNMENT__ 16UL
 //
 // RUN: %clang_cc1 -triple arm-linux-androideabi20 -E -dM < /dev/null | FileCheck -match-full-lines -check-prefix ANDROID20 %s
-// ANDROID20:#define __ANDROID_API__ 20
+// ANDROID20:#define __ANDROID_API__ __ANDROID_MIN_SDK_VERSION__
+// ANDROID20:#define __ANDROID_MIN_SDK_VERSION__ 20
 // ANDROID20:#define __ANDROID__ 1
 // ANDROID-NOT:#define __gnu_linux__
 //
Index: clang/lib/Basic/Targets/OSTargets.h
===
--- clang/lib/Basic/Targets/OSTargets.h
+++ clang/lib/Basic/Targets/OSTargets.h
@@ -383,8 +383,12 @@
   Triple.getEnvironmentVersion(Maj, Min, Rev);
   this->PlatformName = "android";
   this->PlatformMinVersion = VersionTuple(Maj, Min, Rev);
-  if (Maj)
-Builder.defineMacro("__ANDROID_API__", Twine(Maj));
+  if (Maj) {
+Builder.defineMacro("__ANDROID_MIN_SDK_VERSION__", Twine(Maj));
+// This historical but ambiguous name for the minSdkVersion macro. Keep
+// defined for compatibility.
+Builder.defineMacro("__ANDROID_API__", "__ANDROID_MIN_SDK_VERSION__");
+  }
 } else {
 Builder.defineMacro("__gnu_linux__");
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91590: [NVPTX] Efficently support dynamic index on CUDA kernel aggregate parameters.

2020-11-17 Thread Justin Lebar via Phabricator via cfe-commits

jlebar added a comment.

I am legit excited about this if we could figure out how to make it work, but I 
don't have anything to add beyond what tra said.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91590

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

[PATCH] D17993: [CodeGen] Apply 'nonnull' and 'dereferenceable(N)' to 'this' pointer arguments.

2020-11-17 Thread Johannes Doerfert via Phabricator via cfe-commits

jdoerfert added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:2169
+if (!CodeGenOpts.NullPointerIsValid &&
+getContext().getTargetAddressSpace(FI.arg_begin()->type) == 0) {
+  Attrs.addAttribute(llvm::Attribute::NonNull);

arichardson wrote:
> Isn't the `this` pointer also nonnull in other address spaces?
> 
> In our CHERI fork we use AS200 for the this pointer and would quite like to 
> have the nonnull attribute.
> I can obviously change this line locally when I next merge from upstream, but 
> I would like to avoid diffs and it seems to me like this restriction is 
> unnecessary.
I also think `NullPointerIsValid` is sufficient. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D17993

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

[PATCH] D87928: Provide -fsource-dir flag in Clang

2020-11-17 Thread Duncan P. N. Exon Smith via Phabricator via cfe-commits

dexonsmith added a comment.

In D87928#2396448 , @phosek wrote:

> `-fsource-dir` is only used for coverage mapping and macros.

Oh, interesting; it would be nice to consistently make all paths relative paths.

> Here `out/default` is the working directory so if we used 
> `-frelative-to-working-dir`, we would end up with paths like 
> `../../sources/lib/a/a.c` in the coverage mapping or macro expansions like 
> `__FILE__`. While this may be acceptable for macro expansions, it's 
> definitely undesirable in coverage mapping.
>
> I can think of some potential solutions. We use could use 
> `-fprofile-prefix-map=../../=` to strip the `../../` prefix.

This solution makes sense to me.

>> - What's the expected behaviour when building modules implicitly with 
>> `-fmodules`? What value (if any) should they get for `-fsource-dir`?
>
> Right now this flag doesn't apply to modules at all so there's no effect.

I'm not quite following this. I don't see logic to avoid this flag applying to 
modules, although I may be missing something. I believe modules built 
explicitly via the command-line will happily accept and apply this option, and, 
similarly, I think modules built implicitly by cloning a `CompilerInvocation` 
will pick up the same value as the importing `CompilerInstance`.

>> It seems cleaner to have everything relative to `/working/dir`, not 
>> `/working/dir/sources`, since you can name all compiler inputs and outputs 
>> with the former. But the latter is conceptually the source directory.
>
> Yes, I agree. Ultimately it's up to the user to decide what output they 
> expect. I guess this is one advantage of using the 
> `-frelative-to-working-dir` approach where it's unambiguous and user can use 
> other flags to do additional rewriting if needed.

If we want to handle it consistently everywhere, I wonder if we'd want a 
feature in the `FileManager` to do this, potentially handling many other cases. 
E.g., instead of canonicalizing to an absolute path, it would canonicalize to a 
path relative to working directory. Then callers just need to worry about the 
prefix map. I'm not sure though.

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

https://reviews.llvm.org/D87928

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

[PATCH] D17993: [CodeGen] Apply 'nonnull' and 'dereferenceable(N)' to 'this' pointer arguments.

2020-11-17 Thread Alexander Richardson via Phabricator via cfe-commits

arichardson added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:2169
+if (!CodeGenOpts.NullPointerIsValid &&
+getContext().getTargetAddressSpace(FI.arg_begin()->type) == 0) {
+  Attrs.addAttribute(llvm::Attribute::NonNull);

Isn't the `this` pointer also nonnull in other address spaces?

In our CHERI fork we use AS200 for the this pointer and would quite like to 
have the nonnull attribute.
I can obviously change this line locally when I next merge from upstream, but I 
would like to avoid diffs and it seems to me like this restriction is 
unnecessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D17993

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

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

2020-11-17 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

In D91567#2398637 , @mtrofin wrote:

> In D91567#2398623 , @dblaikie wrote:
>
>> In D91567#2398461 , @mtrofin wrote:
>>
>>> In D91567#2398440 , @dblaikie 
>>> wrote:
>>>
> Performing the mandatory inlinings first simplifies the problem the full 
> inliner needs to solve

 That confuses me a bit - is that suggesting that we don't run the 
 AlwaysInliner when we are running the Inliner (ie: we only run the 
 AlwaysInliner at -O0, and use the Inliner at higher optimization levels 
 and let the Inliner do always inlining too)?
 & sounds like this is suggesting that would change? That we would now 
 perform always inlining separately from inlining? Maybe that's an 
 orthogonal/separate change from one implementing the always inlining using 
 the Inliner being run in a separate mode?
>>>
>>> In the NPM, we didn't run the AlwaysInliner until D86988 
>>> . See also the discussion there. The 
>>> normal inliner pass was, and still is, taking care of the mandatory 
>>> inlinings if it finds them. Of course, if we completely upfronted those 
>>> (which this patch can do), then the normal inliner wouldn't need to. I'm 
>>> not suggesting changing that - meaning, it's straightforward for the normal 
>>> inliner to take care of mandatory and policy-driven inlinings. The idea, 
>>> though, is that if we upfront the mandatory inlinings, the shape of the 
>>> call graph the inliner operates over is simpler and the effects of inlining 
>>> probably more easy to glean by the decision making policy. There are 
>>> trade-offs, though - we can increase that "ease of gleaning" by performing 
>>> more function simplification passes between the mandatory inlinings and the 
>>> full inliner.
>>
>> OK, so if I understand correctly with the old Pass Manager there were two 
>> separate passes (always inliner and inliner - they share some code though, 
>> yeah?)
>
> AlwaysInlinerLegacyPass does, yes. The NPM variant doesn't.

The NPM always inliner doesn't share any code with the NPM non-always inliner? 
(though this ( https://reviews.llvm.org/D86988 ) is the patch that added a 
separate always inliner to the NPM, right? And that patch doesn't look like it 
adds a whole new pass implementation - so looks like it's sharing some code 
with something?)

>> and they were run in the pass pipeline but potentially (definitely?) not 
>> adjacent?
>
> From what I can see, the legacy one was used only in the O0/O1 
>  cases, see 
> clang/lib/CodeGen/BackendUtil,cpp:643. The full inliner isn't.

The full inliner isn't.. isn't run at -O0/-O1? So with the Legacy Pass Manager 
one inliner (always or non-always) was used in a given compilation, not both? 
(so I guess then the non-always inliner did the always-inlining in -O2 and 
above in the old pass manager? But didn't have the same recursive always 
inlining miss that the NPM non-always inliner had?)

> New pass manager survived for quite a while with only one inlining pass, that 
> included a mandatorily strong preference for inlining always-inline 
> functions? But still missed some recursive cases. So D86988 
>  made the always inliner run right next 
> to/before the inliner in the NPM.
>
>> Now there's tihs patch, to implement the AlwaysInliner using the inliner - 
>> but is also changing the order of passes to improve optimization 
>> opportunities by doing some cleanup after always inlining?
>
> It doesn't quite change the order D86988  
> introduced. Specifically, D86988  ran 
> AlwaysInliner (a module pass) first, then let the Inliner and function 
> optimizations happen.
> This patch keeps the order between doing mandatory inlinings and inlinings. 
> But, in addition, if in the future we want to also perform some of the 
> function passes that happen in the inliner case, to help the full inliner, we 
> can more easily do so.

I'm still a bit confused/trying to understand better - am I understanding 
correctly when I say: D86988  added always 
inlining (for the NPM) as a separate process within the non-always inliner? And 
this patch you're proposing. breaks always inlining out into a separate pass 
proper, so that at some point, if someone wanted to (but not being done in this 
patch) they could put some passes in between the two runs of inlining (always 
and non-always)?

(I guess one thing I might be especially confused about is the "reuse X to do 
Y" would, to me, immediately lead me to think about "so I expect to see a bunch 
of deleted code because X presumably was doing a bunch of stuff itself that it 
now doesn't have to" - but I guess that's not

[PATCH] D91651: [clang] Add a warning (à la gcc) for too small enum bitfields

2020-11-17 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:16443-16446
+  const unsigned BitsNeeded =
+  IsSignedEnum
+  ? std::max(ED->getNumPositiveBits() + 1, 
ED->getNumNegativeBits())
+  : ED->getNumPositiveBits();

Please add a member function to `EnumDecl` to compute this. SemaChecking.cpp 
does the same calculation twice, and CGExpr.cpp does it once too, so it seems 
past time to factor out this calculation.



Comment at: clang/lib/Sema/SemaDecl.cpp:16448
+  
+  if (BitsNeeded > Value.getZExtValue()) {
+// TODO: Should we be emitting diagnostics for unnamed bitfields that

> When comparing an APSInt to an unsigned value - should i prefer using the 
> overloaded operator (i.e. Value < BitsNeeded) or stick with BitsNeeded > 
> Value.getZExtValue().

Never use `get*ExtValue` unless you've already checked that the integer fits in 
64 bits (and even then, it's better to avoid it when you can). For example, due 
to the incorrect pre-existing use of `getZExtValue` in this function, we 
asserted on:

```
enum E {};
struct X { E e : (__int128)0x7fff''' * 
(__int128)0x7fff'''; };
```

I've fixed that in rG8e923ec2a803d54154aaa0079c1cfcf146b7a22f, but we should 
avoid reintroducing uses of `getZExtValue` here.



Comment at: clang/lib/Sema/SemaDecl.cpp:16454-16459
+} else {
+  Diag(FieldLoc, diag::warn_bitfield_too_small_for_enum)
+  << "" << ED;
+  //<< FieldName << ED;
+  //
+}

> Should we be emitting such a warning for unnamed bitfields?

The warning is not useful for unnamed bit-fields, as they represent only 
padding and not values. We should suppress it in that case.

(Maybe we should have a warning for unnamed bit-fields of "unusual" types, 
since that probably indicates that a name was forgotten, but there might also 
be valid reasons to do that, such as to avoid the problematic corners of the MS 
ABI bit-field layout rule. So it seems better to suppress it entirely.)



Comment at: clang/test/SemaCXX/warn-bitfield-enum-conversion.cpp:62
+
+#ifdef FV_UNNAMED_BITFIELD
+#define NAME(x)

Please remove the `FV_` prefix here -- authorship information is in the version 
control history if we need it.

It would also be better to write out the test twice (once with named and once 
with unnamed bit-fields) instead of using a macro and running the entire test 
file an additional time. Each `clang` invocation adds to our total test time 
much more substantially than a few more lines of testcase do, and 
non-macro-expanded testcases are easier to understand and maintain.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91651

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

[clang] 8e923ec - Fix assertions and bad warnings on extremely wide bit-fields.

2020-11-17 Thread Richard Smith via cfe-commits


Author: Richard Smith
Date: 2020-11-17T14:36:51-08:00
New Revision: 8e923ec2a803d54154aaa0079c1cfcf146b7a22f

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

LOG: Fix assertions and bad warnings on extremely wide bit-fields.

We used to produce a bogus warning if the width couldn't be represented
in 32 bits, and assert if it couldn't be represented in 64 bits.

Added: 


Modified: 
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Sema/SemaDecl.cpp
clang/test/SemaCXX/bitfield-layout.cpp

Removed: 




diff  --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index c47084c01367..2e479307ce22 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -5649,9 +5649,8 @@ def err_incorrect_number_of_vector_initializers : Error<
 def warn_bitfield_width_exceeds_type_width: Warning<
   "width of bit-field %0 (%1 bits) exceeds the width of its type; value will "
   "be truncated to %2 bit%s2">, InGroup;
-def warn_anon_bitfield_width_exceeds_type_width : Warning<
-  "width of anonymous bit-field (%0 bits) exceeds width of its type; value "
-  "will be truncated to %1 bit%s1">, InGroup;
+def err_bitfield_too_wide : Error<
+  "%select{bit-field %1|anonymous bit-field}0 is too wide (%2 bits)">;
 def warn_bitfield_too_small_for_enum : Warning<
   "bit-field %0 is not wide enough to store all enumerators of %1">,
   InGroup, DefaultIgnore;

diff  --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 19501f7f0d52..c78d37f70a04 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -16432,6 +16432,13 @@ ExprResult Sema::VerifyBitField(SourceLocation 
FieldLoc,
   << Value.toString(10);
   }
 
+  // The size of the bit-field must not exceed our maximum permitted object
+  // size.
+  if (Value.getActiveBits() > ConstantArrayType::getMaxSizeBits(Context)) {
+return Diag(FieldLoc, diag::err_bitfield_too_wide)
+   << !FieldName << FieldName << Value.toString(10);
+  }
+
   if (!FieldTy->isDependentType()) {
 uint64_t TypeStorageSize = Context.getTypeSize(FieldTy);
 uint64_t TypeWidth = Context.getIntWidth(FieldTy);
@@ -16449,25 +16456,21 @@ ExprResult Sema::VerifyBitField(SourceLocation 
FieldLoc,
   CStdConstraintViolation ? TypeWidth : TypeStorageSize;
   if (FieldName)
 return Diag(FieldLoc, diag::err_bitfield_width_exceeds_type_width)
-   << FieldName << (unsigned)Value.getZExtValue()
+   << FieldName << Value.toString(10)
<< !CStdConstraintViolation << DiagWidth;
 
   return Diag(FieldLoc, diag::err_anon_bitfield_width_exceeds_type_width)
- << (unsigned)Value.getZExtValue() << !CStdConstraintViolation
+ << Value.toString(10) << !CStdConstraintViolation
  << DiagWidth;
 }
 
 // Warn on types where the user might conceivably expect to get all
 // specified bits as value bits: that's all integral types other than
 // 'bool'.
-if (BitfieldIsOverwide && !FieldTy->isBooleanType()) {
-  if (FieldName)
-Diag(FieldLoc, diag::warn_bitfield_width_exceeds_type_width)
-<< FieldName << (unsigned)Value.getZExtValue()
-<< (unsigned)TypeWidth;
-  else
-Diag(FieldLoc, diag::warn_anon_bitfield_width_exceeds_type_width)
-<< (unsigned)Value.getZExtValue() << (unsigned)TypeWidth;
+if (BitfieldIsOverwide && !FieldTy->isBooleanType() && FieldName) {
+  Diag(FieldLoc, diag::warn_bitfield_width_exceeds_type_width)
+  << FieldName << Value.toString(10)
+  << (unsigned)TypeWidth;
 }
   }
 

diff  --git a/clang/test/SemaCXX/bitfield-layout.cpp 
b/clang/test/SemaCXX/bitfield-layout.cpp
index 25aa82229925..7efd1d38c682 100644
--- a/clang/test/SemaCXX/bitfield-layout.cpp
+++ b/clang/test/SemaCXX/bitfield-layout.cpp
@@ -10,6 +10,12 @@ struct Test1 {
 CHECK_SIZE(Test1, 2);
 CHECK_ALIGN(Test1, 1);
 
+struct Test1a {
+  char : 9; // no warning (there's no value to truncate here)
+};
+CHECK_SIZE(Test1a, 2);
+CHECK_ALIGN(Test1a, 1);
+
 struct Test2 {
   char c : 16; // expected-warning {{width of bit-field 'c' (16 bits) exceeds 
the width of its type; value will be truncated to 8 bits}}
 };
@@ -28,3 +34,16 @@ struct Test4 {
 CHECK_SIZE(Test4, 8);
 CHECK_ALIGN(Test4, 8);
 
+struct Test5 {
+  char c : 0x10001; // expected-warning {{width of bit-field 'c' 
(4294967297 bits) exceeds the width of its type; value will be truncated to 8 
bits}}
+};
+// Size and align don't really matter here, just make sure we don't crash.
+CHECK_SIZE(Test5, 1);
+CHECK_ALIGN(Test5, 1);
+
+struct Test6 {
+  char c : (unsigned __int128)0x

[clang] a72f11e - Fix a pair of tests that would fail on a win32 box

2020-11-17 Thread Erich Keane via cfe-commits


Author: Erich Keane
Date: 2020-11-17T14:28:52-08:00
New Revision: a72f11ee20fec2df5611c49ec5ec2ce32ab8eb4c

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

LOG: Fix a pair of tests that would fail on a win32 box

The tests don't specify a triple in some cases, since they shouldn't be
necessary, so I've updated the tests to detect via macro when they are
running on win32 to give the slightly altered diagnostic.

Added: 


Modified: 
clang/test/CXX/expr/expr.prim/expr.prim.lambda/default-arguments.cpp
clang/test/SemaOpenCLCXX/address-space-lambda.cl

Removed: 




diff  --git 
a/clang/test/CXX/expr/expr.prim/expr.prim.lambda/default-arguments.cpp 
b/clang/test/CXX/expr/expr.prim/expr.prim.lambda/default-arguments.cpp
index 5d3c63f92c28..3e28288fb82a 100644
--- a/clang/test/CXX/expr/expr.prim/expr.prim.lambda/default-arguments.cpp
+++ b/clang/test/CXX/expr/expr.prim/expr.prim.lambda/default-arguments.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify=expected,nowin32
-// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify=expected,win32 -triple 
i386-windows
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify -triple i386-windows
 
 void defargs() {
   auto l1 = [](int i, int j = 17, int k = 18) { return i + j + k; };
@@ -44,9 +44,12 @@ template void defargs_in_template_unused(NoDefaultCtor);  // 
expected-note{{in i
 template
 void defargs_in_template_used() {
   auto l1 = [](const T& value = T()) { }; // expected-error{{no matching 
constructor for initialization of 'NoDefaultCtor'}} \
-  // expected-note{{candidate function 
not viable: requires single argument 'value', but no arguments were provided}} \
-  // nowin32-note{{conversion 
candidate of type 'void (*)(const NoDefaultCtor &)'}}\
-  // win32-note{{conversion candidate 
of type 'void (*)(const NoDefaultCtor &) __attribute__((thiscall))'}}
+  // expected-note{{candidate function 
not viable: requires single argument 'value', but no arguments were provided}}
+#if defined(_WIN32) && !defined(_WIN64)
+  // expected-note@46{{conversion 
candidate of type 'void (*)(const NoDefaultCtor &) __attribute__((thiscall))'}}
+#else
+  // expected-note@46{{conversion 
candidate of type 'void (*)(const NoDefaultCtor &)'}}
+#endif
   l1(); // expected-error{{no matching function for call to object of type 
'(lambda at }}
 }
 

diff  --git a/clang/test/SemaOpenCLCXX/address-space-lambda.cl 
b/clang/test/SemaOpenCLCXX/address-space-lambda.cl
index c9e1ec3735a8..571ea9035877 100644
--- a/clang/test/SemaOpenCLCXX/address-space-lambda.cl
+++ b/clang/test/SemaOpenCLCXX/address-space-lambda.cl
@@ -1,5 +1,5 @@
-//RUN: %clang_cc1 %s -cl-std=clc++ -pedantic -ast-dump 
-verify=expected,nowin32 | FileCheck %s
-//RUN: %clang_cc1 %s -cl-std=clc++ -pedantic -ast-dump -verify=expected,win32 
-triple i386-windows | FileCheck %s
+//RUN: %clang_cc1 %s -cl-std=clc++ -pedantic -ast-dump -verify | FileCheck %s
+//RUN: %clang_cc1 %s -cl-std=clc++ -pedantic -ast-dump -verify -triple 
i386-windows | FileCheck %s
 
 //CHECK: CXXMethodDecl {{.*}} constexpr operator() 'int (__private int){{.*}} 
const __generic'
 auto glambda = [](auto a) { return a; };
@@ -32,12 +32,27 @@ __kernel void test_qual() {
 //CHECK: |-CXXMethodDecl {{.*}} constexpr operator() 'void () {{.*}}const 
__generic'
   auto priv2 = []() __generic {};
   priv2();
-  auto priv3 = []() __global {}; //expected-note{{candidate function not 
viable: 'this' object is in address space '__private', but method expects 
object in address space '__global'}} //nowin32-note{{conversion candidate of 
type 'void (*)()'}}//win32-note{{conversion candidate of type 'void (*)() 
__attribute__((thiscall))'}}
+  auto priv3 = []() __global {}; //expected-note{{candidate function not 
viable: 'this' object is in address space '__private', but method expects 
object in address space '__global'}}
+#if defined(_WIN32) && !defined(_WIN64)
+  //expected-note@35{{conversion candidate of type 'void (*)() 
__attribute__((thiscall))'}}
+#else
+  //expected-note@35{{conversion candidate of type 'void (*)()'}}
+#endif
   priv3(); //expected-error{{no matching function for call to object of type}}
 
-  __constant auto const1 = []() __private{}; //expected-note{{candidate 
function not viable: 'this' object is in address space '__constant', but method 
expects object in address space '__private'}} //nowin32-note{{conversion 
candidate of type 'void (*)()'}} //win32-note{{conversion candidate of type 
'void

[PATCH] D91311: Add new 'preferred_name' attribute.

2020-11-17 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

In D91311#2400926 , @ldionne wrote:

> In D91311#2400917 , @dblaikie wrote:
>
>> How would that work for users - they would get error messages from the 
>> compiler using type names that don't exist in the source code? I'd have 
>> thought that would be quite confusing.
>
> Yes, if a library author decides to say something like:
>
>   template
>   class [[preferred_name(basic_string_view, "ahaha I'm such a troll")]]
>   basic_string_view {
>   ...
>   };
>
> Then you might get compiler errors that are not super helpful. I don't think 
> the fact that such nonsense is doable means that we shouldn't give this 
> control to library authors.

It's certainly a cost/risk to weigh against the benefits. (both intentional 
weird uses like that, but also accidental misuses like typos).

> For instance, I can easily imagine a library that provides an API where some 
> types shouldn't be named (for example expression templates). In that case, 
> you might want to describe a type by a string along the lines of 
> `decltype(some-expression)`, which could potentially be a lot more useful 
> than the ability to refer to a typedef. Does this sort of usage ring true to 
> someone else?

A concrete/real-world example might be helpful, if you happen to have one on 
hand.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91311

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

[PATCH] D91660: [OPENMP]Fix PR48174: compile-time crash with target enter data on a global struct.

2020-11-17 Thread Johannes Doerfert via Phabricator via cfe-commits

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91660

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

[PATCH] D91660: [OPENMP]Fix PR48174: compile-time crash with target enter data on a global struct.

2020-11-17 Thread Alexey Bataev via Phabricator via cfe-commits

ABataev created this revision.
ABataev added a reviewer: jdoerfert.
Herald added subscribers: guansong, yaxunl.
Herald added a project: clang.
ABataev requested review of this revision.
Herald added a subscriber: sstefan1.

The compiler should treat array subscript with base pointer as a first
pointer in complex data, it is used only for member expression with base
pointer.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91660

Files:
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/test/OpenMP/target_data_map_pointer_array_subscript_codegen.cpp


Index: clang/test/OpenMP/target_data_map_pointer_array_subscript_codegen.cpp
===
--- /dev/null
+++ clang/test/OpenMP/target_data_map_pointer_array_subscript_codegen.cpp
@@ -0,0 +1,48 @@
+// Test host codegen.
+// RUN: %clang_cc1 -verify -fopenmp -fopenmp-version=45 -x c++ -triple 
powerpc64le-unknown-unknown -fopenmp-targets=powerpc64le-ibm-linux-gnu 
-emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -fopenmp-version=45 -x c++ -std=c++11 -triple 
powerpc64le-unknown-unknown -fopenmp-targets=powerpc64le-ibm-linux-gnu 
-emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -fopenmp-version=45 -x c++ -triple 
powerpc64le-unknown-unknown -fopenmp-targets=powerpc64le-ibm-linux-gnu 
-std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -verify -fopenmp -fopenmp-version=45 -x c++ -triple 
i386-unknown-unknown -fopenmp-targets=i386-pc-linux-gnu -emit-llvm %s -o - | 
FileCheck %s
+// RUN: %clang_cc1 -fopenmp -fopenmp-version=45 -x c++ -std=c++11 -triple 
i386-unknown-unknown -fopenmp-targets=i386-pc-linux-gnu -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -fopenmp-version=45 -x c++ -triple 
i386-unknown-unknown -fopenmp-targets=i386-pc-linux-gnu -std=c++11 -include-pch 
%t -verify %s -emit-llvm -o - | FileCheck %s
+
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple powerpc64le-unknown-unknown 
-fopenmp-targets=powerpc64le-ibm-linux-gnu -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple 
powerpc64le-unknown-unknown -fopenmp-targets=powerpc64le-ibm-linux-gnu 
-emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple powerpc64le-unknown-unknown 
-fopenmp-targets=powerpc64le-ibm-linux-gnu -std=c++11 -include-pch %t -verify 
%s -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -verify -fopenmp -x c++ -triple i386-unknown-unknown 
-fopenmp-targets=i386-pc-linux-gnu -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -fopenmp -x c++ -std=c++11 -triple i386-unknown-unknown 
-fopenmp-targets=i386-pc-linux-gnu -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp -x c++ -triple i386-unknown-unknown 
-fopenmp-targets=i386-pc-linux-gnu -std=c++11 -include-pch %t -verify %s 
-emit-llvm -o - | FileCheck %s
+
+// RUN: %clang_cc1 -verify -fopenmp-simd -x c++ -triple 
powerpc64le-unknown-unknown -fopenmp-targets=powerpc64le-ibm-linux-gnu 
-emit-llvm %s -o - | FileCheck --check-prefix SIMD-ONLY0 %s
+// RUN: %clang_cc1 -fopenmp-simd -x c++ -std=c++11 -triple 
powerpc64le-unknown-unknown -fopenmp-targets=powerpc64le-ibm-linux-gnu 
-emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp-simd -x c++ -triple powerpc64le-unknown-unknown 
-fopenmp-targets=powerpc64le-ibm-linux-gnu -std=c++11 -include-pch %t -verify 
%s -emit-llvm -o - | FileCheck --check-prefix SIMD-ONLY0 %s
+// RUN: %clang_cc1 -verify -fopenmp-simd -x c++ -triple i386-unknown-unknown 
-fopenmp-targets=i386-pc-linux-gnu -emit-llvm %s -o - | FileCheck 
--check-prefix SIMD-ONLY0 %s
+// RUN: %clang_cc1 -fopenmp-simd -x c++ -std=c++11 -triple 
i386-unknown-unknown -fopenmp-targets=i386-pc-linux-gnu -emit-pch -o %t %s
+// RUN: %clang_cc1 -fopenmp-simd -x c++ -triple i386-unknown-unknown 
-fopenmp-targets=i386-pc-linux-gnu -std=c++11 -include-pch %t -verify %s 
-emit-llvm -o - | FileCheck --check-prefix SIMD-ONLY0 %s
+// SIMD-ONLY0-NOT: {{__kmpc|__tgt}}
+
+// expected-no-diagnostics
+#ifndef HEADER
+#define HEADER
+
+#pragma omp declare target
+typedef struct {
+  int *arr;
+} MyObject;
+
+MyObject *objects;
+#pragma omp end declare target
+
+// CHECK-DAG: [[SIZES0:@.+]] = private unnamed_addr constant [1 x i64] [i64 
{{8|4}}]
+// CHECK-DAG: [[MAPS0:@.+]] = private unnamed_addr constant [1 x i64] [i64 49]
+// CHECK-DAG: [[MAPS1:@.+]] = private unnamed_addr constant [2 x i64] [i64 32, 
i64 281474976710673]
+// CHECK: @main
+int main(void) {
+// CHECK: call void @__tgt_target_data_begin_mapper(i64 -1, i32 1, i8** 
%{{.+}}, i8** %{{.+}}, i64* getelementptr inbounds ([1 x i64], [1 x i64]* 
[[SIZES0]], i32 0, i32 0), i64* getelementptr inbounds ([1 x i64], [1 x i64]* 
[[MAPS0]], i32 0, i32 0), i8** null)
+#pragma omp target enter data map(to : objects [0:1])
+// CHECK: call void @__tgt_target_data_begin_mapper(i64 -1, i32 2, i8** 
%{{.+}}, i8** %{{.+}}, i64* %{{.+}}, i64* getelementptr inbounds ([2 x i64], [2 
x i64]* [[MAPS1]], i32 0, i32 0), i8** null)
+#pragma omp target enter data

[PATCH] D13673: Add initial support for the MUSL C library.

2020-11-17 Thread Louis Dionne via Phabricator via cfe-commits

ldionne added a comment.

In D13673#2394187 , @jroelofs wrote:

> More context: https://www.openwall.com/lists/musl/2013/03/29/13

Thanks for the context. This is a very puristic point of view from the musl 
author.. not very useful. There are things that are difficult to detect with a 
check like `__has_feature` or similar, and having a macro to detect the library 
is necessary.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D13673

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

[PATCH] D91659: Allow anonymous enum typedefs to be referenced with the 'enum' specifier under MSVC compat mode

2020-11-17 Thread Shivanshu Goyal via Phabricator via cfe-commits

shivanshu3 created this revision.
shivanshu3 added reviewers: zahen, tiagoma, rnk, hans.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
shivanshu3 requested review of this revision.

Goal: Clang should be able to parse the following code with 
'-fms-compatibility' because MSVC allows using the 'enum' specifier for 
typedef'd anonymous enums:

  typedef enum
  {
  First,
  Second
  } MyEnum;
  
  void func()
  {
  enum MyEnum foo;
  }

Today the code above produces the following compile error:

  :9:7: error: typedef 'MyEnum' cannot be referenced with a enum 
specifier
  enum MyEnum foo;
   ^
  :5:3: note: declared here
  } MyEnum;
^

The reason why this change is desired in Clang is because the MSVC tools 
'mktyplib' and 'midl' produce C++ code which uses the 'enum' specifier in such 
a way. Here is a small repro:

MyEnum.idl:

  [
uuid(da2889bf-6173-4c1c-a23d-52ad85fc91ca)
  ]
  library MyEnumLib
  {
typedef enum { X } MyEnum;
  };

Application.idl:

  [
uuid(4d0cc96f-3258-46f7-98bf-6654c1d027c5)
  ]
  library ApplicationLib
  {
importlib("MyEnum.tlb");
  
[
  uuid(6ac4c3d7-15c7-40c7-8b64-60459e29680b)
]
interface ApplicationInterface : IDispatch
{
  [
id(66),
propget
  ]
  HRESULT Foo([out, retval] enum MyEnum* Foo);
  
  [
id(66),
propput
  ]
  HRESULT Foo([in] enum MyEnum Foo);
};
  };

Use the following commands to build:

  mktyplib /win32 /cpp_cmd cl.exe /cpp_opt /E /tlb MyEnum.tlb /h MyEnum.h 
MyEnum.idl
  midl -h Application.h -iid Application.c -tlb Application.tlb /cpp_cmd cl.exe 
Application.idl

This produces Application.h with the following snippet (note the usage of the 
'enum' specifier):

  MIDL_INTERFACE("6ac4c3d7-15c7-40c7-8b64-60459e29680b")
  ApplicationInterface : public IDispatch
  {
  public:
  virtual /* [propget][id] */ HRESULT STDMETHODCALLTYPE get_Foo( 
  /* [retval][out] */ enum /* external definition not present */ MyEnum 
*Foo) = 0;
  
  virtual /* [propput][id] */ HRESULT STDMETHODCALLTYPE put_Foo( 
  /* [in] */ enum /* external definition not present */ MyEnum Foo) = 0;
  
  };

Note that the 'mktyplib' tool is deprecated now but it's used for building some 
code here at Microsoft, and probably other legacy build scenarios in the 
industry as well.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91659

Files:
  clang/lib/Sema/SemaDecl.cpp
  clang/test/Sema/enum-typedef-msvc.c
  clang/test/Sema/enum-typedef-msvc.cpp


Index: clang/test/Sema/enum-typedef-msvc.cpp
===
--- /dev/null
+++ clang/test/Sema/enum-typedef-msvc.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility %s
+
+typedef enum {
+  First,
+  Second
+} MyEnum;
+
+class AMyInterface {
+  virtual void MyFunc(enum MyEnum *param) = 0;
+};
+
+class MyImpl : public AMyInterface {
+  virtual void MyFunc(enum MyEnum *param) override {}
+};
+
+// expected-no-diagnostics
Index: clang/test/Sema/enum-typedef-msvc.c
===
--- /dev/null
+++ clang/test/Sema/enum-typedef-msvc.c
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fms-compatibility -DUSE_MSVC_COMPAT 
%s
+
+typedef enum {
+  A
+} Foo;
+
+void func() {
+#ifdef USE_MSVC_COMPAT
+  enum Foo foo; // expected-no-diagnostics
+#else
+  enum Foo foo; // expected-error {{variable has incomplete type 'enum Foo'}} 
// expected-note {{forward declaration of 'enum Foo'}}
+#endif
+  (void)foo;
+}
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -15539,6 +15539,34 @@
 // shouldn't be diagnosing.
 LookupName(Previous, S);
 
+// Under MSVC, the 'enum' specifier can be used for typedef'd enums.
+// Note that lookup only fails in C, not C++, so this if condition
+// is only used for C code.
+if (getLangOpts().MSVCCompat && (Kind == TTK_Enum) && Previous.empty() &&
+(TUK == TUK_Reference)) {
+  LookupResult TypedefEnumLookup(*this, Name, NameLoc, LookupOrdinaryName,
+ Redecl);
+  LookupName(TypedefEnumLookup, S);
+
+  if (!TypedefEnumLookup.empty()) {
+if (TypedefNameDecl *TD =
+dyn_cast(TypedefEnumLookup.getFoundDecl())) {
+  const Type *UnderlyingTypePtr =
+  TD->getUnderlyingType().getTypePtrOrNull();
+
+  if (UnderlyingTypePtr) {
+if (EnumDecl *UnderlyingEnumDecl =
+dyn_cast(UnderlyingTypePtr->getAsTagDecl())) {
+  // We only allow this for anonymo

[PATCH] D90719: [DebugInfo] Modify ctor homing as workaround for unconstructed libcxx types

2020-11-17 Thread Louis Dionne via Phabricator via cfe-commits

ldionne added a comment.

Sorry, I read this last week and probably forgot to reply.

I think the right thing to do is to fix the UB, which appears to mean fixing 
libc++. However, can you take a look at whether defining these macros help?

  // Fix undefined behavior in  how __tree stores its end and parent nodes.
  #  define _LIBCPP_ABI_TREE_REMOVE_NODE_POINTER_UB
  // Fix undefined behavior in how __hash_table stores its pointer types.
  #  define _LIBCPP_ABI_FIX_UNORDERED_NODE_POINTER_UB
  #  define _LIBCPP_ABI_FORWARD_LIST_REMOVE_NODE_POINTER_UB
  #  define _LIBCPP_ABI_FIX_UNORDERED_CONTAINER_SIZE_TYPE

We have some known UB in our container types, but fixing that UB is an ABI 
break, so it's only enabled when some specific macros are turned on. I think 
you might be hitting that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90719

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

[PATCH] D91311: Add new 'preferred_name' attribute.

2020-11-17 Thread Louis Dionne via Phabricator via cfe-commits

ldionne added subscribers: david_stone, mattcalabrese, mpark.
ldionne added a comment.

In D91311#2400917 , @dblaikie wrote:

> How would that work for users - they would get error messages from the 
> compiler using type names that don't exist in the source code? I'd have 
> thought that would be quite confusing.

Yes, if a library author decides to say something like:

  template
  class [[preferred_name(basic_string_view, "ahaha I'm such a troll")]]
  basic_string_view {
  ...
  };

Then you might get compiler errors that are not super helpful. I don't think 
the fact that such nonsense is doable means that we shouldn't give this control 
to library authors.

For instance, I can easily imagine a library that provides an API where some 
types shouldn't be named (for example expression templates). In that case, you 
might want to describe a type by a string along the lines of 
`decltype(some-expression)`, which could potentially be a lot more useful than 
the ability to refer to a typedef. Does this sort of usage ring true to someone 
else?

Pinging fellow library folks in case they have an opinion. @mattcalabrese 
@david_stone @mpark

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91311

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

[PATCH] D90719: [DebugInfo] Modify ctor homing as workaround for unconstructed libcxx types

2020-11-17 Thread Amy Huang via Phabricator via cfe-commits

akhuang added a comment.

Ok, it seems like the general opinion here is that libc++ should be changed in 
some way and not ctor homing. I don't know who'd be responsible for changing 
the libc++ code, though.

Maybe I should file a bug for libc++?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90719

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

[PATCH] D91585: [NPM] Move more O0 pass building into PassBuilder

2020-11-17 Thread Arthur Eubanks via Phabricator via cfe-commits

aeubanks updated this revision to Diff 305900.
aeubanks added a comment.

make build(Thin)LTODefaultPipeline handle O0


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91585

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/lib/Passes/PassBuilder.cpp
  llvm/test/Other/new-pass-manager.ll
  llvm/test/Other/new-pm-defaults.ll
  llvm/test/Other/new-pm-thinlto-defaults.ll
  llvm/test/Other/new-pm-thinlto-prelink-pgo-defaults.ll
  llvm/test/Other/new-pm-thinlto-prelink-samplepgo-defaults.ll
  llvm/test/Transforms/CanonicalizeAliases/canonicalize.ll
  llvm/test/Transforms/NameAnonGlobals/rename.ll

Index: llvm/test/Transforms/NameAnonGlobals/rename.ll
===
--- llvm/test/Transforms/NameAnonGlobals/rename.ll
+++ llvm/test/Transforms/NameAnonGlobals/rename.ll
@@ -1,5 +1,6 @@
 ; RUN: opt -S -name-anon-globals < %s | FileCheck %s
-; RUN: opt -prepare-for-thinlto -O0 -module-summary -o %t.bc < %s
+; RUN: opt -prepare-for-thinlto -O0 -module-summary -o %t.bc -enable-new-pm=0 < %s
+; RUN: opt -passes='thinlto-pre-link,require' -o %t.bc < %s
 
 
 ; foo contribute to the unique hash for the module
Index: llvm/test/Transforms/CanonicalizeAliases/canonicalize.ll
===
--- llvm/test/Transforms/CanonicalizeAliases/canonicalize.ll
+++ llvm/test/Transforms/CanonicalizeAliases/canonicalize.ll
@@ -1,6 +1,7 @@
 ; RUN: opt -S -canonicalize-aliases < %s | FileCheck %s
-; RUN: opt -prepare-for-thinlto -O0 -module-summary -o - < %s | llvm-dis -o - | FileCheck %s
 ; RUN: opt -S -passes=canonicalize-aliases < %s | FileCheck %s
+; RUN: opt -prepare-for-thinlto -O0 -module-summary -o - < %s -enable-new-pm=0 | llvm-dis -o - | FileCheck %s
+; RUN: opt -passes='thinlto-pre-link,require' -o - < %s | llvm-dis -o - | FileCheck %s
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
Index: llvm/test/Other/new-pm-thinlto-prelink-samplepgo-defaults.ll
===
--- llvm/test/Other/new-pm-thinlto-prelink-samplepgo-defaults.ll
+++ llvm/test/Other/new-pm-thinlto-prelink-samplepgo-defaults.ll
@@ -2,27 +2,27 @@
 ;
 ; RUN: opt -disable-verify -debug-pass-manager \
 ; RUN: -pgo-kind=pgo-sample-use-pipeline -profile-file='%S/Inputs/new-pm-thinlto-samplepgo-defaults.prof' \
-; RUN: -passes='thinlto-pre-link,name-anon-globals' -S %s 2>&1 \
+; RUN: -passes='thinlto-pre-link' -S %s 2>&1 \
 ; RUN: | FileCheck %s --check-prefixes=CHECK-O,CHECK-O1
 ; RUN: opt -disable-verify -debug-pass-manager \
 ; RUN: -pgo-kind=pgo-sample-use-pipeline -profile-file='%S/Inputs/new-pm-thinlto-samplepgo-defaults.prof' \
-; RUN: -passes='thinlto-pre-link,name-anon-globals' -S  %s 2>&1 \
+; RUN: -passes='thinlto-pre-link' -S  %s 2>&1 \
 ; RUN: | FileCheck %s --check-prefixes=CHECK-O,CHECK-O2,CHECK-O23SZ
 ; RUN: opt -disable-verify -debug-pass-manager \
 ; RUN: -pgo-kind=pgo-sample-use-pipeline -profile-file='%S/Inputs/new-pm-thinlto-samplepgo-defaults.prof' \
-; RUN: -passes='thinlto-pre-link,name-anon-globals' -S -passes-ep-pipeline-start='no-op-module' %s 2>&1 \
+; RUN: -passes='thinlto-pre-link' -S -passes-ep-pipeline-start='no-op-module' %s 2>&1 \
 ; RUN: | FileCheck %s --check-prefixes=CHECK-O,CHECK-O3,CHECK-O23SZ,CHECK-EP-PIPELINE-START
 ; RUN: opt -disable-verify -debug-pass-manager \
 ; RUN: -pgo-kind=pgo-sample-use-pipeline -profile-file='%S/Inputs/new-pm-thinlto-samplepgo-defaults.prof' \
-; RUN: -passes='thinlto-pre-link,name-anon-globals' -S %s 2>&1 \
+; RUN: -passes='thinlto-pre-link' -S %s 2>&1 \
 ; RUN: | FileCheck %s --check-prefixes=CHECK-O,CHECK-Os,CHECK-O23SZ
 ; RUN: opt -disable-verify -debug-pass-manager \
 ; RUN: -pgo-kind=pgo-sample-use-pipeline -profile-file='%S/Inputs/new-pm-thinlto-samplepgo-defaults.prof' \
-; RUN: -passes='thinlto-pre-link,name-anon-globals' -S %s 2>&1 \
+; RUN: -passes='thinlto-pre-link' -S %s 2>&1 \
 ; RUN: | FileCheck %s --check-prefixes=CHECK-O,CHECK-Oz,CHECK-O23SZ
 ; RUN: opt -disable-verify -debug-pass-manager -new-pm-debug-info-for-profiling \
 ; RUN: -pgo-kind=pgo-sample-use-pipeline -profile-file='%S/Inputs/new-pm-thinlto-samplepgo-defaults.prof' \
-; RUN: -passes='thinlto-pre-link,name-anon-globals' -S  %s 2>&1 \
+; RUN: -passes='thinlto-pre-link' -S  %s 2>&1 \
 ; RUN: | FileCheck %s --check-prefixes=CHECK-O,CHECK-O2,CHECK-O23SZ
 ;
 ; CHECK-O: Starting {{.*}}Module pass manager run.
@@ -173,6 +173,7 @@
 ; CHECK-O-NEXT: Finished {{.*}}Module pass manager run.
 ; CHECK-O-NEXT: Running pass: GlobalOptPass
 ; CHECK-O-NEXT: Running pass: AnnotationRemarksPass on foo
+; CHECK-O-NEXT: Running pass: CanonicalizeAliasesPass
 ; CHECK-O-NEXT: Running pass: NameAnonGlobalPass
 ; CHECK-O-N

[PATCH] D91311: Add new 'preferred_name' attribute.

2020-11-17 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

In D91311#2400775 , @ldionne wrote:

> In D91311#2398526 , @rsmith wrote:
>
>> [...]
>
> Thanks for your detailed explanation. I did not understand the philosophy of 
> the attribute, and it's now clear to me that it shouldn't be tied to the 
> typedef, indeed.
>
>> There's another design approach we could follow, that would keep the 
>> attribute on the template but avoid the awkwardness of requiring the typedef 
>> to appear first: we could completely divorce this feature from typedefs. 
>> Instead, we could say the attribute is of the form `preferred_name(type, 
>> "name")`, where `type` is a specialization of the type to which the 
>> attribute is attached, and `"name"` is a name used in diagnostics when 
>> referring to that type, which is printed as if it's a member of the 
>> enclosing namespace (but there'd be no enforcement that such a member 
>> actually exists and declares the corresponding type). What do you think?
>
> I think this is pretty interesting, actually. While it does seem a bit more 
> loosey-goosey in terms of the type system, I can imagine useful applications 
> of being able to use arbitrary strings, such as providing better diagnostics 
> for specific types without having to actually have an associated typedef.

How would that work for users - they would get error messages from the compiler 
using type names that don't exist in the source code? I'd have thought that 
would be quite confusing.

> Just to clarify, that would look like this, right?
>
>   template
>   class [[preferred_name(basic_string_view, "string_view")]]
> [[preferred_name(basic_string_view, "u8string_view")]]
> [[preferred_name(basic_string_view, "u16string_view")]]
> [[preferred_name(basic_string_view, "u32string_view")]]
> [[preferred_name(basic_string_view, "wstring_view")]]
>   basic_string_view {
>   ...
>   };
>
> I don't want to ask for something unreasonable.. but I think that's a neat 
> design that's actually more powerful than the current one. I would be curious 
> to hear what other people think as well, to see whether I'm the only one 
> preferring the more free-form approach.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91311

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

[PATCH] D91656: [clang-tidy] add concurrent module

2020-11-17 Thread Roman Lebedev via Phabricator via cfe-commits

lebedev.ri added a comment.

I'm not a native english speaker, but i think the module name be `concurrency`, 
not `concurrent`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91656

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

[PATCH] D91656: [clang-tidy] add concurrent module

2020-11-17 Thread Nathan James via Phabricator via cfe-commits

njames93 added a comment.

You have messed up the revisions, this should be the parent, D90944 
 should be the child. Thats why the build bot 
can't apply the patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91656

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

[PATCH] D91485: [clang-tidy] ElseAfterReturn check wont suggest fixes if preprocessor branches are involved

2020-11-17 Thread Nathan James via Phabricator via cfe-commits

njames93 updated this revision to Diff 305898.
njames93 added a comment.

Whoops missed one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91485

Files:
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
  clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
@@ -226,3 +226,89 @@
   }
   return;
 }
+
+void testPPConditionals() {
+
+  // These cases the return isn't inside the conditional so diagnose as normal.
+  if (true) {
+return;
+#if 1
+#endif
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+return;
+  }
+  if (true) {
+#if 1
+#endif
+return;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+return;
+  }
+
+  // No return here in the AST, no special handling needed.
+  if (true) {
+#if 0
+return;
+#endif
+  } else {
+return;
+  }
+
+  // Return here is inside a preprocessor conditional block, ignore this case.
+  if (true) {
+#if 1
+return;
+#endif
+  } else {
+return;
+  }
+
+  // These cases, same as above but with an #else block.
+  if (true) {
+#if 1
+return;
+#else
+#endif
+  } else {
+return;
+  }
+  if (true) {
+#if 0
+#else
+return;
+#endif
+  } else {
+return;
+  }
+
+// Ensure it can handle macros.
+#define RETURN return
+  if (true) {
+#if 1
+RETURN;
+#endif
+  } else {
+return;
+  }
+#define ELSE else
+  if (true) {
+#if 1
+return;
+#endif
+  }
+  ELSE {
+return;
+  }
+
+  // Whole statement is in a conditional block so diagnose as normal.
+#if 1
+  if (true) {
+return;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+return;
+  }
+#endif
+}
Index: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
===
--- clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
+++ clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
@@ -10,6 +10,7 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_ELSEAFTERRETURNCHECK_H
 
 #include "../ClangTidyCheck.h"
+#include "llvm/ADT/DenseMap.h"
 
 namespace clang {
 namespace tidy {
@@ -23,12 +24,18 @@
   ElseAfterReturnCheck(StringRef Name, ClangTidyContext *Context);
 
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+   Preprocessor *ModuleExpanderPP) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
+  using ConditionalBranchMap =
+  llvm::DenseMap>;
+
 private:
   const bool WarnOnUnfixable;
   const bool WarnOnConditionVariables;
+  ConditionalBranchMap PPConditionals;
 };
 
 } // namespace readability
Index: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
@@ -10,6 +10,7 @@
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
+#include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/FixIt.h"
 #include "llvm/ADT/SmallVector.h"
 
@@ -19,10 +20,30 @@
 namespace tidy {
 namespace readability {
 
-static const char ReturnStr[] = "return";
-static const char ContinueStr[] = "continue";
-static const char BreakStr[] = "break";
-static const char ThrowStr[] = "throw";
+namespace {
+
+class PPConditionalCollector : public PPCallbacks {
+public:
+  PPConditionalCollector(
+  ElseAfterReturnCheck::ConditionalBranchMap &Collections,
+  const SourceManager &SM)
+  : Collections(Collections), SM(SM) {}
+  void Endif(SourceLocation Loc, SourceLocation IfLoc) override {
+if (!SM.isWrittenInSameFile(Loc, IfLoc))
+  return;
+auto &Collection = Collections[SM.getFileID(Loc)];
+assert(Collection.empty() || Collection.back().getEnd() < Loc);
+Collection.emplace_back(IfLoc, Loc);
+  }
+
+private:
+  ElseAfterReturnCheck::ConditionalBranchMap &Collections;
+  const SourceManager &SM;
+};
+
+} // namespace
+
+static const char InterruptingStr[] = "interrupting";
 static const char WarningMessage[] = "do not use 'else' after '%0'";
 static const char WarnOnUnfixableStr[] = "WarnOnUnfixable";
 static const char WarnOnConditionVariablesStr[] = "

[PATCH] D91485: [clang-tidy] ElseAfterReturn check wont suggest fixes if preprocessor branches are involved

2020-11-17 Thread Nathan James via Phabricator via cfe-commits

njames93 updated this revision to Diff 305897.
njames93 marked an inline comment as done.
njames93 added a comment.

Address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91485

Files:
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
  clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp
@@ -226,3 +226,89 @@
   }
   return;
 }
+
+void testPPConditionals() {
+
+  // These cases the return isn't inside the conditional so diagnose as normal.
+  if (true) {
+return;
+#if 1
+#endif
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+return;
+  }
+  if (true) {
+#if 1
+#endif
+return;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+return;
+  }
+
+  // No return here in the AST, no special handling needed.
+  if (true) {
+#if 0
+return;
+#endif
+  } else {
+return;
+  }
+
+  // Return here is inside a preprocessor conditional block, ignore this case.
+  if (true) {
+#if 1
+return;
+#endif
+  } else {
+return;
+  }
+
+  // These cases, same as above but with an #else block.
+  if (true) {
+#if 1
+return;
+#else
+#endif
+  } else {
+return;
+  }
+  if (true) {
+#if 0
+#else
+return;
+#endif
+  } else {
+return;
+  }
+
+// Ensure it can handle macros.
+#define RETURN return
+  if (true) {
+#if 1
+RETURN;
+#endif
+  } else {
+return;
+  }
+#define ELSE else
+  if (true) {
+#if 1
+return;
+#endif
+  }
+  ELSE {
+return;
+  }
+
+  // Whole statement is in a conditional block so diagnost as normal.
+#if 1
+  if (true) {
+return;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not use 'else' after 'return'
+return;
+  }
+#endif
+}
Index: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
===
--- clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
+++ clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.h
@@ -10,6 +10,7 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_ELSEAFTERRETURNCHECK_H
 
 #include "../ClangTidyCheck.h"
+#include "llvm/ADT/DenseMap.h"
 
 namespace clang {
 namespace tidy {
@@ -23,12 +24,18 @@
   ElseAfterReturnCheck(StringRef Name, ClangTidyContext *Context);
 
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+   Preprocessor *ModuleExpanderPP) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
+  using ConditionalBranchMap =
+  llvm::DenseMap>;
+
 private:
   const bool WarnOnUnfixable;
   const bool WarnOnConditionVariables;
+  ConditionalBranchMap PPConditionals;
 };
 
 } // namespace readability
Index: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
@@ -10,6 +10,7 @@
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
+#include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/FixIt.h"
 #include "llvm/ADT/SmallVector.h"
 
@@ -19,10 +20,30 @@
 namespace tidy {
 namespace readability {
 
-static const char ReturnStr[] = "return";
-static const char ContinueStr[] = "continue";
-static const char BreakStr[] = "break";
-static const char ThrowStr[] = "throw";
+namespace {
+
+class PPConditionalCollector : public PPCallbacks {
+public:
+  PPConditionalCollector(
+  ElseAfterReturnCheck::ConditionalBranchMap &Collections,
+  const SourceManager &SM)
+  : Collections(Collections), SM(SM) {}
+  void Endif(SourceLocation Loc, SourceLocation IfLoc) override {
+if (!SM.isWrittenInSameFile(Loc, IfLoc))
+  return;
+auto &Collection = Collections[SM.getFileID(Loc)];
+assert(Collection.empty() || Collection.back().getEnd() < Loc);
+Collection.emplace_back(IfLoc, Loc);
+  }
+
+private:
+  ElseAfterReturnCheck::ConditionalBranchMap &Collections;
+  const SourceManager &SM;
+};
+
+} // namespace
+
+static const char InterruptingStr[] = "interrupting";
 static const char WarningMessage[] = "do not use 'else' after '%0'";
 static const char WarnOnUnfixableStr[] = "WarnOnUnfixable";
 static co

[PATCH] D91485: [clang-tidy] ElseAfterReturn check wont suggest fixes if preprocessor branches are involved

2020-11-17 Thread Nathan James via Phabricator via cfe-commits

njames93 marked 8 inline comments as done.
njames93 added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp:313
+#endif
+}

aaron.ballman wrote:
> We should probably add some tests for more pathological cases, like:
> ```
> #if 1
> if (true) {
>   return;
> #else
> {
> #endif
> } else {
>   return;
> }
> ```
I'm don't even think its worth adding tests for that kind of code. Its 
impossible to reason about at the AST level(we dont get any AST for discarded 
pp conditional branches). So for these or cases like :
```lang=c++
if (true) {
#if 1
  return;
} else {
#endif
  return;
}
```
Its just simpler to leave the behaviour as is and hope that a user sees the 
change and addresses (or suppresses) it manually.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91485

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

[PATCH] D91279: [PowerPC] DForm instructions should be preferred when using zero register

2020-11-17 Thread Stefan Pintilie via Phabricator via cfe-commits

stefanp added a comment.

In D91279#2390160 , @shchenz wrote:

> Using dform with offset 0 can save one register r0/X0, this is benefit for 
> register allocation? But adding it in `PPCPreEmitPeephole` pass which is 
> after register allocation will make the benefit gone.
> Maybe we need to do it before register allocation? For example at the place 
> where the x-form with zero register is generated.
>
> I checked one example `loadConstant` in  
> `test/CodeGen/PowerPC/f128-passByValue.ll`.
> We generate `LXVX $zero8, ` in ISEL because we meet the worst case and we 
> don't have d-form choice for the instruction selection. so we have to use 
> x-form and in x-form selection, we have to use zero/zero8 as the base and use 
> load address as the index. See `PPCTargetLowering::SelectAddressRegRegOnly`.
>
> I guess most cases are with same reason for generating x-form + zero 
> register, we meet the worst case in ISEL, so we have to use x-form + zero 
> register form, with this form, we can always select a powerpc load/store 
> instruction.
>
> For me, a better solution should be change the worst case handling in ISEL, 
> it is before RA and it is also transparent for types like STXVX/LXVX/ and 
> also LDX/STDX, LFDX/STFDX...

I'm just going to jump in to give a little more background. The initial reason 
we wanted to do this was to enable an optimization that actually happens in the 
linker after the code is emitted.
To get the idea you can look at this test:

  /llvm/test/CodeGen/PowerPC/pcrel-linkeropt.ll

Which contains this section:

  ; FIXME: we should always convert X-Form instructions that use
  ; PPC::ZERO[8] to the corresponding D-Form so we can perform this opt.
  define dso_local void @ReadWrite128() local_unnamed_addr #0 {
  ; CHECK-LABEL: ReadWrite128:
  ; CHECK:   # %bb.0: # %entry
  ; CHECK-NEXT:pld r3, input128@got@pcrel(0), 1
  ; CHECK-NEXT:lxvx vs0, 0, r3
  ; CHECK-NEXT:pld r3, output128@got@pcrel(0), 1
  ; CHECK-NEXT:stxvx vs0, 0, r3
  ; CHECK-NEXT:blr
  entry:
%0 = load i128, i128* @input128, align 16
store i128 %0, i128* @output128, align 16
ret void
  }

When we have a GOT access like this it is possible for the compiler to mark the 
instruction with `R_PPC64_PCREL_OPT` and then the linker merges the two 
instructions into one and replaces the second instruction with a `nop`. The 
problem is that this opt can only be done if the second instruction is DForm. 
We noticed that when we implemented this optimization we could not catch all of 
the cases because in some situations (like the one above) we use the XForm 
instead of the DForm.

Having said that, we should try to do this before the PreEmitPeephole. The 
optimization that adds the `R_PPC64_PCREL_OPT` relocation is also in the 
PreEmitPeephole and I'm not sure if it will be detected if we do both things at 
the same time (both as in convert the XForm to a DForm and then have the same 
opt use that DForm to add the relocation).

I agree that ISel is a better place for this. If we cannot do this in ISel then 
we should still try to do this before we get to the PreEmitPeephole or at least 
make sure that both the DForm is present and that the `R_PPC64_PCREL_OPT` 
relocation is added as we expected in the same pass.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91279

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

[PATCH] D72184: [BPF] support atomic instructions

2020-11-17 Thread Yonghong Song via Phabricator via cfe-commits

yonghong-song updated this revision to Diff 305895.
yonghong-song added a comment.

use the same register for dst and val for fetch-add, fetch-sub and xchg 
instructions.
for fetch-sub, if it is deemed later using different registers for dst/val is 
preferred, I can make the change then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72184

Files:
  clang/lib/Basic/Targets/BPF.cpp
  clang/test/Misc/target-invalid-cpu-note.c
  llvm/lib/Target/BPF/BPF.td
  llvm/lib/Target/BPF/BPFInstrFormats.td
  llvm/lib/Target/BPF/BPFInstrInfo.td
  llvm/lib/Target/BPF/BPFSubtarget.cpp
  llvm/lib/Target/BPF/BPFSubtarget.h
  llvm/lib/Target/BPF/Disassembler/BPFDisassembler.cpp
  llvm/test/CodeGen/BPF/atomics.ll
  llvm/test/CodeGen/BPF/atomics_2.ll

Index: llvm/test/CodeGen/BPF/atomics_2.ll
===
--- /dev/null
+++ llvm/test/CodeGen/BPF/atomics_2.ll
@@ -0,0 +1,139 @@
+; RUN: llc < %s -march=bpfel -verify-machineinstrs -show-mc-encoding -mcpu=v4 | FileCheck %s
+;
+; Source:
+;   char test_load_sub_8(char *p, char v) {
+; return __sync_fetch_and_sub(p, v);
+;   }
+;   short test_load_sub_16(short *p, short v) {
+; return __sync_fetch_and_sub(p, v);
+;   }
+;   int test_load_sub_32(int *p, int v) {
+; return __sync_fetch_and_sub(p, v);
+;   }
+;   int test_load_sub_64(long *p, long v) {
+; return __sync_fetch_and_sub(p, v);
+;   }
+;   int test_xchg_8(char *p, char v) {
+; return __sync_lock_test_and_set(p, v);
+;   }
+;   int test_xchg_16(short *p, short v) {
+; return __sync_lock_test_and_set(p, v);
+;   }
+;   int test_xchg_32(int *p, int v) {
+; return __sync_lock_test_and_set(p, v);
+;   }
+;   int test_xchg_64(long *p, long v) {
+; return __sync_lock_test_and_set(p, v);
+;   }
+;   int test_cas_32(int *p, int old, int new) {
+; return __sync_val_compare_and_swap(p, old, new);
+;   }
+;   long test_cas_64(long *p, long old, long new) {
+; return __sync_val_compare_and_swap(p, old, new);
+;   }
+
+; CHECK-LABEL: test_load_sub_8
+; CHECK: w0 = w2
+; CHECK: w0 = atomic_fetch_sub((u8 *)(r1 + 0), w0)
+; CHECK: encoding: [0xd3,0x01,0x00,0x00,0x11,0x00,0x00,0x00]
+define dso_local signext i8 @test_load_sub_8(i8* nocapture %p, i8 signext %v) local_unnamed_addr #0 {
+entry:
+  %0 = atomicrmw sub i8* %p, i8 %v seq_cst
+  ret i8 %0
+}
+
+; CHECK-LABEL: test_load_sub_16
+; CHECK: w0 = w2
+; CHECK: w0 = atomic_fetch_sub((u16 *)(r1 + 0), w0)
+; CHECK: encoding: [0xcb,0x01,0x00,0x00,0x11,0x00,0x00,0x00]
+define dso_local signext i16 @test_load_sub_16(i16* nocapture %p, i16 signext %v) local_unnamed_addr #0 {
+entry:
+  %0 = atomicrmw sub i16* %p, i16 %v seq_cst
+  ret i16 %0
+}
+
+; CHECK-LABEL: test_load_sub_32
+; CHECK: w0 = w2
+; CHECK: w0 = atomic_fetch_sub((u32 *)(r1 + 0), w0)
+; CHECK: encoding: [0xc3,0x01,0x00,0x00,0x11,0x00,0x00,0x00]
+define dso_local i32 @test_load_sub_32(i32* nocapture %p, i32 %v) local_unnamed_addr {
+entry:
+  %0 = atomicrmw sub i32* %p, i32 %v seq_cst
+  ret i32 %0
+}
+
+; CHECK-LABEL: test_load_sub_64
+; CHECK: r0 = r2
+; CHECK: r0 = atomic_fetch_sub((u64 *)(r1 + 0), r0)
+; CHECK: encoding: [0xdb,0x01,0x00,0x00,0x11,0x00,0x00,0x00]
+define dso_local i32 @test_load_sub_64(i64* nocapture %p, i64 %v) local_unnamed_addr {
+entry:
+  %0 = atomicrmw sub i64* %p, i64 %v seq_cst
+  %conv = trunc i64 %0 to i32
+  ret i32 %conv
+}
+
+; CHECK-LABEL: test_xchg_8
+; CHECK: w0 = w2
+; CHECK: w0 = xchg32_8(r1 + 0, w0)
+; CHECK: encoding: [0xd3,0x01,0x00,0x00,0xe1,0x00,0x00,0x00]
+define dso_local i32 @test_xchg_8(i8* nocapture %p, i8 signext %v) local_unnamed_addr {
+entry:
+  %0 = atomicrmw xchg i8* %p, i8 %v seq_cst
+  %conv = sext i8 %0 to i32
+  ret i32 %conv
+}
+
+; CHECK-LABEL: test_xchg_16
+; CHECK: w0 = w2
+; CHECK: w0 = xchg32_16(r1 + 0, w0)
+; CHECK: encoding: [0xcb,0x01,0x00,0x00,0xe1,0x00,0x00,0x00]
+define dso_local i32 @test_xchg_16(i16* nocapture %p, i16 signext %v) local_unnamed_addr {
+entry:
+  %0 = atomicrmw xchg i16* %p, i16 %v seq_cst
+  %conv = sext i16 %0 to i32
+  ret i32 %conv
+}
+
+; CHECK-LABEL: test_xchg_32
+; CHECK: w0 = w2
+; CHECK: w0 = xchg32_32(r1 + 0, w0)
+; CHECK: encoding: [0xc3,0x01,0x00,0x00,0xe1,0x00,0x00,0x00]
+define dso_local i32 @test_xchg_32(i32* nocapture %p, i32 %v) local_unnamed_addr {
+entry:
+  %0 = atomicrmw xchg i32* %p, i32 %v seq_cst
+  ret i32 %0
+}
+
+; CHECK-LABEL: test_xchg_64
+; CHECK: r0 = r2
+; CHECK: r0 = xchg_64(r1 + 0, r0)
+; CHECK: encoding: [0xdb,0x01,0x00,0x00,0xe1,0x00,0x00,0x00]
+define dso_local i32 @test_xchg_64(i64* nocapture %p, i64 %v) local_unnamed_addr {
+entry:
+  %0 = atomicrmw xchg i64* %p, i64 %v seq_cst
+  %conv = trunc i64 %0 to i32
+  ret i32 %conv
+}
+
+; CHECK-LABEL: test_cas_32
+; CHECK: w0 = w2
+; CHECK: w0 = cmpxchg32_32(r1 + 0, w0, w3)
+; CHECK: encoding: [0xc3,0x01,0x00,0x00,0xf1,0x03,0x00,0x00]
+define dso_local i32 @test_cas_32(i32* nocapture %p, i3

[PATCH] D85808: [Remarks][2/2] Expand remarks hotness threshold option support in more tools

2020-11-17 Thread Wei Wang via Phabricator via cfe-commits

weiwang updated this revision to Diff 305891.
weiwang added a comment.

update test case for clang option pass-through


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85808

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Driver/opt-record.c
  llvm/include/llvm/Analysis/ProfileSummaryInfo.h
  llvm/include/llvm/IR/LLVMContext.h
  llvm/include/llvm/IR/Module.h
  llvm/lib/Analysis/OptimizationRemarkEmitter.cpp
  llvm/lib/IR/LLVMContext.cpp
  llvm/lib/IR/LLVMRemarkStreamer.cpp
  llvm/lib/IR/Module.cpp
  llvm/test/Other/optimization-remarks-auto.ll
  llvm/test/Transforms/SampleProfile/Inputs/remarks-hotness.prof
  llvm/test/Transforms/SampleProfile/remarks-hotness.ll

Index: llvm/test/Transforms/SampleProfile/remarks-hotness.ll
===
--- /dev/null
+++ llvm/test/Transforms/SampleProfile/remarks-hotness.ll
@@ -0,0 +1,96 @@
+;; This test verifies 'auto' hotness threshold when profile file is provided.
+;;
+;; new PM
+; RUN: rm -f %t.yaml %t.hot.yaml
+; RUN: opt %s --enable-new-pm --passes='sample-profile,cgscc(inline)' \
+; RUN: --sample-profile-file=%S/Inputs/remarks-hotness.prof \
+; RUN: -S --pass-remarks-filter=inline --pass-remarks-output=%t.yaml \
+; RUN: -pass-remarks-with-hotness --disable-output
+; RUN: FileCheck %s -check-prefix=YAML-PASS < %t.yaml
+; RUN: FileCheck %s -check-prefix=YAML-MISS < %t.yaml
+
+;; test 'auto' threshold
+; RUN: opt %s --enable-new-pm --passes='sample-profile,cgscc(inline)' \
+; RUN: --sample-profile-file=%S/Inputs/remarks-hotness.prof \
+; RUN: -S --pass-remarks-filter=inline --pass-remarks-output=%t.hot.yaml \
+; RUN: --pass-remarks-with-hotness --pass-remarks-hotness-threshold=auto --disable-output
+; RUN: FileCheck %s -check-prefix=YAML-PASS < %t.hot.yaml
+; RUN: not FileCheck %s -check-prefix=YAML-MISS < %t.hot.yaml
+
+; RUN: opt %s --enable-new-pm --passes='sample-profile,cgscc(inline)' \
+; RUN: --sample-profile-file=%S/Inputs/remarks-hotness.prof \
+; RUN: -S --pass-remarks=inline --pass-remarks-missed=inline --pass-remarks-analysis=inline \
+; RUN: --pass-remarks-with-hotness --pass-remarks-hotness-threshold=auto --disable-output 2>&1 | FileCheck %s -check-prefix=CHECK-RPASS
+
+; YAML-PASS:  --- !Passed
+; YAML-PASS-NEXT: Pass:inline
+; YAML-PASS-NEXT: Name:Inlined
+; YAML-PASS-NEXT: DebugLoc:{ File: remarks-hotness.cpp, Line: 10, Column: 10 }
+; YAML-PASS-NEXT: Function:_Z7caller1v
+; YAML-PASS-NEXT: Hotness: 401
+
+; YAML-MISS:  --- !Missed
+; YAML-MISS-NEXT: Pass:inline
+; YAML-MISS-NEXT: Name:NeverInline
+; YAML-MISS-NEXT: DebugLoc:{ File: remarks-hotness.cpp, Line: 14, Column: 10 }
+; YAML-MISS-NEXT: Function:_Z7caller2v
+; YAML-MISS-NEXT: Hotness: 2
+
+; CHECK-RPASS: _Z7callee1v inlined into _Z7caller1v with (cost=-30, threshold=4500) at callsite _Z7caller1v:1 (hotness: 401)
+; CHECK-RPASS-NOT: _Z7callee2v not inlined into _Z7caller2v because it should never be inlined (cost=never): noinline function attribute (hotness: 2)
+
+; ModuleID = 'remarks-hotness.cpp'
+source_filename = "remarks-hotness.cpp"
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: use-sample-profile
+define dso_local i32 @_Z7callee1v() #0 !dbg !7 {
+  ret i32 1, !dbg !11
+}
+
+; Function Attrs: noinline nounwind uwtable use-sample-profile
+define dso_local i32 @_Z7callee2v() #1 !dbg !12 {
+  ret i32 2, !dbg !13
+}
+
+; Function Attrs: use-sample-profile
+define dso_local i32 @_Z7caller1v() #0 !dbg !14 {
+  %1 = call i32 @_Z7callee1v(), !dbg !15
+  ret i32 %1, !dbg !16
+}
+
+; Function Attrs: use-sample-profile
+define dso_local i32 @_Z7caller2v() #0 !dbg !17 {
+  %1 = call i32 @_Z7callee2v(), !dbg !18
+  ret i32 %1, !dbg !19
+}
+
+attributes #0 = { "use-sample-profile" }
+attributes #1 = { noinline nounwind uwtable "use-sample-profile" }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, splitDebugInlining: false, debugInfoForProfiling: true, nameTableKind: None)
+!1 = !DIFile(filename: "remarks-hotness.cpp", directory: ".")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang version 11.0.0"}
+!7 = distinct !DISubprogram(name: "callee1", linkageName: "_Z7callee1v", scope: !1, file: !1, line: 1, type: !8, scopeLine: 1, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, un

[PATCH] D91585: [NPM] Move more O0 pass building into PassBuilder

2020-11-17 Thread Arthur Eubanks via Phabricator via cfe-commits

aeubanks planned changes to this revision.
aeubanks added inline comments.



Comment at: llvm/lib/Passes/PassBuilder.cpp:2365
+  // Don't do anything for (thin)lto backend compiles at O0.
+  if (Matches[1] != "thinlto" && Matches[1] != "lto")
+MPM.addPass(buildO0DefaultPipeline(L, Matches[1] != "default"));

aeubanks wrote:
> tejohnson wrote:
> > This seems to change behavior. For one, previously we were only suppressing 
> > adding the PGO Instr passes for ThinLTO. Now this will suppress adding the 
> > coroutines passes and whatever else runRegisteredEPCallbacks was doing. 
> > Also, it's now doing the same for regular LTO but it didn't seem to do any 
> > special handling in that case before. Are these changes intended?
> The callbacks are generally for two purposes. One is to lower certain 
> constructs, which only needs to be done once. It should already have been 
> done in the pre-link step. The other is for optimization purposes at specific 
> points in the pipeline. Since this is -O0, that doesn't matter.
> 
> So I think this makes sense.
Actually, I just noticed that build(Thin)LTODefaultPipeline does handle O0. The 
default and LTO pre-link ones assert when passed O0 so I assumed that was the 
case for the LTO pipelines, but that's not the case. I'll change that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91585

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

[PATCH] D86119: [OPENMP50]Allow overlapping mapping in target constrcuts.

2020-11-17 Thread Alexey Bataev via Phabricator via cfe-commits

ABataev added inline comments.



Comment at: openmp/libomptarget/src/omptarget.cpp:233
 MapperComponents
-.Components[target_data_function == targetDataEnd ? I : E - I - 1];
+.Components[target_data_function == targetDataEnd ? E - I - 1 : I];
 MapperArgsBase[I] = C.Base;

grokos wrote:
> What is the current status of the order of the arguments clang emits? Is it 
> still necessary to traverse arguments in reverse order here?
Yes, still required


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86119

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

[PATCH] D86119: [OPENMP50]Allow overlapping mapping in target constrcuts.

2020-11-17 Thread George Rokos via Phabricator via cfe-commits

grokos added inline comments.



Comment at: openmp/libomptarget/src/omptarget.cpp:233
 MapperComponents
-.Components[target_data_function == targetDataEnd ? I : E - I - 1];
+.Components[target_data_function == targetDataEnd ? E - I - 1 : I];
 MapperArgsBase[I] = C.Base;

What is the current status of the order of the arguments clang emits? Is it 
still necessary to traverse arguments in reverse order here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86119

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

[PATCH] D91585: [NPM] Move more O0 pass building into PassBuilder

2020-11-17 Thread Arthur Eubanks via Phabricator via cfe-commits

aeubanks added inline comments.



Comment at: llvm/lib/Passes/PassBuilder.cpp:2365
+  // Don't do anything for (thin)lto backend compiles at O0.
+  if (Matches[1] != "thinlto" && Matches[1] != "lto")
+MPM.addPass(buildO0DefaultPipeline(L, Matches[1] != "default"));

tejohnson wrote:
> This seems to change behavior. For one, previously we were only suppressing 
> adding the PGO Instr passes for ThinLTO. Now this will suppress adding the 
> coroutines passes and whatever else runRegisteredEPCallbacks was doing. Also, 
> it's now doing the same for regular LTO but it didn't seem to do any special 
> handling in that case before. Are these changes intended?
The callbacks are generally for two purposes. One is to lower certain 
constructs, which only needs to be done once. It should already have been done 
in the pre-link step. The other is for optimization purposes at specific points 
in the pipeline. Since this is -O0, that doesn't matter.

So I think this makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91585

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

[PATCH] D91630: [Parse] Add parsing support for C++ attributes on using-declarations

2020-11-17 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D91630#2400731 , @rsmith wrote:

> Do `[[deprecated]]` and `[[maybe_unused]]` now work for 
> //using-declarator//s? If so, a test for that would be useful.

I think the answer is yes and no, respectively (at least in terms of whether 
the attribute will apply to the subject) -- but 
`[[clang::annotate("whatever")]]` is also a good test attribute as would be any 
attribute whose `Subjects` includes `Named` (for `NamedDecl`s).

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

https://reviews.llvm.org/D91630

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

[PATCH] D91495: [clang-tidy] false-positive for bugprone-redundant-branch-condition in case of passed-by-ref params

2020-11-17 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

Thank you for working on this!




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp:10
+bool tryToExtinguish(bool&);
+bool tryToExtinguishByVal(bool &);
 void tryPutFireOut();

Did you mean for the parameter to be a reference?



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp:963
+  if (tryToExtinguishByVal(isSet) && isSet) {
+if (tryToExtinguishByVal(isSet) && isSet) {
+  // NO-MESSAGE: fire may have been extinguished

FWIW, I would expect this one to be diagnosed if the call really was byval 
instead of byref.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp:968
+  }
+}
+

Another test that would be interesting is:
```
if (tryToExtinguish(isSet) && isSet) {
  if (tryToExtinguishByVal(isSet) && isSet) { // Dupe check of isSet should be 
diagnosed
scream();
  }
}

if (tryToExtinguishByVal(isSet) && isSet) {
  if (tryToExtinguish(isSet) && isSet) { // Ok
scream();
  }
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91495

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

[PATCH] D91485: [clang-tidy] ElseAfterReturn check wont suggest fixes if preprocessor branches are involved

2020-11-17 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:118
 static void removeElseAndBrackets(DiagnosticBuilder &Diag, ASTContext &Context,
-   const Stmt *Else, SourceLocation ElseLoc) {
+  const Stmt *Else, SourceLocation ElseLoc) {
   auto Remap = [&](SourceLocation Loc) {

Unrelated formatting change?



Comment at: 
clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:189
+static bool hasPreprocessorBranchEndBetweenLocations(
+ElseAfterReturnCheck::ConditionalBranchMap ConditionalBranchMap,
+const SourceManager &SM, SourceLocation StartLoc, SourceLocation EndLoc) {

Should the map be passed by const ref to avoid copies?



Comment at: 
clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:203
+
+  auto &ConditionalBranches =
+  ConditionalBranchMap[SM.getFileID(ExpandedEndLoc)];

Should this be `const auto &` to be clear there are no modifications to the map?



Comment at: 
clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:214
+
+  // First conditional block that ends after ExpandedStartLoc
+  const auto *Begin =

Missing a full stop at the end of the comment.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp:267
+
+  // These cases, Same as above but with an #else block
+  if (true) {

Same -> same
Also, missing a full stop.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp:285
+
+// ensure it can handle macros
+#define RETURN return

ensure -> Ensure
Also, missing a full stop.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp:304
+
+  // Whole statement is in a conditional block so diagnost as normal.
+#if 1

diagnost -> diagnose



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp:313
+#endif
+}

We should probably add some tests for more pathological cases, like:
```
#if 1
if (true) {
  return;
#else
{
#endif
} else {
  return;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91485

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

[PATCH] D91311: Add new 'preferred_name' attribute.

2020-11-17 Thread Louis Dionne via Phabricator via cfe-commits

ldionne added a comment.

In D91311#2398526 , @rsmith wrote:

> [...]

Thanks for your detailed explanation. I did not understand the philosophy of 
the attribute, and it's now clear to me that it shouldn't be tied to the 
typedef, indeed.

> There's another design approach we could follow, that would keep the 
> attribute on the template but avoid the awkwardness of requiring the typedef 
> to appear first: we could completely divorce this feature from typedefs. 
> Instead, we could say the attribute is of the form `preferred_name(type, 
> "name")`, where `type` is a specialization of the type to which the attribute 
> is attached, and `"name"` is a name used in diagnostics when referring to 
> that type, which is printed as if it's a member of the enclosing namespace 
> (but there'd be no enforcement that such a member actually exists and 
> declares the corresponding type). What do you think?

I think this is pretty interesting, actually. While it does seem a bit more 
loosey-goosey in terms of the type system, I can imagine useful applications of 
being able to use arbitrary strings, such as providing better diagnostics for 
specific types without having to actually have an associated typedef. Just to 
clarify, that would look like this, right?

  template
  class [[preferred_name(basic_string_view, "string_view")]]
[[preferred_name(basic_string_view, "u8string_view")]]
[[preferred_name(basic_string_view, "u16string_view")]]
[[preferred_name(basic_string_view, "u32string_view")]]
[[preferred_name(basic_string_view, "wstring_view")]]
  basic_string_view {
  ...
  };

I don't want to ask for something unreasonable.. but I think that's a neat 
design that's actually more powerful than the current one. I would be curious 
to hear what other people think as well, to see whether I'm the only one 
preferring the more free-form approach.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91311

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

[PATCH] D91610: [clangd] Add OverriddenBy Relation to index.

2020-11-17 Thread Haojian Wu via Phabricator via cfe-commits

hokein accepted this revision.
hokein added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/index/Relation.h:31
+/// { Subject = A, Predicate = BaseOf, Object = B }.
+///   - "A::Foo is overriden by B::Foo" is represented as
+/// { Subject = A::Foo, Predicate = OverriddenBy, Object = B::Foo }.

nit: I would explicitly indicate the base hierarchy here, maybe `Derived::Foo() 
overrides Base::Foo()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91610

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

[PATCH] D90982: Ignore implicit nodes in IgnoreUnlessSpelledInSource mode

2020-11-17 Thread Leonard Chan via Phabricator via cfe-commits

leonardchan added a comment.

Hi. I suspect it might be this patch or D90984 
 that might be leading to the test failure 
we're seeing on our arm64 builders

  FAIL: Clang-Unit :: 
AST/./ASTTests/Traverse.IgnoreUnlessSpelledInSourceImplicit (15871 of 26886)
   TEST 'Clang-Unit :: 
AST/./ASTTests/Traverse.IgnoreUnlessSpelledInSourceImplicit' FAILED 

  Note: Google Test filter = Traverse.IgnoreUnlessSpelledInSourceImplicit
  [==] Running 1 test from 1 test case.
  [--] Global test environment set-up.
  [--] 1 test from Traverse
  [ RUN  ] Traverse.IgnoreUnlessSpelledInSourceImplicit
  clang/unittests/AST/ASTTraverserTest.cpp:1114: Failure
Expected: dumpASTString(TK_AsIs, TUDecl)
Which is: "\nTranslationUnitDecl\n|-TypedefDecl '__int128_t'\n| ...

Would you mind taking a look? Thanks.

Builder link: 
https://luci-milo.appspot.com/p/fuchsia/builders/ci/clang-linux-arm64/b8863364626069720736


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90982

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

[PATCH] D91585: [NPM] Move more O0 pass building into PassBuilder

2020-11-17 Thread Teresa Johnson via Phabricator via cfe-commits

tejohnson added a comment.

Thanks, generally this seems to be good cleanup. Question on one of the changes 
below though.




Comment at: llvm/lib/Passes/PassBuilder.cpp:2365
+  // Don't do anything for (thin)lto backend compiles at O0.
+  if (Matches[1] != "thinlto" && Matches[1] != "lto")
+MPM.addPass(buildO0DefaultPipeline(L, Matches[1] != "default"));

This seems to change behavior. For one, previously we were only suppressing 
adding the PGO Instr passes for ThinLTO. Now this will suppress adding the 
coroutines passes and whatever else runRegisteredEPCallbacks was doing. Also, 
it's now doing the same for regular LTO but it didn't seem to do any special 
handling in that case before. Are these changes intended?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91585

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

[PATCH] D91630: [Parse] Add parsing support for C++ attributes on using-declarations

2020-11-17 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

rsmith added a comment.

Do `[[deprecated]]` and `[[maybe_unused]]` now work for //using-declarator//s? 
If so, a test for that would be useful.




Comment at: clang/lib/Parse/ParseDeclCXX.cpp:714
 
-  // C++11 attributes are not allowed on a using-declaration, but GNU ones
-  // are.
   ProhibitAttributes(MisplacedAttrs);
+  DiagnoseCXX11AttributeExtension(PrefixAttrs);

Should we suggest moving the attributes after the identifier in this case (even 
though that will leave the program ill-formed), as we do for alias-declarations?



Comment at: clang/lib/Parse/ParseDeclCXX.cpp:737
+DiagnoseCXX11AttributeExtension(Attrs);
+Attrs.addAll(PrefixAttrs.begin(), PrefixAttrs.end());
 

Should the prefix attributes go before the suffix attributes? (I could imagine 
there might be attributes for which order matters.) What do we do in the 
simple-declaration case?


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

https://reviews.llvm.org/D91630

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

[PATCH] D91162: Give up on evaluating value-dependent potential constexpr before hitting the assertion.

2020-11-17 Thread Haojian Wu via Phabricator via cfe-commits

hokein added a comment.

oops, this reminds me of the patch https://reviews.llvm.org/D84637 (I should 
have landed it, sorry), that patch should fix a general recovery-expr crash 
inside constexpr function body. I think the crash test should be fixed by that 
(let me check tomorrow).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91162

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

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

2020-11-17 Thread Mircea Trofin via Phabricator via cfe-commits

mtrofin added a comment.

In D91567#2400207 , @aeubanks wrote:

> What about removing the existing AlwaysInlinerPass and replacing it with this 
> one? Or is that something you were planning to do in a follow-up change?

That's the plan, yes.

>> open the opportunity to help the full inliner by performing additional 
>> function passes after the mandatory inlinings, but before the full inliner
>
> This change doesn't run the function simplification pipeline between the 
> mandatory and full inliner though, only
>
>   if (AttributorRun & AttributorRunOption::CGSCC)
> MainCGPipeline.addPass(AttributorCGSCCPass());
>   
>   if (PTO.Coroutines)
> MainCGPipeline.addPass(CoroSplitPass(Level != OptimizationLevel::O0));
>   
>   // Now deduce any function attributes based in the current code.
>   MainCGPipeline.addPass(PostOrderFunctionAttrsPass());

Right - my point was that we could more easily explore doing so by having this 
new AlwaysInliner separate. In this patch I included those additional passes 
because I thought they may be necessary or beneficial, but I can remove them as 
a first step if they are not necessary. @jdoerfert - should the Attributor be 
run post-always inlining, or is it fine to not be run?

> And is there any evidence that running the function simplification pipeline 
> between the mandatory and full inliner is helpful? It could affect compile 
> times.

In the ML-driven -Oz case, we saw some marginal improvement. I haven't in -O3 
cases using the "non-ml" inliner. I suspect that it helps in the ML policy case 
(both -Oz and -O3, on which we're currently working), because: 1) the current 
-Oz takes some global (module-wide) features, so probably simplifying out the 
trivial cases helps; and 2) we plan on taking into consideration regions of the 
call graph, and the intuition is that eliminating the trivial cases (mandatory 
cases) would rise the visibility (for a training algorithm) of the non-trivial 
cases.

> I'd think that adding the mandatory inliner right before the full inliner in 
> the same CGSCC pass manager would do the job. e.g. add it in 
> `ModuleInlinerWrapperPass::ModuleInlinerWrapperPass()` right before 
> `PM.addPass(InlinerPass());`

It would, and what I'm proposing here is equivalent to that, but the proposal 
here helps with these other explorations, with (arguably) not much of a  
difference cost-wise in itself (meaning, of course, if we discover there's 
benefit in running those additional passes, we pay with compile time, but in of 
itself, factoring the always inliner in its own wrapper, or in the same wrapper 
as the inliner, doesn't really come at much of a cost).

Now, if we determine there is no value, we can bring it back easily - wdyt?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91567

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

[PATCH] D91656: [clang-tidy] add concurrent module

2020-11-17 Thread Vasily Kulikov via Phabricator via cfe-commits

segoon created this revision.
segoon added a reviewer: njames93.
Herald added subscribers: cfe-commits, lxfind, arphaman, modocache, xazax.hun, 
mgorny.
Herald added a project: clang.
segoon requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added a subscriber: sstefan1.

The module will contain checks related to concurrent programming (including 
threads, fibers, coroutines, etc.).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91656

Files:
  clang-tools-extra/clang-tidy/CMakeLists.txt
  clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
  clang-tools-extra/clang-tidy/concurrent/CMakeLists.txt
  clang-tools-extra/clang-tidy/concurrent/ConcurrentTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/index.rst

Index: clang-tools-extra/docs/clang-tidy/index.rst
===
--- clang-tools-extra/docs/clang-tidy/index.rst
+++ clang-tools-extra/docs/clang-tidy/index.rst
@@ -64,6 +64,8 @@
 ``bugprone-``  Checks that target bugprone code constructs.
 ``cert-``  Checks related to CERT Secure Coding Guidelines.
 ``clang-analyzer-``Clang Static Analyzer checks.
+``concurrent-``Checks related to concurrent programming (including
+   threads, fibers, coroutines, etc.).
 ``cppcoreguidelines-`` Checks related to C++ Core Guidelines.
 ``darwin-``Checks related to Darwin coding conventions.
 ``fuchsia-``   Checks related to Fuchsia coding conventions.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -82,6 +82,10 @@
   `Altera SDK for OpenCL: Best Practices Guide
   `_.
 
+- New ``concurrent`` module.
+
+  Includes checks related to concurrent programming (e.g. threads, fibers, coroutines, etc.).
+
 New checks
 ^^
 
Index: clang-tools-extra/clang-tidy/concurrent/ConcurrentTidyModule.cpp
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/concurrent/ConcurrentTidyModule.cpp
@@ -0,0 +1,33 @@
+//===--- ConcurrentTidyModule.cpp - clang-tidy ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "../ClangTidy.h"
+#include "../ClangTidyModule.h"
+#include "../ClangTidyModuleRegistry.h"
+
+namespace clang {
+namespace tidy {
+namespace concurrent {
+
+class ConcurrentModule : public ClangTidyModule {
+public:
+  void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {}
+};
+
+} // namespace concurrent
+
+// Register the ConcurrentTidyModule using this statically initialized variable.
+static ClangTidyModuleRegistry::Add
+X("concurrent-module", "Adds concurrent checks.");
+
+// This anchor is used to force the linker to link in the generated object file
+// and thus register the ConcurrentModule.
+volatile int ConcurrentModuleAnchorSource = 0;
+
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/clang-tidy/concurrent/CMakeLists.txt
===
--- /dev/null
+++ clang-tools-extra/clang-tidy/concurrent/CMakeLists.txt
@@ -0,0 +1,23 @@
+set(LLVM_LINK_COMPONENTS
+  FrontendOpenMP
+  Support
+  )
+
+add_clang_library(clangTidyConcurrentModule
+  ConcurrentTidyModule.cpp
+
+  LINK_LIBS
+  clangTidy
+  clangTidyUtils
+  )
+
+clang_target_link_libraries(clangTidyConcurrentModule
+  PRIVATE
+  clangAnalysis
+  clangAST
+  clangASTMatchers
+  clangBasic
+  clangLex
+  clangSerialization
+  clangTooling
+  )
Index: clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
===
--- clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
+++ clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
@@ -45,6 +45,11 @@
 static int LLVM_ATTRIBUTE_UNUSED CERTModuleAnchorDestination =
 CERTModuleAnchorSource;
 
+// This anchor is used to force the linker to link the ConcurrentModule.
+extern volatile int ConcurrentModuleAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED ConcurrentModuleAnchorDestination =
+ConcurrentModuleAnchorSource;
+
 // This anchor is used to force the linker to link the CppCoreGuidelinesModule.
 extern volatile int CppCoreGuidelinesModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED CppCoreGuidelinesModuleAnchorDestination =
Index: clang-tools-extra/clang-tidy/CMakeLists.txt
===
--- clang-tools-extra/clang-tidy/CMake

[PATCH] D91625: [clang] Do not crash on pointer wchar_t pointer assignment.

2020-11-17 Thread Haojian Wu via Phabricator via cfe-commits

hokein added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:8726
+else if (!lhptee->isWideCharType() &&
+ lhptee->hasSignedIntegerRepresentation())
   ltrans = S.Context.getCorrespondingUnsignedType(ltrans);

I'm wondering whether the fix is in 
`ASTContext::getCorrespondingUnsignedType()` 

if my reading of the standard is right:

- there exists a corresponding standard (or extended) unsigned integer type for 
each of standard (or extended) signed integer types, 
http://eel.is/c++draft/basic.fundamental#2
- wchar_t is an integer type, http://eel.is/c++draft/basic.fundamental#11, and 
has an implementation-defined signed or unsigned integer type as its underlying 
type, http://eel.is/c++draft/basic.fundamental#8

I think the rule also applies on `wchar_t`, the fix seems to handle `WChar_S` 
case (return `ASTContext::getUnsignedWCharType()`) in 
`ASTContext::getCorrespondingUnsignedType()`. What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91625

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

[PATCH] D91428: Add support for multiple program address spaces

2020-11-17 Thread Thomas Lively via Phabricator via cfe-commits

tlively added a comment.

@arsenm, Are you suggesting that we just relax the verification rules to allow 
calling function pointers to arbitrary address spaces without needing any 
changes to the data layout string? That seems fine to me, but the data layout 
string solution does allow targets to opt in to more precise validation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91428

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

[PATCH] D91428: Add support for multiple program address spaces

2020-11-17 Thread Matt Arsenault via Phabricator via cfe-commits

arsenm added a comment.

While the IR definitely should support mixing functions with different address 
spaces, I don't see the point of encoding multiple of these in the datalayout




Comment at: llvm/include/llvm/IR/DataLayout.h:125-129
+  /// Vector of address spaces that can contain code
+  /// Always non-empty, contains the default address space by default.
+  /// Harvard architectures can specify a different address space.
+  /// WebAssembly might specify more than one address space.
+  SmallVector ProgramAddrSpaces;

I don't see why we need a list of program address spaces. The datalayout only 
needs to know the one to use for situations when generic code needs to 
synthesize a function out of thin air. Anything beyond that would need target 
information in the specific pass.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91428

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

[PATCH] D91580: [Frontend] Add flag to allow PCM generation despite compiler errors

2020-11-17 Thread Argyrios Kyrtzidis via Phabricator via cfe-commits

akyrtzi added a comment.

I'd like if we only had to use one flag (`-fallow-pcm-with-compiler-errors`) 
and have it handle both modules and PCH. Could you make the flag also work for 
PCH and/or add a test that verifies it works?
You may only have to change

  Opts.AllowPCHWithCompilerErrors = Args.hasArg(OPT_fallow_pch_with_errors);

to

  Opts.AllowPCHWithCompilerErrors = Args.hasArg(OPT_fallow_pch_with_errors, 
OPT_fallow_pcm_with_errors);


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

https://reviews.llvm.org/D91580

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

1 2 3 >

1 - 100 of 250 matches

Mail list logo