[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

2021-12-13 Thread Alexander Yermolovich via Phabricator via cfe-commits 
ayermolo added a comment.

@modocache Just checking is this diff abandoned?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58920

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

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

2021-09-16 Thread Nathan Sidwell via Phabricator via cfe-commits 
urnathan added inline comments.



Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:3198-3199
 // lookups will find it.
 MergeDC->makeDeclVisibleInContextImpl(New, /*Internal*/true);
+if (isa(New) && Name.getAsString() == "std")
+  if (!Reader.getSema()->StdNamespace)

Don't we also have to check New's context is the global namespace?  What 
happens for something like 'namespace evil::std {};'?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58920

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

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

2019-11-04 Thread Brian Gesiak via Phabricator via cfe-commits 
modocache updated this revision to Diff 227808.
modocache added a comment.

Rebasing onto the monorepo. @rsmith, I confirmed the test cases that this diff 
adds still fail on trunk, and that the Clang source changes made in this diff 
fix the test case failures. Tomorrow I plan on poking around to see if I can 
reproduce similar issues in the C++20 modules implementation. But in the 
meantime, how do you feel about this patch? You suggested a change to 
`~FindExistingResult` but also that it'd be difficult to test/verify such a 
change. Is that change still something you're looking for before accepting this 
diff?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58920

Files:
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/test/Modules/Inputs/pr39287-2/a.h
  clang/test/Modules/Inputs/pr39287-2/module.modulemap
  clang/test/Modules/Inputs/pr39287/a.h
  clang/test/Modules/Inputs/pr39287/module.modulemap
  clang/test/Modules/pr39287-2.cpp
  clang/test/Modules/pr39287.cpp

Index: clang/test/Modules/pr39287.cpp
===
--- /dev/null
+++ clang/test/Modules/pr39287.cpp
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++17 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/pr39287 %s -verify
+
+class A {
+  virtual ~A() {}
+};
+
+#include "a.h"
+
+namespace std { class type_info; }
+
+void foo() {
+  typeid(foo); // expected-warning {{expression result unused}}
+}
Index: clang/test/Modules/pr39287-2.cpp
===
--- /dev/null
+++ clang/test/Modules/pr39287-2.cpp
@@ -0,0 +1,12 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++17 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/pr39287-2 %s -verify
+
+class A {
+  virtual ~A() {}
+};
+
+#include "a.h"
+
+void foo() {
+  typeid(foo); // expected-warning {{expression result unused}}
+}
Index: clang/test/Modules/Inputs/pr39287/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/pr39287/module.modulemap
@@ -0,0 +1,3 @@
+module "a.h" {
+  header "a.h"
+}
Index: clang/test/Modules/Inputs/pr39287/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/pr39287/a.h
@@ -0,0 +1 @@
+namespace std {}
Index: clang/test/Modules/Inputs/pr39287-2/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/pr39287-2/module.modulemap
@@ -0,0 +1,3 @@
+module "a.h" {
+  header "a.h"
+}
Index: clang/test/Modules/Inputs/pr39287-2/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/pr39287-2/a.h
@@ -0,0 +1 @@
+namespace std { class type_info; }
Index: clang/lib/Serialization/ASTReaderDecl.cpp
===
--- clang/lib/Serialization/ASTReaderDecl.cpp
+++ clang/lib/Serialization/ASTReaderDecl.cpp
@@ -47,6 +47,7 @@
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/Specifiers.h"
 #include "clang/Sema/IdentifierResolver.h"
+#include "clang/Sema/Sema.h"
 #include "clang/Serialization/ASTBitCodes.h"
 #include "clang/Serialization/ASTReader.h"
 #include "clang/Serialization/ContinuousRangeMap.h"
@@ -3195,6 +3196,9 @@
 // Add the declaration to its redeclaration context so later merging
 // lookups will find it.
 MergeDC->makeDeclVisibleInContextImpl(New, /*Internal*/true);
+if (isa(New) && Name.getAsString() == "std")
+  if (!Reader.getSema()->StdNamespace)
+Reader.getSema()->StdNamespace = New;
   }
 }
 
@@ -3358,6 +3362,14 @@
   return FindExistingResult(Reader, D, Existing, AnonymousDeclNumber,
 TypedefNameForLinkage);
 }
+if (isa(D) && D->getName() == "std") {
+  auto StdPtr = Reader.getSema()->StdNamespace;
+  if (StdPtr.isValid() && !StdPtr.isOffset())
+if (auto *Std = cast_or_null(StdPtr.get(nullptr)))
+  if (isSameEntity(Std, D))
+return FindExistingResult(Reader, D, Std, AnonymousDeclNumber,
+  TypedefNameForLinkage);
+}
   } else {
 // Not in a mergeable context.
 return FindExistingResult(Reader);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

2019-05-28 Thread Brian Gesiak via Phabricator via cfe-commits 
modocache added a comment.

@rsmith, what do you think of the patch as-is?


Repository:
  rC Clang

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

https://reviews.llvm.org/D58920



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

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

2019-05-21 Thread Brian Gesiak via Phabricator via cfe-commits 
modocache updated this revision to Diff 200518.
modocache added a comment.

Hmm... alright, I'm not really sure how I could implement a test that fails 
without this, but I added a check in the FindExistingResult destructor.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58920

Files:
  lib/Serialization/ASTReaderDecl.cpp
  test/Modules/Inputs/mod-virtual-destructor-bug-two/a.h
  test/Modules/Inputs/mod-virtual-destructor-bug-two/module.modulemap
  test/Modules/Inputs/mod-virtual-destructor-bug/a.h
  test/Modules/Inputs/mod-virtual-destructor-bug/module.modulemap
  test/Modules/mod-virtual-destructor-bug-two.cpp
  test/Modules/mod-virtual-destructor-bug.cpp

Index: test/Modules/mod-virtual-destructor-bug.cpp
===
--- /dev/null
+++ test/Modules/mod-virtual-destructor-bug.cpp
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++17 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/mod-virtual-destructor-bug %s -verify
+
+class A {
+  virtual ~A() {}
+};
+
+#include "a.h"
+
+namespace std { class type_info; }
+
+void foo() {
+  typeid(foo); // expected-warning {{expression result unused}}
+}
Index: test/Modules/mod-virtual-destructor-bug-two.cpp
===
--- /dev/null
+++ test/Modules/mod-virtual-destructor-bug-two.cpp
@@ -0,0 +1,12 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++17 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/mod-virtual-destructor-bug-two %s -verify
+
+class A {
+  virtual ~A() {}
+};
+
+#include "a.h"
+
+void foo() {
+  typeid(foo); // expected-warning {{expression result unused}}
+}
Index: test/Modules/Inputs/mod-virtual-destructor-bug/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/mod-virtual-destructor-bug/module.modulemap
@@ -0,0 +1,3 @@
+module "a.h" {
+  header "a.h"
+}
Index: test/Modules/Inputs/mod-virtual-destructor-bug/a.h
===
--- /dev/null
+++ test/Modules/Inputs/mod-virtual-destructor-bug/a.h
@@ -0,0 +1 @@
+namespace std {}
Index: test/Modules/Inputs/mod-virtual-destructor-bug-two/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/mod-virtual-destructor-bug-two/module.modulemap
@@ -0,0 +1,3 @@
+module "a.h" {
+  header "a.h"
+}
Index: test/Modules/Inputs/mod-virtual-destructor-bug-two/a.h
===
--- /dev/null
+++ test/Modules/Inputs/mod-virtual-destructor-bug-two/a.h
@@ -0,0 +1 @@
+namespace std { class type_info; }
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -47,6 +47,7 @@
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/Specifiers.h"
 #include "clang/Sema/IdentifierResolver.h"
+#include "clang/Sema/Sema.h"
 #include "clang/Serialization/ASTBitCodes.h"
 #include "clang/Serialization/ASTReader.h"
 #include "clang/Serialization/ContinuousRangeMap.h"
@@ -3252,6 +3253,9 @@
 // Add the declaration to its redeclaration context so later merging
 // lookups will find it.
 MergeDC->makeDeclVisibleInContextImpl(New, /*Internal*/true);
+if (isa(New) && Name.getAsString() == "std")
+  if (!Reader.getSema()->StdNamespace)
+Reader.getSema()->StdNamespace = New;
   }
 }
 
@@ -3415,6 +3419,14 @@
   return FindExistingResult(Reader, D, Existing, AnonymousDeclNumber,
 TypedefNameForLinkage);
 }
+if (isa(D) && D->getName() == "std") {
+  auto StdPtr = Reader.getSema()->StdNamespace;
+  if (StdPtr.isValid() && !StdPtr.isOffset())
+if (auto *Std = cast_or_null(StdPtr.get(nullptr)))
+  if (isSameEntity(Std, D))
+return FindExistingResult(Reader, D, Std, AnonymousDeclNumber,
+  TypedefNameForLinkage);
+}
   } else {
 // Not in a mergeable context.
 return FindExistingResult(Reader);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

2019-05-16 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added a comment.

In D58920#1503872 , @modocache wrote:

> Thanks for the help, @rsmith! Your suggestions were spot-on. (It took me a 
> little while to figure out why, even using the `LazyDeclPtr` directly, I was 
> still triggering deserialization. It turns out `dump()` causes 
> deserialization too -- whoops!)
>
> > You should also change `FindExistingResult::~FindExistingResult` to update 
> > `Sema::StdNamespace` to point to your newly-deserialized namespace if you 
> > didn't find a prior declaration of it, so that `Sema::getStdNamespace()` 
> > finds the deserialized namespace.
>
> I haven't done this yet. I'm trying to think of a test case that would fail 
> if this were not done properly -- or would there not be one?


I think the failure case is a little complex to set up. I think you need:

- two modules each of which contains an invisible (implicitly-declared) `std` 
namespace
- arrange for `Sema::StdNamespace` to point to the ID of one of them
- trigger the import of the other one
- perform a name lookup for `std` to erase the placeholder `std` lookup result 
from the translation unit's lookup table
- trigger the import of the first `std` (eg, by triggering the declaration of 
`operator new`)

At that point, there is nowhere for the newly-imported `std` namespace to find 
the old one: it's not in the translation unit's name lookup table, and it's not 
in `Sema::StdNamespace`, so we would presumably end up with two `std` 
namespaces not connected to one another.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58920



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

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

2019-05-15 Thread Brian Gesiak via Phabricator via cfe-commits 
modocache updated this revision to Diff 199709.
modocache added a comment.

Oops, sent the patch from the wrong repository.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58920

Files:
  lib/Serialization/ASTReaderDecl.cpp
  test/Modules/Inputs/mod-virtual-destructor-bug-two/a.h
  test/Modules/Inputs/mod-virtual-destructor-bug-two/module.modulemap
  test/Modules/Inputs/mod-virtual-destructor-bug/a.h
  test/Modules/Inputs/mod-virtual-destructor-bug/module.modulemap
  test/Modules/mod-virtual-destructor-bug-two.cpp
  test/Modules/mod-virtual-destructor-bug.cpp


Index: test/Modules/mod-virtual-destructor-bug.cpp
===
--- /dev/null
+++ test/Modules/mod-virtual-destructor-bug.cpp
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++17 -fmodules -fimplicit-module-maps 
-fmodules-cache-path=%t -I %S/Inputs/mod-virtual-destructor-bug %s -verify
+
+class A {
+  virtual ~A() {}
+};
+
+#include "a.h"
+
+namespace std { class type_info; }
+
+void foo() {
+  typeid(foo); // expected-warning {{expression result unused}}
+}
Index: test/Modules/mod-virtual-destructor-bug-two.cpp
===
--- /dev/null
+++ test/Modules/mod-virtual-destructor-bug-two.cpp
@@ -0,0 +1,12 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++17 -fmodules -fimplicit-module-maps 
-fmodules-cache-path=%t -I %S/Inputs/mod-virtual-destructor-bug-two %s -verify
+
+class A {
+  virtual ~A() {}
+};
+
+#include "a.h"
+
+void foo() {
+  typeid(foo); // expected-warning {{expression result unused}}
+}
Index: test/Modules/Inputs/mod-virtual-destructor-bug/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/mod-virtual-destructor-bug/module.modulemap
@@ -0,0 +1,3 @@
+module "a.h" {
+  header "a.h"
+}
Index: test/Modules/Inputs/mod-virtual-destructor-bug/a.h
===
--- /dev/null
+++ test/Modules/Inputs/mod-virtual-destructor-bug/a.h
@@ -0,0 +1 @@
+namespace std {}
Index: test/Modules/Inputs/mod-virtual-destructor-bug-two/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/mod-virtual-destructor-bug-two/module.modulemap
@@ -0,0 +1,3 @@
+module "a.h" {
+  header "a.h"
+}
Index: test/Modules/Inputs/mod-virtual-destructor-bug-two/a.h
===
--- /dev/null
+++ test/Modules/Inputs/mod-virtual-destructor-bug-two/a.h
@@ -0,0 +1 @@
+namespace std { class type_info; }
Index: lib/Serialization/ASTReaderDecl.cpp
===
--- lib/Serialization/ASTReaderDecl.cpp
+++ lib/Serialization/ASTReaderDecl.cpp
@@ -47,6 +47,7 @@
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/Specifiers.h"
 #include "clang/Sema/IdentifierResolver.h"
+#include "clang/Sema/Sema.h"
 #include "clang/Serialization/ASTBitCodes.h"
 #include "clang/Serialization/ASTReader.h"
 #include "clang/Serialization/ContinuousRangeMap.h"
@@ -3401,6 +3402,14 @@
   return FindExistingResult(Reader, D, Existing, AnonymousDeclNumber,
 TypedefNameForLinkage);
 }
+if (isa(D) && D->getName() == "std") {
+  auto StdPtr = Reader.getSema()->StdNamespace;
+  if (StdPtr.isValid() && !StdPtr.isOffset())
+if (auto *Std = cast_or_null(StdPtr.get(nullptr)))
+  if (isSameEntity(Std, D))
+return FindExistingResult(Reader, D, Std, AnonymousDeclNumber,
+  TypedefNameForLinkage);
+}
   } else {
 // Not in a mergeable context.
 return FindExistingResult(Reader);


Index: test/Modules/mod-virtual-destructor-bug.cpp
===
--- /dev/null
+++ test/Modules/mod-virtual-destructor-bug.cpp
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++17 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/mod-virtual-destructor-bug %s -verify
+
+class A {
+  virtual ~A() {}
+};
+
+#include "a.h"
+
+namespace std { class type_info; }
+
+void foo() {
+  typeid(foo); // expected-warning {{expression result unused}}
+}
Index: test/Modules/mod-virtual-destructor-bug-two.cpp
===
--- /dev/null
+++ test/Modules/mod-virtual-destructor-bug-two.cpp
@@ -0,0 +1,12 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++17 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/mod-virtual-destructor-bug-two %s -verify
+
+class A {
+  virtual ~A() {}
+};
+
+#include "a.h"
+
+void foo() {
+  typeid(foo); // expected-warning {{expression result unused}}
+}
Index: test/Modules/Inputs/mod-virtual-destructor-bug/module.modulemap

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

2019-05-15 Thread Brian Gesiak via Phabricator via cfe-commits 
modocache updated this revision to Diff 199708.
modocache added a comment.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Thanks for the help, @rsmith! Your suggestions were spot-on. (It took me a 
little while to figure out why, even using the `LazyDeclPtr` directly, I was 
still triggering deserialization. It turns out `dump()` causes deserialization 
too -- whoops!)

> You should also change `FindExistingResult::~FindExistingResult` to update 
> `Sema::StdNamespace` to point to your newly-deserialized namespace if you 
> didn't find a prior declaration of it, so that `Sema::getStdNamespace()` 
> finds the deserialized namespace.

I haven't done this yet. I'm trying to think of a test case that would fail if 
this were not done properly -- or would there not be one?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58920

Files:
  lib/Target/X86/X86ISelLowering.cpp
  test/CodeGen/X86/vector-reduce-mul-widen.ll
  test/CodeGen/X86/vector-reduce-mul.ll

Index: test/CodeGen/X86/vector-reduce-mul.ll
===
--- test/CodeGen/X86/vector-reduce-mul.ll
+++ test/CodeGen/X86/vector-reduce-mul.ll
@@ -2465,36 +2465,37 @@
 ; AVX512BW-NEXT:vpandq %zmm3, %zmm0, %zmm0
 ; AVX512BW-NEXT:vpackuswb %zmm2, %zmm0, %zmm0
 ; AVX512BW-NEXT:vextracti128 $1, %ymm0, %xmm1
-; AVX512BW-NEXT:vpunpckhbw {{.*#+}} zmm2 = zmm0[8,8,9,9,10,10,11,11,12,12,13,13,14,14,15,15,24,24,25,25,26,26,27,27,28,28,29,29,30,30,31,31,40,40,41,41,42,42,43,43,44,44,45,45,46,46,47,47,56,56,57,57,58,58,59,59,60,60,61,61,62,62,63,63]
-; AVX512BW-NEXT:vpunpckhbw {{.*#+}} zmm4 = zmm1[8],zmm0[8],zmm1[9],zmm0[9],zmm1[10],zmm0[10],zmm1[11],zmm0[11],zmm1[12],zmm0[12],zmm1[13],zmm0[13],zmm1[14],zmm0[14],zmm1[15],zmm0[15],zmm1[24],zmm0[24],zmm1[25],zmm0[25],zmm1[26],zmm0[26],zmm1[27],zmm0[27],zmm1[28],zmm0[28],zmm1[29],zmm0[29],zmm1[30],zmm0[30],zmm1[31],zmm0[31],zmm1[40],zmm0[40],zmm1[41],zmm0[41],zmm1[42],zmm0[42],zmm1[43],zmm0[43],zmm1[44],zmm0[44],zmm1[45],zmm0[45],zmm1[46],zmm0[46],zmm1[47],zmm0[47],zmm1[56],zmm0[56],zmm1[57],zmm0[57],zmm1[58],zmm0[58],zmm1[59],zmm0[59],zmm1[60],zmm0[60],zmm1[61],zmm0[61],zmm1[62],zmm0[62],zmm1[63],zmm0[63]
-; AVX512BW-NEXT:vpmullw %zmm4, %zmm2, %zmm2
-; AVX512BW-NEXT:vpandq %zmm3, %zmm2, %zmm2
-; AVX512BW-NEXT:vpunpcklbw {{.*#+}} zmm0 = zmm0[0,0,1,1,2,2,3,3,4,4,5,5,6,6,7,7,16,16,17,17,18,18,19,19,20,20,21,21,22,22,23,23,32,32,33,33,34,34,35,35,36,36,37,37,38,38,39,39,48,48,49,49,50,50,51,51,52,52,53,53,54,54,55,55]
-; AVX512BW-NEXT:vpunpcklbw {{.*#+}} zmm1 = zmm1[0],zmm0[0],zmm1[1],zmm0[1],zmm1[2],zmm0[2],zmm1[3],zmm0[3],zmm1[4],zmm0[4],zmm1[5],zmm0[5],zmm1[6],zmm0[6],zmm1[7],zmm0[7],zmm1[16],zmm0[16],zmm1[17],zmm0[17],zmm1[18],zmm0[18],zmm1[19],zmm0[19],zmm1[20],zmm0[20],zmm1[21],zmm0[21],zmm1[22],zmm0[22],zmm1[23],zmm0[23],zmm1[32],zmm0[32],zmm1[33],zmm0[33],zmm1[34],zmm0[34],zmm1[35],zmm0[35],zmm1[36],zmm0[36],zmm1[37],zmm0[37],zmm1[38],zmm0[38],zmm1[39],zmm0[39],zmm1[48],zmm0[48],zmm1[49],zmm0[49],zmm1[50],zmm0[50],zmm1[51],zmm0[51],zmm1[52],zmm0[52],zmm1[53],zmm0[53],zmm1[54],zmm0[54],zmm1[55],zmm0[55]
-; AVX512BW-NEXT:vpmullw %zmm1, %zmm0, %zmm0
-; AVX512BW-NEXT:vpandq %zmm3, %zmm0, %zmm0
-; AVX512BW-NEXT:vpackuswb %zmm2, %zmm0, %zmm0
-; AVX512BW-NEXT:vpunpckhbw {{.*#+}} zmm1 = zmm0[8,8,9,9,10,10,11,11,12,12,13,13,14,14,15,15,24,24,25,25,26,26,27,27,28,28,29,29,30,30,31,31,40,40,41,41,42,42,43,43,44,44,45,45,46,46,47,47,56,56,57,57,58,58,59,59,60,60,61,61,62,62,63,63]
-; AVX512BW-NEXT:vpunpcklbw {{.*#+}} zmm0 = zmm0[0,0,1,1,2,2,3,3,4,4,5,5,6,6,7,7,16,16,17,17,18,18,19,19,20,20,21,21,22,22,23,23,32,32,33,33,34,34,35,35,36,36,37,37,38,38,39,39,48,48,49,49,50,50,51,51,52,52,53,53,54,54,55,55]
-; AVX512BW-NEXT:vpmullw %zmm1, %zmm0, %zmm0
-; AVX512BW-NEXT:vpandq %zmm3, %zmm0, %zmm0
+; AVX512BW-NEXT:vpunpckhbw {{.*#+}} xmm2 = xmm0[8,8,9,9,10,10,11,11,12,12,13,13,14,14,15,15]
+; AVX512BW-NEXT:vpunpckhbw {{.*#+}} xmm3 = xmm1[8],xmm0[8],xmm1[9],xmm0[9],xmm1[10],xmm0[10],xmm1[11],xmm0[11],xmm1[12],xmm0[12],xmm1[13],xmm0[13],xmm1[14],xmm0[14],xmm1[15],xmm0[15]
+; AVX512BW-NEXT:vpmullw %xmm3, %xmm2, %xmm2
+; AVX512BW-NEXT:vmovdqa {{.*#+}} xmm3 = [255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255,255]
+; AVX512BW-NEXT:vpand %xmm3, %xmm2, %xmm2
+; AVX512BW-NEXT:vpmovzxbw {{.*#+}} xmm0 = xmm0[0],zero,xmm0[1],zero,xmm0[2],zero,xmm0[3],zero,xmm0[4],zero,xmm0[5],zero,xmm0[6],zero,xmm0[7],zero
+; AVX512BW-NEXT:vpmovzxbw {{.*#+}} xmm1 = xmm1[0],zero,xmm1[1],zero,xmm1[2],zero,xmm1[3],zero,xmm1[4],zero,xmm1[5],zero,xmm1[6],zero,xmm1[7],zero
+; AVX512BW-NEXT:vpmullw %xmm1, %xmm0, %xmm0
+; AVX512BW-NEXT:vpand %xmm3, %xmm0, %xmm0
+; AVX512BW-NEXT:vpackuswb %xmm2, %xmm0, %xmm0
+; AVX512BW-NEXT:vpunpckhbw {{.*#+}} xmm1 =

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

2019-05-13 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added a comment.

The problem may well be that you're recursively triggering deserialization of 
the `std` namespace from its own deserialization.

In D58920#1500246 , @modocache wrote:

>   @@ -3401,6 +3402,22 @@ ASTDeclReader::FindExistingResult 
> ASTDeclReader::findExisting(NamedDecl *D) {
>  return FindExistingResult(Reader, D, Existing, 
> AnonymousDeclNumber,
>TypedefNameForLinkage);
>}
>   +if (isa(D) && D->getName() == "std") {
>   +  llvm::outs() << "Found std namespace: ";
>   +  D->dump();
>   +  llvm::outs() << "Merging into Sema std namespace: ";
>   +  Sema *S = Reader.getSema();
>   +  S->getStdNamespace()->dump();
>


You shouldn't call `getStdNamespace()` here, because that will trigger 
deserialization. You should query the `LazyDeclPtr` directly to see if there's 
an already-deserialized `std` namespace.

>   +  NamedDecl *Existing =
>   +  getDeclForMerging(S->getStdNamespace(), TypedefNameForLinkage);

(No need to call this, `std` isn't an anonymous struct.)

>   +  llvm::outs() << "Found Existing: ";
>   +  Existing->dump();
>   +  if (isSameEntity(Existing, D)) {
>   +llvm::outs() << "isSameEntity is true\n";
>   +return FindExistingResult(Reader, D, Existing, AnonymousDeclNumber,
>   +  TypedefNameForLinkage);
>   +  }
>   +}
>  } else {
>// Not in a mergeable context.
>return FindExistingResult(Reader);

You should also change `FindExistingResult::~FindExistingResult` to update 
`Sema::StdNamespace` to point to your newly-deserialized namespace if you 
didn't find a prior declaration of it, so that `Sema::getStdNamespace()` finds 
the deserialized namespace.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58920



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

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

2019-05-13 Thread Brian Gesiak via Phabricator via cfe-commits 
modocache added a comment.

Thanks @rsmith for the guidance here! I appreciate it very much. One snag I ran 
into after following your suggestion, though, is that when I modify 
`ASTDeclReader::findExisting` to return Sema's existing implicit std namespace, 
I run into an assertion later on, when the decl chain is being marked as 
incomplete. That is, the following patch (debug output included):

  diff --git a/lib/Serialization/ASTReaderDecl.cpp 
b/lib/Serialization/ASTReaderDecl.cpp
  index 32bd82d077..9d447952e1 100644
  --- a/lib/Serialization/ASTReaderDecl.cpp
  +++ b/lib/Serialization/ASTReaderDecl.cpp
  @@ -47,6 +47,7 @@
   #include "clang/Basic/SourceLocation.h"
   #include "clang/Basic/Specifiers.h"
   #include "clang/Sema/IdentifierResolver.h"
  +#include "clang/Sema/Sema.h"
   #include "clang/Serialization/ASTBitCodes.h"
   #include "clang/Serialization/ASTReader.h"
   #include "clang/Serialization/ContinuousRangeMap.h"
  @@ -3401,6 +3402,22 @@ ASTDeclReader::FindExistingResult 
ASTDeclReader::findExisting(NamedDecl *D) {
 return FindExistingResult(Reader, D, Existing, AnonymousDeclNumber,
   TypedefNameForLinkage);
   }
  +if (isa(D) && D->getName() == "std") {
  +  llvm::outs() << "Found std namespace: ";
  +  D->dump();
  +  llvm::outs() << "Merging into Sema std namespace: ";
  +  Sema *S = Reader.getSema();
  +  S->getStdNamespace()->dump();
  +  NamedDecl *Existing =
  +  getDeclForMerging(S->getStdNamespace(), TypedefNameForLinkage);
  +  llvm::outs() << "Found Existing: ";
  +  Existing->dump();
  +  if (isSameEntity(Existing, D)) {
  +llvm::outs() << "isSameEntity is true\n";
  +return FindExistingResult(Reader, D, Existing, AnonymousDeclNumber,
  +  TypedefNameForLinkage);
  +  }
  +}
 } else {
   // Not in a mergeable context.
   return FindExistingResult(Reader);

Results in the expected output, along with an unexpected assert:

  Found std namespace: NamespaceDecl 0x55a391530910 
 col:11 imported in a.h std
  Merging into Sema std namespace: NamespaceDecl 0x55a391501ec8 <>  implicit std
  Found Existing: NamespaceDecl 0x55a391501ec8 <>  
implicit std
  isSameEntity is true
  clang-9: ../include/llvm/ADT/PointerUnion.h:135: T llvm::PointerUnion::get() const [with T = clang::LazyGenerationalUpdatePtr; 
PT1 = llvm::PointerUnion; PT2 = 
clang::LazyGenerationalUpdatePtr]: Assertion `is() && 
"Invalid accessor called"' failed.
  Stack dump:
  0.  Program arguments: 
/home/modocache/Source/llvm/git/dev/llvm/build/bin/clang-9 -cc1 -triple 
x86_64-unknown-linux-gnu -emit-obj -mrelax-all -disable-free -main-file-name 
mod-virtual-destructor-bug.cpp -mrelocation-model static -mthread-model posix 
-mdisable-fp-elim -fmath-errno -masm-verbose -mconstructor-aliases 
-munwind-tables -fuse-init-array -target-cpu x86-64 -dwarf-column-info 
-debugger-tuning=gdb -resource-dir 
/home/modocache/Source/llvm/git/dev/llvm/build/lib/clang/9.0.0 -I 
mod-virtual-destructor-bug -internal-isystem 
/usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8 -internal-isystem 
/usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/x86_64-linux-gnu/c++/8 
-internal-isystem 
/usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/x86_64-linux-gnu/c++/8 
-internal-isystem 
/usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/backward 
-internal-isystem /usr/local/include -internal-isystem 
/home/modocache/Source/llvm/git/dev/llvm/build/lib/clang/9.0.0/include 
-internal-externc-isystem /usr/include/x86_64-linux-gnu 
-internal-externc-isystem /include -internal-externc-isystem /usr/include 
-std=c++17 -fdeprecated-macro -fdebug-compilation-dir 
/home/modocache/Source/tmp/mod -ferror-limit 19 -fmessage-length 195 -fmodules 
-fimplicit-module-maps -fmodules-cache-path=mod-virtual-destructor-cache 
-fmodules-validate-system-headers -fobjc-runtime=gcc -fcxx-exceptions 
-fexceptions -fdiagnostics-show-option -fcolor-diagnostics -o 
/tmp/mod-virtual-destructor-bug-0da92f.o -x c++ mod-virtual-destructor-bug.cpp 
-faddrsig
  1.  mod-virtual-destructor-bug.cpp:10:17: current parser token 'class'
   #0 0x55a388bfaaa5 llvm::sys::PrintStackTrace(llvm::raw_ostream&) 
/home/modocache/Source/llvm/git/dev/llvm/build/../lib/Support/Unix/Signals.inc:494:22
   #1 0x55a388bfab38 PrintStackTraceSignalHandler(void*) 
/home/modocache/Source/llvm/git/dev/llvm/build/../lib/Support/Unix/Signals.inc:558:1
   #2 0x55a388bf8b32 llvm::sys::RunSignalHandlers() 
/home/modocache/Source/llvm/git/dev/llvm/build/../lib/Support/Signals.cpp:68:20
   #3 0x55a388bfa4fb SignalHandler(int) 
/home/modocache/Source/llvm/git/dev/llvm/build/../lib/Support/Unix/Signals.inc:357:1
   #4 0x7f909ab86dd0 __restore_rt 
(/lib/x86_64-linux-gnu/libpthread.so.0+0x12dd0)
   #5 0x7f909a249077 raise 
/build/glibc-B9XfQf/glibc-2.28/signal/../sysdeps/unix/sysv/linux/raise.c:51:1
   #6

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

2019-04-10 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
rsmith added a comment.

I expect that we get this wrong in the same way for all the `SemaDeclRefs` 
declarations (`StdNamespace`, `StdBadAlloc`, and `StdAlignValT`).

I think the place where this is going wrong is `ASTDeclReader::findExisting`, 
which is assuming that the prior declaration for normal `NamedDecl`s can be 
found by name lookup; that's not true for these particular implicitly-declared 
entities. Perhaps the simplest fix would be to add a check around here:

  } else if (DeclContext *MergeDC = getPrimaryContextForMerging(Reader, DC)) {
DeclContext::lookup_result R = MergeDC->noload_lookup(Name);
for (DeclContext::lookup_iterator I = R.begin(), E = R.end(); I != E; ++I) {
  if (NamedDecl *Existing = getDeclForMerging(*I, TypedefNameForLinkage))
if (isSameEntity(Existing, D))
  return FindExistingResult(Reader, D, Existing, AnonymousDeclNumber,
TypedefNameForLinkage);
}
// [HERE]
  }

... to see if `D` is one of our three special entities, and if so to ask `Sema` 
for its current declaration of that entity (without triggering deserialization) 
and if not, set `D` as the declaration of that entity.




Comment at: lib/Sema/SemaDeclCXX.cpp:8915-8919
+  if (II->isStr("std") && PrevNS->isStdNamespace() &&
+  PrevNS != getStdNamespace()) {
+PrevNS = getStdNamespace();
+IsStd = true;
+  }

This doesn't seem right to me. This change appears to affect the case where we 
already have two distinct `std` namespaces and are declaring a third, which is 
too late -- things already went wrong if we reach that situation.



Comment at: lib/Sema/SemaDeclCXX.cpp:9215
+if (getLangOpts().Modules)
+  getStdNamespace()->setHasExternalVisibleStorage(true);
   }

This also doesn't look right: the "has external visible storage" flag should 
only be set by the external source, to indicate that it has lookup results for 
the decl context.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58920



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

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

2019-04-10 Thread Andrew Gallagher via Phabricator via cfe-commits 
andrewjcg added a comment.

Sorry for the delay.  Just catching up on the code this covers, so apologies if 
the questions don't make sense.




Comment at: lib/Sema/SemaDeclCXX.cpp:9214-9215
 getStdNamespace()->setImplicit(true);
+if (getLangOpts().Modules)
+  getStdNamespace()->setHasExternalVisibleStorage(true);
   }

I think you mentioned in another forum that one side effect of this change is 
that it's causing multiple candidates for names in `std`.  Is this the implicit 
align constructros/destructors?  Is this because we're marking the implicit 
namespace as being externally visible?



Comment at: lib/Serialization/ASTReader.cpp:9291-9295
 // Load pending declaration chains.
 for (unsigned I = 0; I != PendingDeclChains.size(); ++I)
   loadPendingDeclChain(PendingDeclChains[I].first,
PendingDeclChains[I].second);
 PendingDeclChains.clear();

How does modules normally "connect up" definitions of the same namspace 
occurring in the imported module?  Is it done here (e.g. so that a name lookup 
will search all namespace definitions)?  Is an alternate solution to this diff 
to make sure this handles the `std` implicit special case?  Apologies for the 
naivety here -- still getting up to speed on this code.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58920



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

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

2019-03-15 Thread Brian Gesiak via Phabricator via cfe-commits 
modocache added a comment.

I realize this isn't the correct solution, but would any would-be reviewers 
like to comment on the problem? Whether it's here or on the Bugzilla report 
https://bugs.llvm.org/show_bug.cgi?id=39287, as a newcomer to Clang modules I 
could use some help understanding whether this sort of behavior is expected, or 
if there are known workarounds. Any and all comments appreciated!


Repository:
  rC Clang

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

https://reviews.llvm.org/D58920



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

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

2019-03-04 Thread Brian Gesiak via Phabricator via cfe-commits 
modocache created this revision.
modocache added reviewers: rsmith, andrewjcg.
Herald added a project: clang.

This patch addresses https://bugs.llvm.org/show_bug.cgi?id=39287.

Clang creates a 'std' namespace in one of two ways: either it parses a
`namespace std { ... }` declaration, or it creates one implicitly.

One case in which Clang creates an implicit 'std' namespace is when it
encounters a virtual destructor in C++17, which results in the implicit
definition of operator new and delete overloads that take a
`std::align_val_t` argument (this behavior was added in rL282800 
).

Unfortunately, when using Clang modules, further definitions of the
`std` namespace may or may not reconcile with previous definitions. For
example, consider the two test cases in this patch:

- `mod-virtual-destructor-bug` defines `std::type_info` within the main 
translation unit. When it does so, there are two 'std' namespaces available: 
the implicitly defined one that is returned by `Sema::getStdNamespace`, and the 
explicit one defined in the `a.h` module. The `std::type_info` definition ends 
up being attached to the module's `std` namespace, and so the subsequent lookup 
of `getStdNamespace`'s `type_info` member fails. To fix this case, this patch 
adds logic to ensure `Sema::ActOnNamespaceDef` finds the `getStdNamespace` 
namespace from the current translation unit.
- `mod-virtual-destructor-bug-two` defines `std::type_info` within a module. 
Later, the lookup of `std::type_info` finds the implicitly created 
`getStdNamespace`, which only defines `std::align_val_t`, not `std::type_info`. 
To fix this case, this patch adds logic that ensures external AST sources are 
loaded when looking up `std::type_info`.


Repository:
  rC Clang

https://reviews.llvm.org/D58920

Files:
  lib/Sema/SemaDeclCXX.cpp
  lib/Serialization/ASTReader.cpp
  test/Modules/Inputs/mod-virtual-destructor-bug-two/a.h
  test/Modules/Inputs/mod-virtual-destructor-bug-two/module.modulemap
  test/Modules/Inputs/mod-virtual-destructor-bug/a.h
  test/Modules/Inputs/mod-virtual-destructor-bug/module.modulemap
  test/Modules/mod-virtual-destructor-bug-two.cpp
  test/Modules/mod-virtual-destructor-bug.cpp

Index: test/Modules/mod-virtual-destructor-bug.cpp
===
--- /dev/null
+++ test/Modules/mod-virtual-destructor-bug.cpp
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++17 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/mod-virtual-destructor-bug %s -verify
+
+class A {
+  virtual ~A() {}
+};
+
+#include "a.h"
+
+namespace std { class type_info; }
+
+void foo() {
+  typeid(foo); // expected-warning {{expression result unused}}
+}
Index: test/Modules/mod-virtual-destructor-bug-two.cpp
===
--- /dev/null
+++ test/Modules/mod-virtual-destructor-bug-two.cpp
@@ -0,0 +1,12 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -std=c++17 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/mod-virtual-destructor-bug-two %s -verify
+
+class A {
+  virtual ~A() {}
+};
+
+#include "a.h"
+
+void foo() {
+  typeid(foo); // expected-warning {{expression result unused}}
+}
Index: test/Modules/Inputs/mod-virtual-destructor-bug/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/mod-virtual-destructor-bug/module.modulemap
@@ -0,0 +1,3 @@
+module "a.h" {
+  header "a.h"
+}
Index: test/Modules/Inputs/mod-virtual-destructor-bug/a.h
===
--- /dev/null
+++ test/Modules/Inputs/mod-virtual-destructor-bug/a.h
@@ -0,0 +1 @@
+namespace std {}
Index: test/Modules/Inputs/mod-virtual-destructor-bug-two/module.modulemap
===
--- /dev/null
+++ test/Modules/Inputs/mod-virtual-destructor-bug-two/module.modulemap
@@ -0,0 +1,3 @@
+module "a.h" {
+  header "a.h"
+}
Index: test/Modules/Inputs/mod-virtual-destructor-bug-two/a.h
===
--- /dev/null
+++ test/Modules/Inputs/mod-virtual-destructor-bug-two/a.h
@@ -0,0 +1 @@
+namespace std { class type_info; }
Index: lib/Serialization/ASTReader.cpp
===
--- lib/Serialization/ASTReader.cpp
+++ lib/Serialization/ASTReader.cpp
@@ -7533,8 +7533,15 @@
 return false;
 
   auto It = Lookups.find(DC);
-  if (It == Lookups.end())
-return false;
+  if (It == Lookups.end()) {
+if (getContext().getLangOpts().Modules && DC->isStdNamespace()) {
+  for (auto StdIt = Lookups.begin(); StdIt != Lookups.end(); StdIt++)
+if (StdIt->first->isStdNamespace())
+  It = StdIt;
+}
+if (It == Lookups.end())
+  return false;
+  }
 
   Deserializing LookupResults(this);
 
Index: lib/Sema/SemaDeclCXX.cpp