[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D86310#4656024 , @Fznamznon wrote:

> Hi there,
>
> This change seems to be causing assertion failure in clang when a struct 
> contains a _BitInt with length longer than 128 - 
> https://godbolt.org/z/4jTrW4fcP .

Thanks for the report. This is a pre-existing problem for `i128:128` targets -- 
the same assertion failure can be seen, without this change, for the same 
program with e.g. `--target=aarch64 -Xclang 
-fexperimental-max-bitint-width=1024` -- so I don't think it's a problem in 
this change directly, but considering X86 is the only target that enables 
>128-bit bit integers by default, it became far more visible after this change 
and because of that, more important to fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

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

[PATCH] D122663: Mark identifier prefixes as substitutable

[Harald van Dijk via Phabricator via cfe-commits]

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

The Itanium C++ ABI says prefixes are substitutable. For most prefixes we 
already handle this: the manglePrefix(const DeclContext *, bool) and 
manglePrefix(QualType) overloads explicitly handles substitutions or defer to 
functions that handle substitutions on their behalf. The 
manglePrefix(NestedNameSpecifier *) overload, however, is different and handles 
some cases implicitly, but not all. The Identifier case was not handled; this 
change adds handling for it, as well as a test case.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122663

Files:
  clang/lib/AST/ItaniumMangle.cpp
  clang/test/CodeGenCXX/mangle.cpp


Index: clang/test/CodeGenCXX/mangle.cpp
===
--- clang/test/CodeGenCXX/mangle.cpp
+++ clang/test/CodeGenCXX/mangle.cpp
@@ -1155,3 +1155,15 @@
   // CHECK-LABEL: @_ZN6test601fIiEEvDTplL_ZNS_1aEEcvT__EE
   template void f(int);
 }
+
+namespace test61 {
+  struct X {
+struct Y {
+  using a = int;
+  using b = int;
+};
+  };
+  template  void f(typename T::Y::a, typename T::Y::b) {}
+  // CHECK-LABEL: @_ZN6test611fINS_1XEEEvNT_1Y1aENS3_1bE
+  template void f(int, int);
+}
Index: clang/lib/AST/ItaniumMangle.cpp
===
--- clang/lib/AST/ItaniumMangle.cpp
+++ clang/lib/AST/ItaniumMangle.cpp
@@ -442,6 +442,7 @@
 private:
 
   bool mangleSubstitution(const NamedDecl *ND);
+  bool mangleSubstitution(NestedNameSpecifier *NNS);
   bool mangleSubstitution(QualType T);
   bool mangleSubstitution(TemplateName Template);
   bool mangleSubstitution(uintptr_t Ptr);
@@ -455,6 +456,11 @@
 
 addSubstitution(reinterpret_cast(ND));
   }
+  void addSubstitution(NestedNameSpecifier *NNS) {
+NNS = Context.getASTContext().getCanonicalNestedNameSpecifier(NNS);
+
+addSubstitution(reinterpret_cast(NNS));
+  }
   void addSubstitution(QualType T);
   void addSubstitution(TemplateName Template);
   void addSubstitution(uintptr_t Ptr);
@@ -2041,12 +2047,16 @@
 return;
 
   case NestedNameSpecifier::Identifier:
+if (mangleSubstitution(qualifier))
+  return;
+
 // Member expressions can have these without prefixes, but that
 // should end up in mangleUnresolvedPrefix instead.
 assert(qualifier->getPrefix());
 manglePrefix(qualifier->getPrefix());
 
 mangleSourceName(qualifier->getAsIdentifier());
+addSubstitution(qualifier);
 return;
   }
 
@@ -5962,6 +5972,11 @@
   return mangleSubstitution(reinterpret_cast(ND));
 }
 
+bool CXXNameMangler::mangleSubstitution(NestedNameSpecifier *NNS) {
+  NNS = Context.getASTContext().getCanonicalNestedNameSpecifier(NNS);
+  return mangleSubstitution(reinterpret_cast(NNS));
+}
+
 /// Determine whether the given type has any qualifiers that are relevant for
 /// substitutions.
 static bool hasMangledSubstitutionQualifiers(QualType T) {


Index: clang/test/CodeGenCXX/mangle.cpp
===
--- clang/test/CodeGenCXX/mangle.cpp
+++ clang/test/CodeGenCXX/mangle.cpp
@@ -1155,3 +1155,15 @@
   // CHECK-LABEL: @_ZN6test601fIiEEvDTplL_ZNS_1aEEcvT__EE
   template void f(int);
 }
+
+namespace test61 {
+  struct X {
+struct Y {
+  using a = int;
+  using b = int;
+};
+  };
+  template  void f(typename T::Y::a, typename T::Y::b) {}
+  // CHECK-LABEL: @_ZN6test611fINS_1XEEEvNT_1Y1aENS3_1bE
+  template void f(int, int);
+}
Index: clang/lib/AST/ItaniumMangle.cpp
===
--- clang/lib/AST/ItaniumMangle.cpp
+++ clang/lib/AST/ItaniumMangle.cpp
@@ -442,6 +442,7 @@
 private:
 
   bool mangleSubstitution(const NamedDecl *ND);
+  bool mangleSubstitution(NestedNameSpecifier *NNS);
   bool mangleSubstitution(QualType T);
   bool mangleSubstitution(TemplateName Template);
   bool mangleSubstitution(uintptr_t Ptr);
@@ -455,6 +456,11 @@
 
 addSubstitution(reinterpret_cast(ND));
   }
+  void addSubstitution(NestedNameSpecifier *NNS) {
+NNS = Context.getASTContext().getCanonicalNestedNameSpecifier(NNS);
+
+addSubstitution(reinterpret_cast(NNS));
+  }
   void addSubstitution(QualType T);
   void addSubstitution(TemplateName Template);
   void addSubstitution(uintptr_t Ptr);
@@ -2041,12 +2047,16 @@
 return;
 
   case NestedNameSpecifier::Identifier:
+if (mangleSubstitution(qualifier))
+  return;
+
 // Member expressions can have these without prefixes, but that
 // should end up in mangleUnresolvedPrefix instead.
 assert(qualifier->getPrefix());
 manglePrefix(qualifier->getPrefix());
 
 mangleSourceName(qualifier->getAsIdentifier());
+addSubstitution(qualifier);
 return;
   }
 
@@ -5962,6 +5972,11 @@
   return mangleSubstitution(rein

[PATCH] D122663: Mark identifier prefixes as substitutable

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

Question: does this need to be covered by `-fclang-abi-compat=` when the prior 
mangling resulted in names that even llvm-cxxfilt agreed made no sense? (If it 
is needed, it should be an easy change.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122663

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

[PATCH] D120254: [OpenCL] Align subgroup builtin guards

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.
Herald added a project: All.

In D120254#3342551 , @dyung wrote:

> Hi, our internal release build bots are showing failures in two clang-tidy 
> tests that I bisected back to your commit, 
> clang-tidy/checkers/altera-id-dependent-backward-branch.cpp and 
> clang-tidy/checkers/altera-single-work-item-barrier.cpp. After this change, 
> both are exhibiting this error:
>
>   Error while processing 
> /home/dyung/src/upstream/aa9c2d19d9b73589d72114d6e0a4fb4ce42b922b-linux/tools/clang/tools/extra/test/clang-tidy/checkers/Output/altera-single-work-item-barrier.cpp.tmp.cpp.
>   error: enum type memory_scope not found; include the base header with 
> -finclude-default-header [clang-diagnostic-error]
>
> Oddly, this only fails in a release configuration. Can you take a look?

This was worked around by modifying tests, but I believe this is a fundamental 
problem in this change and was able to reproduce the error with plain old clang:

  $ cat test.cl
  void sub_group_barrier();
  
  $ bin/clang -cl-std=CL1.2 -S -o - test.cl
  error: enum type memory_scope not found; include the base header with 
-finclude-default-header
  1 error generated.
  
  $ bin/clang --version
  clang version 15.0.0 (g...@github.com:llvm/llvm-project 
c204cee642ee794901d2e8a9819b52ac12f92bc9)
  Target: x86_64-unknown-linux-gnu
  Thread model: posix
  InstalledDir: /home/harald/llvm-project/build/bin

The problem is that this change enables certain built-ins in OpenCL 1.2 that 
take a memory_scope argument, but the memory_scope type is not defined in 
OpenCL 1.2 mode. When we then process the function, sub_group_barrier in my 
example, things break when checking whether the declaration matches the 
built-in. I am not sure what the right fix here is. Can we just define the type 
if any extension is enabled that requires the type, or is that not allowed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120254

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

[PATCH] D120254: [OpenCL] Align subgroup builtin guards

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D120254#3419369 , @svenvh wrote:

> In D120254#3419221 , @hvdijk wrote:
>
>> The problem is that this change enables certain built-ins in OpenCL 1.2 that 
>> take a memory_scope argument, but the memory_scope type is not defined in 
>> OpenCL 1.2 mode. When we then process the function, sub_group_barrier in my 
>> example, things break when checking whether the declaration matches the 
>> built-in. I am not sure what the right fix here is. Can we just define the 
>> type if any extension is enabled that requires the type, or is that not 
>> allowed?
>
> Thanks for digging further and providing a reproducer!  I think the fix is to 
> only make the `sub_group_barrier(cl_mem_fence_flags flags, memory_scope)` 
> overload available for OpenCL 2.0 or above.  That would also match 
> `opencl-c.h`.
>
> The following patch seems to fix the issue that you described:

Huh, I could have sworn I saw more builtins than just this one but clearly I 
messed up there, that's the only one. That fix looks good to me, thanks, do you 
want to submit that as a new change along with the test or would you like a 
hand with it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120254

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

[PATCH] D121097: [C++20][Modules][HU 3/5] Emit module macros for header units.

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added inline comments.



Comment at: clang/test/Modules/cxx20-hu-04.cpp:22
+// RUN: %clang_cc1 -std=c++20 -emit-module-interface importer-01.cpp \
+// RUN:  -fmodule-file=hu-02.pcm -o B.pcm -DTDIR=%t -verify
+

On Windows, when the path starts with `C:\Users\...`, I am seeing
```
error: 'warning' diagnostics seen but not expected: 

  Line 1: \U used with no following hex digits; treating as '\' followed by 
identifier

1 error generated.
```
Looking at this test, `TDIR` is only used as a FileCheck variable, can it just 
be removed from this line?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121097

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

[PATCH] D100148: [Driver] Fix compiler-rt lookup for x32

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk updated this revision to Diff 359016.
hvdijk edited the summary of this revision.
hvdijk added a reviewer: MaskRay.
hvdijk added a comment.

Updated to use `Triple.isX32()`.

Should we perhaps just merge this as is, ahead of the update to compiler-rt to 
create x32 objects? For non-x32, this change is NFC, for x32, the change 
results in a better error message about compiler-rt objects not existing rather 
than being for the wrong architecture.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100148

Files:
  clang/lib/Driver/ToolChain.cpp
  clang/test/Driver/sanitizer-ld.c


Index: clang/test/Driver/sanitizer-ld.c
===
--- clang/test/Driver/sanitizer-ld.c
+++ clang/test/Driver/sanitizer-ld.c
@@ -284,28 +284,28 @@
 // CHECK-MSAN-NO-LINK-RUNTIME-LINUX-NOT: libclang_rt.msan
 
 // RUN: %clang -fsanitize=undefined %s -### -o %t.o 2>&1 \
-// RUN: -target i386-unknown-linux -fuse-ld=ld \
+// RUN: -target x86_64-unknown-linux-gnux32 -fuse-ld=ld \
 // RUN: -resource-dir=%S/Inputs/resource_dir \
-// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN: --sysroot=%S/Inputs/multilib_64bit_linux_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-UBSAN-LINUX %s
 
 // RUN: %clang -fsanitize=float-divide-by-zero %s -### -o %t.o 2>&1 \
-// RUN: -target i386-unknown-linux -fuse-ld=ld \
+// RUN: -target x86_64-unknown-linux-gnux32 -fuse-ld=ld \
 // RUN: -resource-dir=%S/Inputs/resource_dir \
-// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN: --sysroot=%S/Inputs/multilib_64bit_linux_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-UBSAN-LINUX %s
 
 // RUN: %clang -fsanitize=undefined %s -### -o %t.o 2>&1 \
-// RUN: -target i386-unknown-linux -fuse-ld=ld \
+// RUN: -target x86_64-unknown-linux-gnux32 -fuse-ld=ld \
 // RUN: -resource-dir=%S/Inputs/resource_dir \
-// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN: --sysroot=%S/Inputs/multilib_64bit_linux_tree \
 // RUN: -static-libsan \
 // RUN:   | FileCheck --check-prefix=CHECK-UBSAN-LINUX %s
 
 // CHECK-UBSAN-LINUX: "{{.*}}ld{{(.exe)?}}"
 // CHECK-UBSAN-LINUX-NOT: libclang_rt.asan
 // CHECK-UBSAN-LINUX-NOT: libclang_rt.ubsan_standalone_cxx
-// CHECK-UBSAN-LINUX: "--whole-archive" 
"{{.*}}libclang_rt.ubsan_standalone-i386.a" "--no-whole-archive"
+// CHECK-UBSAN-LINUX: "--whole-archive" 
"{{.*}}libclang_rt.ubsan_standalone-x32.a" "--no-whole-archive"
 // CHECK-UBSAN-LINUX-NOT: libclang_rt.asan
 // CHECK-UBSAN-LINUX-NOT: libclang_rt.ubsan_standalone_cxx
 // CHECK-UBSAN-LINUX-NOT: "-lstdc++"
Index: clang/lib/Driver/ToolChain.cpp
===
--- clang/lib/Driver/ToolChain.cpp
+++ clang/lib/Driver/ToolChain.cpp
@@ -383,6 +383,9 @@
   if (TC.getArch() == llvm::Triple::x86 && Triple.isAndroid())
 return "i686";
 
+  if (TC.getArch() == llvm::Triple::x86_64 && Triple.isX32())
+return "x32";
+
   return llvm::Triple::getArchTypeName(TC.getArch());
 }
 


Index: clang/test/Driver/sanitizer-ld.c
===
--- clang/test/Driver/sanitizer-ld.c
+++ clang/test/Driver/sanitizer-ld.c
@@ -284,28 +284,28 @@
 // CHECK-MSAN-NO-LINK-RUNTIME-LINUX-NOT: libclang_rt.msan
 
 // RUN: %clang -fsanitize=undefined %s -### -o %t.o 2>&1 \
-// RUN: -target i386-unknown-linux -fuse-ld=ld \
+// RUN: -target x86_64-unknown-linux-gnux32 -fuse-ld=ld \
 // RUN: -resource-dir=%S/Inputs/resource_dir \
-// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN: --sysroot=%S/Inputs/multilib_64bit_linux_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-UBSAN-LINUX %s
 
 // RUN: %clang -fsanitize=float-divide-by-zero %s -### -o %t.o 2>&1 \
-// RUN: -target i386-unknown-linux -fuse-ld=ld \
+// RUN: -target x86_64-unknown-linux-gnux32 -fuse-ld=ld \
 // RUN: -resource-dir=%S/Inputs/resource_dir \
-// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN: --sysroot=%S/Inputs/multilib_64bit_linux_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-UBSAN-LINUX %s
 
 // RUN: %clang -fsanitize=undefined %s -### -o %t.o 2>&1 \
-// RUN: -target i386-unknown-linux -fuse-ld=ld \
+// RUN: -target x86_64-unknown-linux-gnux32 -fuse-ld=ld \
 // RUN: -resource-dir=%S/Inputs/resource_dir \
-// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN: --sysroot=%S/Inputs/multilib_64bit_linux_tree \
 // RUN: -static-libsan \
 // RUN:   | FileCheck --check-prefix=CHECK-UBSAN-LINUX %s
 
 // CHECK-UBSAN-LINUX: "{{.*}}ld{{(.exe)?}}"
 // CHECK-UBSAN-LINUX-NOT: libclang_rt.asan
 // CHECK-UBSAN-LINUX-NOT: libclang_rt.ubsan_standalone_cxx
-// CHECK-UBSAN-LINUX: "--whole-archive" "{{.*}}libclang_rt.ubsan_standalone-i386.a" "--no-whole-archive"
+// CHECK-UBSAN-LINUX: "--whole-archive" "{{.*}}libclang_rt.ubsan_standalone-x32.a" "--no-whole-archive"
 /

[PATCH] D100148: [Driver] Fix compiler-rt lookup for x32

[Harald van Dijk 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 rG66ab8568c485: [Driver] Fix compiler-rt lookup for x32 
(authored by hvdijk).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100148

Files:
  clang/lib/Driver/ToolChain.cpp
  clang/test/Driver/sanitizer-ld.c


Index: clang/test/Driver/sanitizer-ld.c
===
--- clang/test/Driver/sanitizer-ld.c
+++ clang/test/Driver/sanitizer-ld.c
@@ -284,28 +284,28 @@
 // CHECK-MSAN-NO-LINK-RUNTIME-LINUX-NOT: libclang_rt.msan
 
 // RUN: %clang -fsanitize=undefined %s -### -o %t.o 2>&1 \
-// RUN: -target i386-unknown-linux -fuse-ld=ld \
+// RUN: -target x86_64-unknown-linux-gnux32 -fuse-ld=ld \
 // RUN: -resource-dir=%S/Inputs/resource_dir \
-// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN: --sysroot=%S/Inputs/multilib_64bit_linux_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-UBSAN-LINUX %s
 
 // RUN: %clang -fsanitize=float-divide-by-zero %s -### -o %t.o 2>&1 \
-// RUN: -target i386-unknown-linux -fuse-ld=ld \
+// RUN: -target x86_64-unknown-linux-gnux32 -fuse-ld=ld \
 // RUN: -resource-dir=%S/Inputs/resource_dir \
-// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN: --sysroot=%S/Inputs/multilib_64bit_linux_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-UBSAN-LINUX %s
 
 // RUN: %clang -fsanitize=undefined %s -### -o %t.o 2>&1 \
-// RUN: -target i386-unknown-linux -fuse-ld=ld \
+// RUN: -target x86_64-unknown-linux-gnux32 -fuse-ld=ld \
 // RUN: -resource-dir=%S/Inputs/resource_dir \
-// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN: --sysroot=%S/Inputs/multilib_64bit_linux_tree \
 // RUN: -static-libsan \
 // RUN:   | FileCheck --check-prefix=CHECK-UBSAN-LINUX %s
 
 // CHECK-UBSAN-LINUX: "{{.*}}ld{{(.exe)?}}"
 // CHECK-UBSAN-LINUX-NOT: libclang_rt.asan
 // CHECK-UBSAN-LINUX-NOT: libclang_rt.ubsan_standalone_cxx
-// CHECK-UBSAN-LINUX: "--whole-archive" 
"{{.*}}libclang_rt.ubsan_standalone-i386.a" "--no-whole-archive"
+// CHECK-UBSAN-LINUX: "--whole-archive" 
"{{.*}}libclang_rt.ubsan_standalone-x32.a" "--no-whole-archive"
 // CHECK-UBSAN-LINUX-NOT: libclang_rt.asan
 // CHECK-UBSAN-LINUX-NOT: libclang_rt.ubsan_standalone_cxx
 // CHECK-UBSAN-LINUX-NOT: "-lstdc++"
Index: clang/lib/Driver/ToolChain.cpp
===
--- clang/lib/Driver/ToolChain.cpp
+++ clang/lib/Driver/ToolChain.cpp
@@ -383,6 +383,9 @@
   if (TC.getArch() == llvm::Triple::x86 && Triple.isAndroid())
 return "i686";
 
+  if (TC.getArch() == llvm::Triple::x86_64 && Triple.isX32())
+return "x32";
+
   return llvm::Triple::getArchTypeName(TC.getArch());
 }
 


Index: clang/test/Driver/sanitizer-ld.c
===
--- clang/test/Driver/sanitizer-ld.c
+++ clang/test/Driver/sanitizer-ld.c
@@ -284,28 +284,28 @@
 // CHECK-MSAN-NO-LINK-RUNTIME-LINUX-NOT: libclang_rt.msan
 
 // RUN: %clang -fsanitize=undefined %s -### -o %t.o 2>&1 \
-// RUN: -target i386-unknown-linux -fuse-ld=ld \
+// RUN: -target x86_64-unknown-linux-gnux32 -fuse-ld=ld \
 // RUN: -resource-dir=%S/Inputs/resource_dir \
-// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN: --sysroot=%S/Inputs/multilib_64bit_linux_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-UBSAN-LINUX %s
 
 // RUN: %clang -fsanitize=float-divide-by-zero %s -### -o %t.o 2>&1 \
-// RUN: -target i386-unknown-linux -fuse-ld=ld \
+// RUN: -target x86_64-unknown-linux-gnux32 -fuse-ld=ld \
 // RUN: -resource-dir=%S/Inputs/resource_dir \
-// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN: --sysroot=%S/Inputs/multilib_64bit_linux_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-UBSAN-LINUX %s
 
 // RUN: %clang -fsanitize=undefined %s -### -o %t.o 2>&1 \
-// RUN: -target i386-unknown-linux -fuse-ld=ld \
+// RUN: -target x86_64-unknown-linux-gnux32 -fuse-ld=ld \
 // RUN: -resource-dir=%S/Inputs/resource_dir \
-// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN: --sysroot=%S/Inputs/multilib_64bit_linux_tree \
 // RUN: -static-libsan \
 // RUN:   | FileCheck --check-prefix=CHECK-UBSAN-LINUX %s
 
 // CHECK-UBSAN-LINUX: "{{.*}}ld{{(.exe)?}}"
 // CHECK-UBSAN-LINUX-NOT: libclang_rt.asan
 // CHECK-UBSAN-LINUX-NOT: libclang_rt.ubsan_standalone_cxx
-// CHECK-UBSAN-LINUX: "--whole-archive" "{{.*}}libclang_rt.ubsan_standalone-i386.a" "--no-whole-archive"
+// CHECK-UBSAN-LINUX: "--whole-archive" "{{.*}}libclang_rt.ubsan_standalone-x32.a" "--no-whole-archive"
 // CHECK-UBSAN-LINUX-NOT: libclang_rt.asan
 // CHECK-UBSAN-LINUX-NOT: libclang_rt.ubsan_standalone_cxx
 // CHECK-UBSAN-LINUX-NOT: "-lstdc++"
Index: clang/lib/Driver/ToolChain.cpp
=

[PATCH] D118935: [SYCL] Disallow explicit casts between mismatching address spaces

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.
Herald added a project: All.

Hi, what is the rationale here? This reuses the logic that was written for 
OpenCL mode, but in OpenCL mode, it was made an error with the idea that a new 
keyword `addrspace_cast` could be used in those cases where the user actually 
wants an address space cast. Here, in SYCL mode, it's just made an error with 
no way out for the user that I can see if they actually want this. Is this 
really correct?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118935

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

[PATCH] D118935: [SYCL] Disallow explicit casts between mismatching address spaces

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D118935#3399095 , @Fznamznon wrote:

> I was under impression that the change is small and therefore easy to 
> understand. Would some comment like "SYCL re-uses OpenCL mode diagnostics to 
> emit errors in case the cast happens between disjoint address spaces" help?

Speaking only for myself, but it was clear to me that that's what it did. What 
wasn't clear to me was why that's okay, and that was because I was under the 
impression that there were ways around this in OpenCL that are unavailable in 
SYCL. What might have helped me was a comment not saying that the cast is also 
an error is OpenCL mode, but that there is no way at all to express the 
conversion in OpenCL mode. This is all in hindsight though, I am not sure to 
what extent my confusion was foreseeable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118935

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

[PATCH] D122663: Mark identifier prefixes as substitutable

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added subscribers: urnathan, erichkeane.
hvdijk added a comment.

ping. Apologies, I don't know who to add as a reviewer, there is no one 
specifically listed as code owner and it does not seem to be handled by anyone 
in particular. @urnathan, @erichkeane, you two appear to be the most recent 
people who pushed commits specifically for mangling issues, would either of you 
be able to review this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122663

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

[PATCH] D122663: Mark identifier prefixes as substitutable

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk updated this revision to Diff 423426.
hvdijk added a comment.
Herald added a subscriber: dexonsmith.

Allow the old mangling with suitable -fclang-abi-compat= value


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122663

Files:
  clang/include/clang/Basic/LangOptions.h
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGenCXX/clang-abi-compat.cpp
  clang/test/CodeGenCXX/mangle.cpp

Index: clang/test/CodeGenCXX/mangle.cpp
===
--- clang/test/CodeGenCXX/mangle.cpp
+++ clang/test/CodeGenCXX/mangle.cpp
@@ -1155,3 +1155,15 @@
   // CHECK-LABEL: @_ZN6test601fIiEEvDTplL_ZNS_1aEEcvT__EE
   template void f(int);
 }
+
+namespace test61 {
+  struct X {
+struct Y {
+  using a = int;
+  using b = int;
+};
+  };
+  template  void f(typename T::Y::a, typename T::Y::b) {}
+  // CHECK-LABEL: @_ZN6test611fINS_1XEEEvNT_1Y1aENS3_1bE
+  template void f(int, int);
+}
Index: clang/test/CodeGenCXX/clang-abi-compat.cpp
===
--- clang/test/CodeGenCXX/clang-abi-compat.cpp
+++ clang/test/CodeGenCXX/clang-abi-compat.cpp
@@ -1,25 +1,27 @@
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++98 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=3.0 %s -emit-llvm -o - -Wno-c++11-extensions \
-// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=3.0 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=3.8 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=3.9 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,PRE5,PRE12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,PRE5,PRE12,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=4.0 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,PRE5,PRE12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,PRE5,PRE12,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=5 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=11 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++20 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=11 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17,PRE12-CXX20,PRE13-CXX20 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17,PRE12-CXX20,PRE13-CXX20,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++20 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=12 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V12-CXX17,V12-CXX20,PRE13-CXX20 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V12-CXX17,V12-CXX20,PRE13-CXX20,PRE15 %s
+// RUN: %clang_cc1 -no-opaque-pointers -std=c++20 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=14 %s -emit-llvm -o - \
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V12-CXX17,V12-CXX20,V13-CXX20,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++98 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=latest %s -emit-llvm -o - -Wno-c++11-extensions \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++20 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=latest %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V12-CXX17,V12-CXX20,V13-CXX20 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V12-CXX17,V12-CXX20,V13-CXX20,V15 %s
 
 typedef __attribute__((vector_size(8))) long long v1xi64;
 void clang39(v1xi64) {}
@@ -147,3 +149,14 @@
 inline auto inline_var_lambda = observe_lambdas([]{}, []{}, (int*)0, (int*)0);

[PATCH] D122663: Mark identifier prefixes as substitutable

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D122663#3456507 , @erichkeane 
wrote:

> I believe the answer here is 'yes'.  We also likely need release notes.

Thanks for the feedback. -fclang-abi-compat= support added. Other mangling 
bugfixes appear to not have made it into release notes so I did not add it for 
this one either, but if that is something that's wanted I can add it; I'll wait 
for some more comments on that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122663

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

[PATCH] D122663: Mark identifier prefixes as substitutable

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D122663#3457131 , @erichkeane 
wrote:

> Our most recent direction is to document any non-NFC patches in the release 
> notes if at all sensible, so I think this meets those requirements.

Thanks, I'm not finding documentation of any change in policy even after 
specifically looking for it, so I have opened 
https://github.com/llvm/llvm-project/issues/54965 for that. However, I have no 
issue with the policy and will add a release note for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122663

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

[PATCH] D122663: Mark identifier prefixes as substitutable

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk updated this revision to Diff 423445.
hvdijk added a comment.

Add to release notes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122663

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/LangOptions.h
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGenCXX/clang-abi-compat.cpp
  clang/test/CodeGenCXX/mangle.cpp

Index: clang/test/CodeGenCXX/mangle.cpp
===
--- clang/test/CodeGenCXX/mangle.cpp
+++ clang/test/CodeGenCXX/mangle.cpp
@@ -1155,3 +1155,15 @@
   // CHECK-LABEL: @_ZN6test601fIiEEvDTplL_ZNS_1aEEcvT__EE
   template void f(int);
 }
+
+namespace test61 {
+  struct X {
+struct Y {
+  using a = int;
+  using b = int;
+};
+  };
+  template  void f(typename T::Y::a, typename T::Y::b) {}
+  // CHECK-LABEL: @_ZN6test611fINS_1XEEEvNT_1Y1aENS3_1bE
+  template void f(int, int);
+}
Index: clang/test/CodeGenCXX/clang-abi-compat.cpp
===
--- clang/test/CodeGenCXX/clang-abi-compat.cpp
+++ clang/test/CodeGenCXX/clang-abi-compat.cpp
@@ -1,25 +1,27 @@
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++98 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=3.0 %s -emit-llvm -o - -Wno-c++11-extensions \
-// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=3.0 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=3.8 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=3.9 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,PRE5,PRE12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,PRE5,PRE12,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=4.0 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,PRE5,PRE12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,PRE5,PRE12,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=5 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=11 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++20 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=11 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17,PRE12-CXX20,PRE13-CXX20 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17,PRE12-CXX20,PRE13-CXX20,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++20 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=12 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V12-CXX17,V12-CXX20,PRE13-CXX20 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V12-CXX17,V12-CXX20,PRE13-CXX20,PRE15 %s
+// RUN: %clang_cc1 -no-opaque-pointers -std=c++20 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=14 %s -emit-llvm -o - \
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V12-CXX17,V12-CXX20,V13-CXX20,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++98 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=latest %s -emit-llvm -o - -Wno-c++11-extensions \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++20 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=latest %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V12-CXX17,V12-CXX20,V13-CXX20 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V12-CXX17,V12-CXX20,V13-CXX20,V15 %s
 
 typedef __attribute__((vector_size(8))) long long v1xi64;
 void clang39(v1xi64) {}
@@ -147,3 +149,14 @@
 inline auto inline_var_lambda = observe_lambdas([]{}, []{}, (int*)0, (int*)0);
 int use_inline_var_lambda() { return inline_var_l

[PATCH] D122663: Mark identifier prefixes as substitutable

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk updated this revision to Diff 423482.
hvdijk added a comment.

Extend test, add assert.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122663

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/LangOptions.h
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGenCXX/clang-abi-compat.cpp
  clang/test/CodeGenCXX/mangle.cpp

Index: clang/test/CodeGenCXX/mangle.cpp
===
--- clang/test/CodeGenCXX/mangle.cpp
+++ clang/test/CodeGenCXX/mangle.cpp
@@ -1155,3 +1155,15 @@
   // CHECK-LABEL: @_ZN6test601fIiEEvDTplL_ZNS_1aEEcvT__EE
   template void f(int);
 }
+
+namespace test61 {
+  struct X {
+struct Y {
+  using a = int;
+  using b = int;
+};
+  };
+  template  void f(typename T::Y::a, typename T::Y::b) {}
+  // CHECK-LABEL: @_ZN6test611fINS_1XEEEvNT_1Y1aENS3_1bE
+  template void f(int, int);
+}
Index: clang/test/CodeGenCXX/clang-abi-compat.cpp
===
--- clang/test/CodeGenCXX/clang-abi-compat.cpp
+++ clang/test/CodeGenCXX/clang-abi-compat.cpp
@@ -1,25 +1,27 @@
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++98 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=3.0 %s -emit-llvm -o - -Wno-c++11-extensions \
-// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=3.0 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=3.8 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=3.9 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,PRE5,PRE12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,PRE5,PRE12,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=4.0 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,PRE5,PRE12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,PRE5,PRE12,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=5 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=11 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++20 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=11 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17,PRE12-CXX20,PRE13-CXX20 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17,PRE12-CXX20,PRE13-CXX20,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++20 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=12 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V12-CXX17,V12-CXX20,PRE13-CXX20 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V12-CXX17,V12-CXX20,PRE13-CXX20,PRE15 %s
+// RUN: %clang_cc1 -no-opaque-pointers -std=c++20 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=14 %s -emit-llvm -o - \
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V12-CXX17,V12-CXX20,V13-CXX20,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++98 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=latest %s -emit-llvm -o - -Wno-c++11-extensions \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++20 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=latest %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V12-CXX17,V12-CXX20,V13-CXX20 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V12-CXX17,V12-CXX20,V13-CXX20,V15 %s
 
 typedef __attribute__((vector_size(8))) long long v1xi64;
 void clang39(v1xi64) {}
@@ -147,3 +149,14 @@
 inline auto inline_var_lambda = observe_lambdas([]{}, []{}, (int*)0, (int*)0);
 int use_inline_var_lambda() { return inline_va

[PATCH] D122663: Mark identifier prefixes as substitutable

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk marked 2 inline comments as done.
hvdijk added inline comments.



Comment at: clang/lib/AST/ItaniumMangle.cpp:6031
+bool CXXNameMangler::mangleSubstitution(NestedNameSpecifier *NNS) {
+  NNS = Context.getASTContext().getCanonicalNestedNameSpecifier(NNS);
+  return mangleSubstitution(reinterpret_cast(NNS));

rsmith wrote:
> This seems a little error-prone to me: calling this on a type NNS would do 
> the wrong thing (those are supposed to share a substitution number with the 
> type, rather than have a substitution of their own).
> 
> We could handle the various cases here and dispatch to the right forms of 
> `mangleSubstitution` depending on the kind of NNS, but that code would all be 
> unreachable / untested. So maybe we should just make this assert that 
> `NNS->getKind() == NestedNameSpecifier::Identifier`. (And optionally we could 
> give this a more specific name, eg `mangleSubstitutionForIdentifierNNS`?)
I have added the assert that you suggested. I would actually have preferred for 
this function to be used for other NNS substitutions as well to better align 
with how the spec says substitutions should be handled (it's a rule of 
``, not any component within) but that seemed like an unnecessarily 
more invasive change. If you are okay with it I would like to keep the function 
named just `mangleSubstitution` to keep that open as option for a possible 
future clean-up.



Comment at: clang/test/CodeGenCXX/clang-abi-compat.cpp:160
+template  void test10(typename T::Y::a, typename T::Y::b) {}
+// PRE15: @_Z6test10I1XEvNT_1Y1aENS1_1Y1bE
+// V15:   @_Z6test10I1XEvNT_1Y1aENS2_1bE

rsmith wrote:
> I think this test does not capture an important property for which we should 
> have test coverage: that we used to not register a substitution for `T::Y` 
> (not only that we used to not *use* a substitution for it). For example, this 
> test would capture that:
> 
> ```
> struct X {
>   struct Y {
> using a = int;
> using b = int;
>   };
> };
> template  void test10(typename T::Y::a, typename T::Y::b, float*, 
> float*) {}
> template void test10(int, int, float*, float*);
> ```
> ... where the numbering of `float*` depends on how many substitutions we 
> created for the earlier types.
> 
> (Put another way, this test covers only one of the two `if (!Clang14Compat)` 
> tests in ItaniumMangle.cpp, and we should cover both.)
I get where you're coming from. My thinking was the other way around, if we add 
the first `if (!Clang14Compat && ...)` by itself that will have no effect and 
cannot be tested for, then if we add the second `if (!Clang14Compat)` that does 
have impact and needs testing. You're looking at it the other way around, if we 
add the second `if (!Clang14Compat)` first, we fix one bug, and if we then add 
the first `if (!Clang14Compat && ...)` we fix another. Happy to extend the test 
like you suggested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122663

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

[PATCH] D122663: Mark identifier prefixes as substitutable

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:240
+  GCC. This breaks binary compatibility with code compiled with earlier 
versions
+  of clang; use the `-fclang-abi-compat=14` option to get the old mangling.
 

This is incorrect rst syntax.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122663

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

[PATCH] D122663: Mark identifier prefixes as substitutable

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk updated this revision to Diff 423879.
hvdijk added a comment.

Fixed release notes to use correct RST syntax.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122663

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/LangOptions.h
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGenCXX/clang-abi-compat.cpp
  clang/test/CodeGenCXX/mangle.cpp

Index: clang/test/CodeGenCXX/mangle.cpp
===
--- clang/test/CodeGenCXX/mangle.cpp
+++ clang/test/CodeGenCXX/mangle.cpp
@@ -1155,3 +1155,15 @@
   // CHECK-LABEL: @_ZN6test601fIiEEvDTplL_ZNS_1aEEcvT__EE
   template void f(int);
 }
+
+namespace test61 {
+  struct X {
+struct Y {
+  using a = int;
+  using b = int;
+};
+  };
+  template  void f(typename T::Y::a, typename T::Y::b) {}
+  // CHECK-LABEL: @_ZN6test611fINS_1XEEEvNT_1Y1aENS3_1bE
+  template void f(int, int);
+}
Index: clang/test/CodeGenCXX/clang-abi-compat.cpp
===
--- clang/test/CodeGenCXX/clang-abi-compat.cpp
+++ clang/test/CodeGenCXX/clang-abi-compat.cpp
@@ -1,25 +1,27 @@
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++98 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=3.0 %s -emit-llvm -o - -Wno-c++11-extensions \
-// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=3.0 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=3.8 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=3.9 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,PRE5,PRE12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,PRE5,PRE12,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=4.0 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,PRE5,PRE12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,PRE5,PRE12,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=5 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=11 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++20 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=11 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17,PRE12-CXX20,PRE13-CXX20 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17,PRE12-CXX20,PRE13-CXX20,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++20 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=12 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V12-CXX17,V12-CXX20,PRE13-CXX20 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V12-CXX17,V12-CXX20,PRE13-CXX20,PRE15 %s
+// RUN: %clang_cc1 -no-opaque-pointers -std=c++20 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=14 %s -emit-llvm -o - \
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V12-CXX17,V12-CXX20,V13-CXX20,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++98 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=latest %s -emit-llvm -o - -Wno-c++11-extensions \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++20 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=latest %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V12-CXX17,V12-CXX20,V13-CXX20 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V12-CXX17,V12-CXX20,V13-CXX20,V15 %s
 
 typedef __attribute__((vector_size(8))) long long v1xi64;
 void clang39(v1xi64) {}
@@ -147,3 +149,14 @@
 inline auto inline_var_lambda = observe_lambdas([]{}, []{}, (int*)0, (int*)0);
 int use_inline_var_lambd

[PATCH] D122663: Mark identifier prefixes as substitutable

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D122663#3457330 , @erichkeane 
wrote:

> LGTM!  I would like @rjmccall to take a pass if he ends up having time in the 
> next day or two (perhaps tack on an extra day or two because of Easter), else 
> I'll be willing to approve later in the week.

ping, I did get feedback from @rsmith (much appreciated) and applied his 
suggestions, but not from @rjmccall, would you be okay to approve it then?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122663

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

[PATCH] D122663: Mark identifier prefixes as substitutable

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D122663#3471741 , @erichkeane 
wrote:

> Ping me EOW if @rsmith doesn't respond in the meantime.  It is also not clear 
> to me whether you were able to capture/fix the issue he had with the 
> clang-abi-compat.cpp test.

Will do, sure. For the record, for that test I applied @rsmith's suggestion 
verbatim so it *should* address his concern, but I will wait for him to confirm.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122663

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

[PATCH] D122663: Mark identifier prefixes as substitutable

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D122663#3471763 , @erichkeane 
wrote:

> I DID see that and am REASONABLY sure you do, but sometimes folks will ask 
> for test coverage because they suspect the resulting behavior will 
> demonstrate some sort of problem with the current patch.  I DID run this 
> through the demangler and found that the PRE15 case looks odd?
>
> `void test10(X::Y::a, X::Y::b, float*, X::Y)`
> vs
> `void test10(X::Y::a, X::Y::b, float*, float*)`
>
> Or is that exactly the bug being caught here?  That is, is the `float*` 
> substitution not being properly registered/emitted and being confused with 
> `X::Y`?

Yes, that is exactly it. That is what @rsmith was getting at with his comment 
about regarding this as two separate bugs (he can correct me if I am misstating 
things), one that we do not add the substitution and as such use wrong numbers 
for subsequent substitutions, the other that we do not use the substitution. 
The other is what I had covered in my initial test, his addition covers that 
first bug where we wrongly used S4 when we should be using S5.

> PS:, and as an unrelated-lament... A while back I wrote a test regarding 
> mangling where I ALSO had it run us through llvm-cxxfilt so that we had an 
> output of every mangled string that showed the demangled version.  I wish 
> we'd done that from the beginning to make it much less confusing what is 
> going on in these.

llvm-cxxfilt has no option to be compatible with earlier incorrect mangling 
though, so it would not help with this particular test. But it could help with 
other tests, agreed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122663

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

[PATCH] D122663: Mark identifier prefixes as substitutable

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D122663#3471866 , @erichkeane 
wrote:

> This actually wouldn't be necessary in this case: cxxfilt is already 'right', 
> this is just for humans to be able to see "we changed this from mangling as 
>  to ".

That's a good point. We do have tests in clang that use llvm-cxxfilt already 
(the PPC intrinsic tests) so there should be no problem with it as a 
dependency; I think once this is in I might submit that as a follow-up change 
for review unless someone else beats me to it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122663

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

[PATCH] D122663: Mark identifier prefixes as substitutable

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D122663#3471741 , @erichkeane 
wrote:

> Ping me EOW if @rsmith doesn't respond in the meantime.

ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122663

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

[PATCH] D122663: Mark identifier prefixes as substitutable

[Harald van Dijk 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 rGfed7be096f8e: Mark identifier prefixes as substitutable 
(authored by hvdijk).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122663

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/LangOptions.h
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGenCXX/clang-abi-compat.cpp
  clang/test/CodeGenCXX/mangle.cpp

Index: clang/test/CodeGenCXX/mangle.cpp
===
--- clang/test/CodeGenCXX/mangle.cpp
+++ clang/test/CodeGenCXX/mangle.cpp
@@ -1155,3 +1155,15 @@
   // CHECK-LABEL: @_ZN6test601fIiEEvDTplL_ZNS_1aEEcvT__EE
   template void f(int);
 }
+
+namespace test61 {
+  struct X {
+struct Y {
+  using a = int;
+  using b = int;
+};
+  };
+  template  void f(typename T::Y::a, typename T::Y::b) {}
+  // CHECK-LABEL: @_ZN6test611fINS_1XEEEvNT_1Y1aENS3_1bE
+  template void f(int, int);
+}
Index: clang/test/CodeGenCXX/clang-abi-compat.cpp
===
--- clang/test/CodeGenCXX/clang-abi-compat.cpp
+++ clang/test/CodeGenCXX/clang-abi-compat.cpp
@@ -1,25 +1,27 @@
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++98 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=3.0 %s -emit-llvm -o - -Wno-c++11-extensions \
-// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=3.0 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=3.8 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,PRE39,PRE5,PRE12,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=3.9 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,PRE5,PRE12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,PRE5,PRE12,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=4.0 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,PRE5,PRE12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,PRE5,PRE12,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=5 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++17 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=11 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++20 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=11 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17,PRE12-CXX20,PRE13-CXX20 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,PRE12,PRE12-CXX17,PRE12-CXX20,PRE13-CXX20,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++20 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=12 %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V12-CXX17,V12-CXX20,PRE13-CXX20 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V12-CXX17,V12-CXX20,PRE13-CXX20,PRE15 %s
+// RUN: %clang_cc1 -no-opaque-pointers -std=c++20 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=14 %s -emit-llvm -o - \
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V12-CXX17,V12-CXX20,V13-CXX20,PRE15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++98 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=latest %s -emit-llvm -o - -Wno-c++11-extensions \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V15 %s
 // RUN: %clang_cc1 -no-opaque-pointers -std=c++20 -triple x86_64-linux-gnu -fenable-matrix -fclang-abi-compat=latest %s -emit-llvm -o - \
-// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V12-CXX17,V12-CXX20,V13-CXX20 %s
+// RUN: | FileCheck --check-prefixes=CHECK,V39,V5,V12,V12-CXX17,V12-CXX20,V13-CXX20,V15 %s
 
 typedef __attribute__((vector_size(8))) long long v1xi64;
 void clang39(v1xi64) {}
@@ -147,3 +149,1

[PATCH] D122663: Mark identifier prefixes as substitutable

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added inline comments.



Comment at: clang/lib/AST/ItaniumMangle.cpp:6031
+bool CXXNameMangler::mangleSubstitution(NestedNameSpecifier *NNS) {
+  NNS = Context.getASTContext().getCanonicalNestedNameSpecifier(NNS);
+  return mangleSubstitution(reinterpret_cast(NNS));

urnathan wrote:
> hvdijk wrote:
> > rsmith wrote:
> > > This seems a little error-prone to me: calling this on a type NNS would 
> > > do the wrong thing (those are supposed to share a substitution number 
> > > with the type, rather than have a substitution of their own).
> > > 
> > > We could handle the various cases here and dispatch to the right forms of 
> > > `mangleSubstitution` depending on the kind of NNS, but that code would 
> > > all be unreachable / untested. So maybe we should just make this assert 
> > > that `NNS->getKind() == NestedNameSpecifier::Identifier`. (And optionally 
> > > we could give this a more specific name, eg 
> > > `mangleSubstitutionForIdentifierNNS`?)
> > I have added the assert that you suggested. I would actually have preferred 
> > for this function to be used for other NNS substitutions as well to better 
> > align with how the spec says substitutions should be handled (it's a rule 
> > of ``, not any component within) but that seemed like an 
> > unnecessarily more invasive change. If you are okay with it I would like to 
> > keep the function named just `mangleSubstitution` to keep that open as 
> > option for a possible future clean-up.
> One could name it `mangleSubstitutionForIdentifierNNS` now and rename it in 
> the future, if your unification dream comes true?  That names it for what it 
> does now.
> 
> Just a thought, not a requirement.
Sorry, that comment came after I pushed it already. I have some more things to 
look into when I have some more time (including @erichkeane's comment about the 
test), will see if it makes sense to include with that, or perhaps to just make 
that NFC change to allow it to be used more generally.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122663

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

[PATCH] D86695: [RFC] Call ParseLangArgs for all inputs

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk created this revision.
Herald added subscribers: cfe-commits, atanasyan, arichardson, sdardis.
Herald added a project: clang.
hvdijk requested review of this revision.

clang/lib/CodeGen/BackendUtil.cpp contains multiple checks for attributes of 
LangOpts. The option processing that sets up LangOpts is mostly done by 
ParseLangArgs, which is skipped for LLVM IR files. Because of this, there are 
code generation differences when the -save-temps command line option is used: 
that command line option causes LLVM IR to be emitted to file and a new process 
to be spawned to process that file, which does not process options the same way.

An example of this is

  typedef float __attribute__((ext_vector_type(2))) float2;
  float2 foo (float2 a, float2 b, float2 c) {
return a * b + c;
  }

This used to generate different code with clang --target=mips -mcpu=mips32r5 
-mfp64 -mmsa -O3 -ffp-contract=fast depending on whether -save-temps was also 
present, because the -ffp-contract=fast option affects instruction selection 
but was ignored for LLVM IR input files.

While CompilerInvocation::CreateFromArgs contained special exceptions for a few 
options that were handled by ParseLangArgs that also need to be handled for 
LLVM IR, there are many more, and just allowing ParseLangArgs to be called 
seems like the simpler fix.

For InputKind::Precompiled, -std=* options were previously silently ignored and 
continue to be silently ignored, as it is reasonable for users to add them and 
in fact this is done in quite a number of tests in the test suite.

For Language::LLVM_IR, -std=* options were previously silently ignored and now 
result in an error, as it is not reasonable for users to add them.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86695

Files:
  clang/lib/Frontend/CompilerInvocation.cpp

Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -2432,9 +2432,11 @@
   const LangStandard &S) {
   switch (IK.getLanguage()) {
   case Language::Unknown:
-  case Language::LLVM_IR:
 llvm_unreachable("should not parse language flags for this input");
 
+  case Language::LLVM_IR:
+return false;
+
   case Language::C:
   case Language::ObjC:
   case Language::RenderScript:
@@ -2507,30 +2509,32 @@
 if (LangStd == LangStandard::lang_unspecified) {
   Diags.Report(diag::err_drv_invalid_value)
 << A->getAsString(Args) << A->getValue();
-  // Report supported standards with short description.
-  for (unsigned KindValue = 0;
-   KindValue != LangStandard::lang_unspecified;
-   ++KindValue) {
-const LangStandard &Std = LangStandard::getLangStandardForKind(
-  static_cast(KindValue));
-if (IsInputCompatibleWithStandard(IK, Std)) {
-  auto Diag = Diags.Report(diag::note_drv_use_standard);
-  Diag << Std.getName() << Std.getDescription();
-  unsigned NumAliases = 0;
+  if (IK.getFormat() != InputKind::Precompiled) {
+// Report supported standards with short description.
+for (unsigned KindValue = 0;
+ KindValue != LangStandard::lang_unspecified;
+ ++KindValue) {
+  const LangStandard &Std = LangStandard::getLangStandardForKind(
+static_cast(KindValue));
+  if (IsInputCompatibleWithStandard(IK, Std)) {
+auto Diag = Diags.Report(diag::note_drv_use_standard);
+Diag << Std.getName() << Std.getDescription();
+unsigned NumAliases = 0;
 #define LANGSTANDARD(id, name, lang, desc, features)
 #define LANGSTANDARD_ALIAS(id, alias) \
-  if (KindValue == LangStandard::lang_##id) ++NumAliases;
+if (KindValue == LangStandard::lang_##id) ++NumAliases;
 #define LANGSTANDARD_ALIAS_DEPR(id, alias)
 #include "clang/Basic/LangStandards.def"
-  Diag << NumAliases;
+Diag << NumAliases;
 #define LANGSTANDARD(id, name, lang, desc, features)
 #define LANGSTANDARD_ALIAS(id, alias) \
-  if (KindValue == LangStandard::lang_##id) Diag << alias;
+if (KindValue == LangStandard::lang_##id) Diag << alias;
 #define LANGSTANDARD_ALIAS_DEPR(id, alias)
 #include "clang/Basic/LangStandards.def"
+  }
 }
   }
-} else {
+} else if (IK.getFormat() != InputKind::Precompiled) {
   // Valid standard, check to make sure language and standard are
   // compatible.
   const LangStandard &Std = LangStandard::getLangStandardForKind(LangStd);
@@ -2592,7 +2596,9 @@
   Opts.DeclareOpenCLBuiltins = Args.hasArg(OPT_fdeclare_opencl_builtins);
 
   llvm::Triple T(TargetOpts.Triple);
-  CompilerInvocation::setLangDefaults(Opts, IK, T, PPOpts, LangStd);
+  if (IK.getFormat() != InputKind::Precompiled &&
+  IK.getLanguage() != Language::LLVM_IR)
+CompilerInvocati

[PATCH] D52050: [Driver] Fix architecture triplets and search paths for Linux x32

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

I've been able to check what Ubuntu 20.10 offers in terms of x32 support. Its 
kernel supports x32 binaries, it provides x32 versions of core system libraries 
in separate packages (e.g. libc6-x32, libx32stdc++6, libx32z1), and it provides 
a compiler that targets x32 by default (gcc-x86-64-linux-gnux32). These Ubuntu 
packages do not use the Debian/Ubuntu multiarch approach: the packages are 
completely independent of the corresponding x64 and i386 versions with separate 
names, and nothing in Ubuntu installs any libraries into any 
/lib/x86_64-linux-gnux32-like path. They are intended to allow x32 cross 
compilation on an Ubuntu system, not intended to act as a basis for running an 
x32 Ubuntu system. This appears to be very different from Debian's x32 support. 
That said, cross-compiled binaries do run on Ubuntu and it should be possible 
to build an x32-native LLVM with the Ubuntu-provided toolchain.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D52050

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

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

Ping 2


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91913

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

[PATCH] D52050: [Driver] Fix architecture triplets and search paths for Linux x32

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D52050#2466874 , @glaubitz wrote:

> However, that's not the same as whether we're on an x86_64 system or on an 
> x32 system determines which GNU triplet to use and which include and library 
> search paths are our primary ones.

I don't see why it should?
When you target i386-linux-gnu, you can use objects from 
/usr/lib/gcc/i386-linux-gnu/*, /usr/lib/gcc/x86_64-linux-gnu/*/32, and 
/usr/lib/gcc/x86_64-linux-gnux32/*/32.
When you target x86_64-linux-gnu, you can use objects from 
/usr/lib/gcc/x86_64-linux-gnu/*, /usr/lib/gcc/x86_64-linux-gnux32/*/64, and 
/usr/lib/gcc/i386-linux-gnu/*/64.
When you target x86_64-linux-gnux32, you can use objects from 
/usr/lib/gcc/x86_64-linux-gnux32/*, /usr/lib/gcc/x86_64-linux-gnu/*/x32, and 
/usr/lib/gcc/i386-linux-gnu/*/x32.
All cases should be read to include all variants of the triples as well. None 
of that depends on the LLVM host architecture, does it? The LLVM host 
architecture makes it more or less likely that a GCC installation will be found 
in any of the listed directories, but not how it should be handled if found.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D52050

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

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

[Harald van Dijk via Phabricator via cfe-commits]

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

The GNU token paste extension that removes the comma in , ## __VA_ARGS__ 
conflicts with C99/C++11's requirements when a variadic macro has no named 
parameters: according to the standard, an invocation as FOO() gives it a single 
empty argument, and concatenation of anything with an empty argument is 
well-defined. For this reason, the GNU extension was already disabled in C99 
standard-conforming mode. It was not yet disabled in C++11 standard-conforming 
mode.

The associated comment suggested that GCC keeps this extension enabled in 
C90/C++03 standard-conforming mode, but it actually does not, so rather than 
adding a check for C++ language version, this change simply removes the check 
for C language version.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91913

Files:
  clang/lib/Lex/TokenLexer.cpp
  clang/test/Preprocessor/macro_fn_comma_swallow2.c


Index: clang/test/Preprocessor/macro_fn_comma_swallow2.c
===
--- clang/test/Preprocessor/macro_fn_comma_swallow2.c
+++ clang/test/Preprocessor/macro_fn_comma_swallow2.c
@@ -1,9 +1,12 @@
 // Test the __VA_ARGS__ comma swallowing extensions of various compiler 
dialects.
 
 // RUN: %clang_cc1 -E %s | FileCheck -check-prefix=GCC -strict-whitespace %s
+// RUN: %clang_cc1 -E -std=c90 %s | FileCheck -check-prefix=C99 
-strict-whitespace %s
 // RUN: %clang_cc1 -E -std=c99 %s | FileCheck -check-prefix=C99 
-strict-whitespace %s
 // RUN: %clang_cc1 -E -std=c11 %s | FileCheck -check-prefix=C99 
-strict-whitespace %s
 // RUN: %clang_cc1 -E -x c++ %s | FileCheck -check-prefix=GCC 
-strict-whitespace %s
+// RUN: %clang_cc1 -E -x c++ -std=c++03 %s | FileCheck -check-prefix=C99 
-strict-whitespace %s
+// RUN: %clang_cc1 -E -x c++ -std=c++11 %s | FileCheck -check-prefix=C99 
-strict-whitespace %s
 // RUN: %clang_cc1 -E -std=gnu99 %s | FileCheck -check-prefix=GCC 
-strict-whitespace %s
 // RUN: %clang_cc1 -E -fms-compatibility %s | FileCheck -check-prefix=MS 
-strict-whitespace %s
 // RUN: %clang_cc1 -E -DNAMED %s | FileCheck -check-prefix=GCC 
-strict-whitespace %s
Index: clang/lib/Lex/TokenLexer.cpp
===
--- clang/lib/Lex/TokenLexer.cpp
+++ clang/lib/Lex/TokenLexer.cpp
@@ -148,12 +148,11 @@
 return false;
 
   // GCC removes the comma in the expansion of " ... , ## __VA_ARGS__ " if
-  // __VA_ARGS__ is empty, but not in strict C99 mode where there are no
-  // named arguments, where it remains.  In all other modes, including C99
-  // with GNU extensions, it is removed regardless of named arguments.
+  // __VA_ARGS__ is empty, but not in strict mode where there are no
+  // named arguments, where it remains.  With GNU extensions, it is removed
+  // regardless of named arguments.
   // Microsoft also appears to support this extension, unofficially.
-  if (PP.getLangOpts().C99 && !PP.getLangOpts().GNUMode
-&& Macro->getNumParams() < 2)
+  if (!PP.getLangOpts().GNUMode && Macro->getNumParams() < 2)
 return false;
 
   // Is a comma available to be removed?


Index: clang/test/Preprocessor/macro_fn_comma_swallow2.c
===
--- clang/test/Preprocessor/macro_fn_comma_swallow2.c
+++ clang/test/Preprocessor/macro_fn_comma_swallow2.c
@@ -1,9 +1,12 @@
 // Test the __VA_ARGS__ comma swallowing extensions of various compiler dialects.
 
 // RUN: %clang_cc1 -E %s | FileCheck -check-prefix=GCC -strict-whitespace %s
+// RUN: %clang_cc1 -E -std=c90 %s | FileCheck -check-prefix=C99 -strict-whitespace %s
 // RUN: %clang_cc1 -E -std=c99 %s | FileCheck -check-prefix=C99 -strict-whitespace %s
 // RUN: %clang_cc1 -E -std=c11 %s | FileCheck -check-prefix=C99 -strict-whitespace %s
 // RUN: %clang_cc1 -E -x c++ %s | FileCheck -check-prefix=GCC -strict-whitespace %s
+// RUN: %clang_cc1 -E -x c++ -std=c++03 %s | FileCheck -check-prefix=C99 -strict-whitespace %s
+// RUN: %clang_cc1 -E -x c++ -std=c++11 %s | FileCheck -check-prefix=C99 -strict-whitespace %s
 // RUN: %clang_cc1 -E -std=gnu99 %s | FileCheck -check-prefix=GCC -strict-whitespace %s
 // RUN: %clang_cc1 -E -fms-compatibility %s | FileCheck -check-prefix=MS -strict-whitespace %s
 // RUN: %clang_cc1 -E -DNAMED %s | FileCheck -check-prefix=GCC -strict-whitespace %s
Index: clang/lib/Lex/TokenLexer.cpp
===
--- clang/lib/Lex/TokenLexer.cpp
+++ clang/lib/Lex/TokenLexer.cpp
@@ -148,12 +148,11 @@
 return false;
 
   // GCC removes the comma in the expansion of " ... , ## __VA_ARGS__ " if
-  // __VA_ARGS__ is empty, but not in strict C99 mode where there are no
-  // named arguments, where it remains.  In all other modes, including C99
-  // with GNU extensions, it is removed regardle

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D91913#2409688 , @lebedev.ri wrote:

> Then the comment needs to be fixed too i would think?

It's the comment right above the code that I changed; I did fix that too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91913

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

[PATCH] D52050: [Driver] Fix architecture triplets and search paths for Linux x32

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D52050#2415723 , @glaubitz wrote:

> Yeah, @hvdijk has made multiple other improvements which should finally allow 
> the backend to be usable.
>
> We still disagree on the search paths for libraries and headers though if I 
> remember correctly.

No disagreement. I hadn't done anything in this area only because my non-Debian 
system doesn't need it, not because I'm in any way opposed to this change. My 
knowledge of the Debian file system layout is a bit limited, but this change 
looks to me like it makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D52050

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

[PATCH] D52050: [Driver] Fix architecture triplets and search paths for Linux x32

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D52050#2428560 , @glaubitz wrote:

> Any idea how it could make clang default to `-mx32` if this condition is met: 
> `getToolChain().getTriple().getEnvironment() == llvm::Triple::GNUX32`?

That sounds like it's one thing in 
https://lists.llvm.org/pipermail/llvm-dev/2020-October/146049.html I haven't 
extracted into a separate patch for submission yet: LLVM does not call 
`Triple::normalize` everywhere it should, and misinterprets x86_64-linux-gnux32 
as x86_64-somethingelse when it's set as the default target because of it. For 
testing this change, it may be easiest to just explicitly specify the triple or 
`-mx32`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D52050

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

[PATCH] D52050: [Driver] Fix architecture triplets and search paths for Linux x32

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D52050#2428693 , @glaubitz wrote:

> Maybe `sys::getDefaultTargetTriple()` needs to be adjusted. I'll have a look.

`sys::getDefaultTargetTriple()` defaults to `LLVM_DEFAULT_TARGET_TRIPLE`, which 
in turn defaults to `LLVM_HOST_TRIPLE`, which in turn defaults to 
`LLVM_INFERRED_HOST_TRIPLE`, which calls `config.guess` which never 
automatically detects X32. Sorry, forgot to mention that on my system, I also 
make sure to explicitly specify `LLVM_HOST_TRIPLE` to an X32 triple when 
building LLVM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D52050

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

[PATCH] D52050: [Driver] Fix architecture triplets and search paths for Linux x32

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D52050#2428735 , @glaubitz wrote:

> Hmm, I was pretty sure that autoconf can deal with x32 inside an x32 chroot.

Most autoconf-using software won't be dealing with a copy of `config.guess` 
that was last updated in 2011. Updating that to a more recent version should 
also work. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D52050

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

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91913

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

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

Ping 4


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91913

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

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

[Harald van Dijk 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 rGf4537935dcdb: Suppress non-conforming GNU paste extension in 
all standard-conforming modes (authored by hvdijk).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91913

Files:
  clang/lib/Lex/TokenLexer.cpp
  clang/test/Preprocessor/macro_fn_comma_swallow2.c


Index: clang/test/Preprocessor/macro_fn_comma_swallow2.c
===
--- clang/test/Preprocessor/macro_fn_comma_swallow2.c
+++ clang/test/Preprocessor/macro_fn_comma_swallow2.c
@@ -1,9 +1,12 @@
 // Test the __VA_ARGS__ comma swallowing extensions of various compiler 
dialects.
 
 // RUN: %clang_cc1 -E %s | FileCheck -check-prefix=GCC -strict-whitespace %s
+// RUN: %clang_cc1 -E -std=c90 %s | FileCheck -check-prefix=C99 
-strict-whitespace %s
 // RUN: %clang_cc1 -E -std=c99 %s | FileCheck -check-prefix=C99 
-strict-whitespace %s
 // RUN: %clang_cc1 -E -std=c11 %s | FileCheck -check-prefix=C99 
-strict-whitespace %s
 // RUN: %clang_cc1 -E -x c++ %s | FileCheck -check-prefix=GCC 
-strict-whitespace %s
+// RUN: %clang_cc1 -E -x c++ -std=c++03 %s | FileCheck -check-prefix=C99 
-strict-whitespace %s
+// RUN: %clang_cc1 -E -x c++ -std=c++11 %s | FileCheck -check-prefix=C99 
-strict-whitespace %s
 // RUN: %clang_cc1 -E -std=gnu99 %s | FileCheck -check-prefix=GCC 
-strict-whitespace %s
 // RUN: %clang_cc1 -E -fms-compatibility %s | FileCheck -check-prefix=MS 
-strict-whitespace %s
 // RUN: %clang_cc1 -E -DNAMED %s | FileCheck -check-prefix=GCC 
-strict-whitespace %s
Index: clang/lib/Lex/TokenLexer.cpp
===
--- clang/lib/Lex/TokenLexer.cpp
+++ clang/lib/Lex/TokenLexer.cpp
@@ -148,12 +148,11 @@
 return false;
 
   // GCC removes the comma in the expansion of " ... , ## __VA_ARGS__ " if
-  // __VA_ARGS__ is empty, but not in strict C99 mode where there are no
-  // named arguments, where it remains.  In all other modes, including C99
-  // with GNU extensions, it is removed regardless of named arguments.
+  // __VA_ARGS__ is empty, but not in strict mode where there are no
+  // named arguments, where it remains.  With GNU extensions, it is removed
+  // regardless of named arguments.
   // Microsoft also appears to support this extension, unofficially.
-  if (PP.getLangOpts().C99 && !PP.getLangOpts().GNUMode
-&& Macro->getNumParams() < 2)
+  if (!PP.getLangOpts().GNUMode && Macro->getNumParams() < 2)
 return false;
 
   // Is a comma available to be removed?


Index: clang/test/Preprocessor/macro_fn_comma_swallow2.c
===
--- clang/test/Preprocessor/macro_fn_comma_swallow2.c
+++ clang/test/Preprocessor/macro_fn_comma_swallow2.c
@@ -1,9 +1,12 @@
 // Test the __VA_ARGS__ comma swallowing extensions of various compiler dialects.
 
 // RUN: %clang_cc1 -E %s | FileCheck -check-prefix=GCC -strict-whitespace %s
+// RUN: %clang_cc1 -E -std=c90 %s | FileCheck -check-prefix=C99 -strict-whitespace %s
 // RUN: %clang_cc1 -E -std=c99 %s | FileCheck -check-prefix=C99 -strict-whitespace %s
 // RUN: %clang_cc1 -E -std=c11 %s | FileCheck -check-prefix=C99 -strict-whitespace %s
 // RUN: %clang_cc1 -E -x c++ %s | FileCheck -check-prefix=GCC -strict-whitespace %s
+// RUN: %clang_cc1 -E -x c++ -std=c++03 %s | FileCheck -check-prefix=C99 -strict-whitespace %s
+// RUN: %clang_cc1 -E -x c++ -std=c++11 %s | FileCheck -check-prefix=C99 -strict-whitespace %s
 // RUN: %clang_cc1 -E -std=gnu99 %s | FileCheck -check-prefix=GCC -strict-whitespace %s
 // RUN: %clang_cc1 -E -fms-compatibility %s | FileCheck -check-prefix=MS -strict-whitespace %s
 // RUN: %clang_cc1 -E -DNAMED %s | FileCheck -check-prefix=GCC -strict-whitespace %s
Index: clang/lib/Lex/TokenLexer.cpp
===
--- clang/lib/Lex/TokenLexer.cpp
+++ clang/lib/Lex/TokenLexer.cpp
@@ -148,12 +148,11 @@
 return false;
 
   // GCC removes the comma in the expansion of " ... , ## __VA_ARGS__ " if
-  // __VA_ARGS__ is empty, but not in strict C99 mode where there are no
-  // named arguments, where it remains.  In all other modes, including C99
-  // with GNU extensions, it is removed regardless of named arguments.
+  // __VA_ARGS__ is empty, but not in strict mode where there are no
+  // named arguments, where it remains.  With GNU extensions, it is removed
+  // regardless of named arguments.
   // Microsoft also appears to support this extension, unofficially.
-  if (PP.getLangOpts().C99 && !PP.getLangOpts().GNUMode
-&& Macro->getNumParams() < 2)
+  if (!PP.getLangOpts().GNUMode && Macro->getNumParams() < 2)
 return false;
 
   // Is a comma available to be removed?
___
cfe-commits mailing list
cf

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D91913#2518448 , @rsmith wrote:

> Looks like this change matches the behavior of GCC all the way back to at 
> least GCC 4.1.

Yes, it's worked this way in GCC ever since it was first introduced back when 
3.3 was in development: 
https://github.com/gcc-mirror/gcc/commit/58551c2335b3f2f16c8341d3bb409d37f8715696.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91913

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

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D91913#2520244 , @dmajor wrote:

> We have a downstream build break due to this commit. One of our files has 
> some convoluted arg-counting logic that now returns a different count, which 
> does not match gcc: https://godbolt.org/z/W8caMr (Note: At time of writing, 
> the clang trunk on godbolt doesn't yet have this change.)

I get `int nArgs = 1;` with GCC and with clang (with this change) in 
`-std=c++11` mode, I get `int nArgs = 0;` with GCC and with clang (with this 
change) in `-std=gnu++11` mode and in default mode. Which command line options 
are needed to get output where GCC and clang differ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91913

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

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

Okay, GCC enables the standards-mandated behaviour with `-std=c++17 
-fms-extensions` as well, but GCC doesn't have any `-fms-compatibility` option 
to compare. It makes sense to me for `clang -std=c++17 -fms-compatibility` to 
match MSVC, and MSVC supports this GCC extension even in those cases where it 
conflicts with the standard, so I'll try to submit a patch to restore this, 
conditional on `-fms-compatibility`, later today.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91913

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

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D91913#2520399 , @zequanwu wrote:

> This change also breaks many chrome ToT bots(not just windows bot), example 
> build: 
> https://ci.chromium.org/ui/p/chrome/builders/ci/ToTLinuxOfficial/10524/steps?succeeded=true&debug=false

That requires an account to see.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91913

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

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D91913#2520432 , @zequanwu wrote:

> In D91913#2520414 , @hvdijk wrote:
>
>> In D91913#2520399 , @zequanwu wrote:
>>
>>> This change also breaks many chrome ToT bots(not just windows bot), example 
>>> build: 
>>> https://ci.chromium.org/ui/p/chrome/builders/ci/ToTLinuxOfficial/10524/steps?succeeded=true&debug=false
>>
>> That requires an account to see.
>
> This should be accessible without an account: 
> https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8857139118288337920/+/steps/compile/0/stdout

Indeed. However, here, this seems to be a user problem. Chrome `-std=c++*` when 
building with clang, but `-std=gnu++*` when building with GCC for exactly this 
reason, see build/config/compiler/BUILD.gn. Chrome should use `-std=gnu++*` 
with clang too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91913

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

[PATCH] D95392: Restore GNU , ## __VA_ARGS__ behavior in MSVC mode too

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk created this revision.
hvdijk added a reviewer: rsmith.
hvdijk added a project: LLVM.
hvdijk requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

As noted in D91913 , MSVC implements the GNU 
behavior for , ## __VA_ARGS__ as well. Do the same when `-fms-compatibility` is 
used.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D95392

Files:
  clang/lib/Lex/TokenLexer.cpp
  clang/test/Preprocessor/macro_fn_comma_swallow2.c


Index: clang/test/Preprocessor/macro_fn_comma_swallow2.c
===
--- clang/test/Preprocessor/macro_fn_comma_swallow2.c
+++ clang/test/Preprocessor/macro_fn_comma_swallow2.c
@@ -9,6 +9,8 @@
 // RUN: %clang_cc1 -E -x c++ -std=c++11 %s | FileCheck -check-prefix=C99 
-strict-whitespace %s
 // RUN: %clang_cc1 -E -std=gnu99 %s | FileCheck -check-prefix=GCC 
-strict-whitespace %s
 // RUN: %clang_cc1 -E -fms-compatibility %s | FileCheck -check-prefix=MS 
-strict-whitespace %s
+// RUN: %clang_cc1 -E -x c++ -fms-compatibility %s | FileCheck 
-check-prefix=MS -strict-whitespace %s
+// RUN: %clang_cc1 -E -x c++ -std=c++11 -fms-compatibility %s | FileCheck 
-check-prefix=MS -strict-whitespace %s
 // RUN: %clang_cc1 -E -DNAMED %s | FileCheck -check-prefix=GCC 
-strict-whitespace %s
 // RUN: %clang_cc1 -E -std=c99 -DNAMED %s | FileCheck -check-prefix=C99 
-strict-whitespace %s
 
Index: clang/lib/Lex/TokenLexer.cpp
===
--- clang/lib/Lex/TokenLexer.cpp
+++ clang/lib/Lex/TokenLexer.cpp
@@ -152,7 +152,8 @@
   // named arguments, where it remains.  With GNU extensions, it is removed
   // regardless of named arguments.
   // Microsoft also appears to support this extension, unofficially.
-  if (!PP.getLangOpts().GNUMode && Macro->getNumParams() < 2)
+  if (!PP.getLangOpts().GNUMode && !PP.getLangOpts().MSVCCompat &&
+  Macro->getNumParams() < 2)
 return false;
 
   // Is a comma available to be removed?


Index: clang/test/Preprocessor/macro_fn_comma_swallow2.c
===
--- clang/test/Preprocessor/macro_fn_comma_swallow2.c
+++ clang/test/Preprocessor/macro_fn_comma_swallow2.c
@@ -9,6 +9,8 @@
 // RUN: %clang_cc1 -E -x c++ -std=c++11 %s | FileCheck -check-prefix=C99 -strict-whitespace %s
 // RUN: %clang_cc1 -E -std=gnu99 %s | FileCheck -check-prefix=GCC -strict-whitespace %s
 // RUN: %clang_cc1 -E -fms-compatibility %s | FileCheck -check-prefix=MS -strict-whitespace %s
+// RUN: %clang_cc1 -E -x c++ -fms-compatibility %s | FileCheck -check-prefix=MS -strict-whitespace %s
+// RUN: %clang_cc1 -E -x c++ -std=c++11 -fms-compatibility %s | FileCheck -check-prefix=MS -strict-whitespace %s
 // RUN: %clang_cc1 -E -DNAMED %s | FileCheck -check-prefix=GCC -strict-whitespace %s
 // RUN: %clang_cc1 -E -std=c99 -DNAMED %s | FileCheck -check-prefix=C99 -strict-whitespace %s
 
Index: clang/lib/Lex/TokenLexer.cpp
===
--- clang/lib/Lex/TokenLexer.cpp
+++ clang/lib/Lex/TokenLexer.cpp
@@ -152,7 +152,8 @@
   // named arguments, where it remains.  With GNU extensions, it is removed
   // regardless of named arguments.
   // Microsoft also appears to support this extension, unofficially.
-  if (!PP.getLangOpts().GNUMode && Macro->getNumParams() < 2)
+  if (!PP.getLangOpts().GNUMode && !PP.getLangOpts().MSVCCompat &&
+  Macro->getNumParams() < 2)
 return false;
 
   // Is a comma available to be removed?
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D91913#2521031 , @thakis wrote:

> We (and every other project out there) needs some incremental rollout plan 
> for this.

Cut the hyperbole please, this change does not affect "every other project out 
there". This change makes clang match GCC's 15+-year behaviour that other 
projects out there will have come to expect. I find it baffling that 
Chrome/Chromium developers would rely on a bug, knowing full well it's a bug 
judging from their own comments, and then not take responsibility for the 
breakage when that bug is fixed. However, I'll defer to @rsmith on whether this 
is sufficient reason for reverting, I do not feel qualified to make that 
judgement.

The MSVC compatibility, however, is something I hadn't foreseen and had just 
submitted a fix for, D95392 . If this change 
is to be reverted, I'll withdraw that followup fix for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91913

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

[PATCH] D95392: Restore GNU , ## __VA_ARGS__ behavior in MSVC mode

[Harald van Dijk via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb43c26d036dc: Restore GNU , ## __VA_ARGS__ behavior in MSVC 
mode (authored by hvdijk).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95392

Files:
  clang/lib/Lex/TokenLexer.cpp
  clang/test/Preprocessor/macro_fn_comma_swallow2.c


Index: clang/test/Preprocessor/macro_fn_comma_swallow2.c
===
--- clang/test/Preprocessor/macro_fn_comma_swallow2.c
+++ clang/test/Preprocessor/macro_fn_comma_swallow2.c
@@ -9,6 +9,8 @@
 // RUN: %clang_cc1 -E -x c++ -std=c++11 %s | FileCheck -check-prefix=C99 
-strict-whitespace %s
 // RUN: %clang_cc1 -E -std=gnu99 %s | FileCheck -check-prefix=GCC 
-strict-whitespace %s
 // RUN: %clang_cc1 -E -fms-compatibility %s | FileCheck -check-prefix=MS 
-strict-whitespace %s
+// RUN: %clang_cc1 -E -x c++ -fms-compatibility %s | FileCheck 
-check-prefix=MS -strict-whitespace %s
+// RUN: %clang_cc1 -E -x c++ -std=c++11 -fms-compatibility %s | FileCheck 
-check-prefix=MS -strict-whitespace %s
 // RUN: %clang_cc1 -E -DNAMED %s | FileCheck -check-prefix=GCC 
-strict-whitespace %s
 // RUN: %clang_cc1 -E -std=c99 -DNAMED %s | FileCheck -check-prefix=C99 
-strict-whitespace %s
 
Index: clang/lib/Lex/TokenLexer.cpp
===
--- clang/lib/Lex/TokenLexer.cpp
+++ clang/lib/Lex/TokenLexer.cpp
@@ -152,7 +152,8 @@
   // named arguments, where it remains.  With GNU extensions, it is removed
   // regardless of named arguments.
   // Microsoft also appears to support this extension, unofficially.
-  if (!PP.getLangOpts().GNUMode && Macro->getNumParams() < 2)
+  if (!PP.getLangOpts().GNUMode && !PP.getLangOpts().MSVCCompat &&
+  Macro->getNumParams() < 2)
 return false;
 
   // Is a comma available to be removed?


Index: clang/test/Preprocessor/macro_fn_comma_swallow2.c
===
--- clang/test/Preprocessor/macro_fn_comma_swallow2.c
+++ clang/test/Preprocessor/macro_fn_comma_swallow2.c
@@ -9,6 +9,8 @@
 // RUN: %clang_cc1 -E -x c++ -std=c++11 %s | FileCheck -check-prefix=C99 -strict-whitespace %s
 // RUN: %clang_cc1 -E -std=gnu99 %s | FileCheck -check-prefix=GCC -strict-whitespace %s
 // RUN: %clang_cc1 -E -fms-compatibility %s | FileCheck -check-prefix=MS -strict-whitespace %s
+// RUN: %clang_cc1 -E -x c++ -fms-compatibility %s | FileCheck -check-prefix=MS -strict-whitespace %s
+// RUN: %clang_cc1 -E -x c++ -std=c++11 -fms-compatibility %s | FileCheck -check-prefix=MS -strict-whitespace %s
 // RUN: %clang_cc1 -E -DNAMED %s | FileCheck -check-prefix=GCC -strict-whitespace %s
 // RUN: %clang_cc1 -E -std=c99 -DNAMED %s | FileCheck -check-prefix=C99 -strict-whitespace %s
 
Index: clang/lib/Lex/TokenLexer.cpp
===
--- clang/lib/Lex/TokenLexer.cpp
+++ clang/lib/Lex/TokenLexer.cpp
@@ -152,7 +152,8 @@
   // named arguments, where it remains.  With GNU extensions, it is removed
   // regardless of named arguments.
   // Microsoft also appears to support this extension, unofficially.
-  if (!PP.getLangOpts().GNUMode && Macro->getNumParams() < 2)
+  if (!PP.getLangOpts().GNUMode && !PP.getLangOpts().MSVCCompat &&
+  Macro->getNumParams() < 2)
 return false;
 
   // Is a comma available to be removed?
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

Restoring the old behaviour for LLVM 12 may or may not help the Chrome/Chromium 
people: if they also regularly build with clang from git, they would need to 
handle this change anyway. However, if it does help, then it sounds like a good 
idea to me.

> So we should be working towards removing the `getNumParams() < 2` check in at 
> least the C++20 case.

Right, and the same change has also been accepted for C, has it not? If it has, 
then also the C2x case.

It would be good to align with GCC here. I would imagine that they will want 
the same thing for GCC you are proposing for clang, but it would be good to 
make sure. Specifically: it would be good to confirm that there are no 
objections from GCC's side to supporting `#ifdef __VA_OPT__` as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91913

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

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

@rnk Taking it upon yourself to revert this without approval and without 
communication on all branches, especially given the earlier suggestion by 
@rsmith to only revert this on the LLVM 12 branch, is an abuse of your commit 
privileges as far as I am concerned.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91913

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

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D91913#2526117 , @rsmith wrote:

> I think @rnk's observation that `__VA_OPT__` isn't actually available in any 
> compilation mode other than C++20 (which I hadn't previously realized) is 
> important here: we'd left longstanding users of this functionality with no 
> path forward, and that is, I think, sufficient motivation for a revert.

My concern isn't with the revert itself, it's without waiting for approval. 
That's a crystal clear LLVM developer policy violation: changes need to be 
approved, or obvious, or by developers responsible for the code being modified. 
@rnk himself has linked to the page making this clear, just before the bit he 
linked to: 
https://llvm.org/docs/CodeReview.html#must-code-be-reviewed-prior-to-being-committed.
 The fact that reviews don't close after committing does not change that.

"Longstanding users" is not right: there has not been any indication that this 
affected anyone other than Google. (Except for the MSVC compatibility issue.)
"No path forward" is not right either: Google already had multiple paths 
forward.

- Google could have reverted this on the LLVM 12 branch and worked 
constructively to help resolve this on the main branch in a way that also works 
for them.
- Google could have temporarily switched to -std=gnu++* and worked 
constructively to help resolve this on both branches in a way that also works 
for them.
- Google could modify their own LLVM version and use -std=c++* on that version, 
or -std=gnu++* with unmodified LLVM. (I would not suggest this for ordinary 
users, but my understanding is that Google already have their own version of 
clang that they normally use. My understanding may be wrong.)
- Google could have chosen to always build in -std=c++20 mode.

And most importantly:

- Google could have fixed their code. I've looked at how this is used. I'm 
about 80-90% sure it's possible to change the macros so that this extension is 
no longer needed, *without* using `__VA_OPT__`, with an admittedly very ugly 
use of the preprocessor. That ugly use could be conditional on the language 
version, using a clean `__VA_OPT__` version in C++20 mode, and the ugly version 
in older modes.

In D91913#2526171 , @rsmith wrote:

> rG0436ec2128c9775ba13b0308937238fc79673fdd 
>  enables 
> `__VA_OPT__` across language modes and allows support for it to be detected 
> by `#ifdef`.

I've since found that C++20 requires a diagnostic for `#ifdef __VA_OPT__`: it 
violates [cpp.replace]p6: "The identifiers __VA_ARGS__ and __VA_OPT__ shall 
occur only in the replacement-list of a function-like macro that uses the 
ellipsis notation in the parameters." It is a hard error with GCC under 
`-pedantic-errors` and given that this hard error is required by the standard, 
I expect that hard error to remain until/unless the standard is modified to 
lift that restriction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91913

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

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D91913#2526403 , @rnk wrote:

> Anyway, I apologize for the misunderstanding. I'm doing my best to operate in 
> good faith with LLVM project policies. Hopefully you feel that you have a 
> path forward here.

Thank you, I appreciate that.

> If that's the case, I believe I asked for options. I'm open to suggestions, 
> and I'm not trying to leave you with a passive-aggressive "patches welcome" 
> offer where you do all the work. I'm truly not aware of how we would make 
> this code conforming. Maybe there is a way that I'm unaware of.

Please take a look at Jens Gustedt's "Detect empty macro arguments": 
https://gustedt.wordpress.com/2010/06/08/detect-empty-macro-arguments/

It cannot function as a general solution for all possible uses of `, ## 
__VA_ARGS__`, because it fails to handle some cases that users of `, ## 
__VA_ARGS__` do expect to be handled (more than listed in that blog post), but 
from what I have seen it should handle what Chromium uses. This can either be 
adapted to directly expand to what Chromium needs, or can be used combined with 
two other macros can be defined, call them `A_0` and `A_1` for simplicity, and 
`CONCAT(A_, ISEMPTY(__VA_ARGS__))(__VA_ARGS__)` (with the obvious definition of 
`CONCAT`) would expand either `A_0(__VA_ARGS__)` or `A_1(__VA_ARGS__)`, where 
`A_1` would leave out a comma that `A_0` would include.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91913

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

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D91913#2526866 , @dmajor wrote:

> If I'm reading git correctly, the change is still present on the 12.x branch. 
> Should it be reverted there too?

I could have sworn that I saw it already reverted on the 12.x branch too, but I 
don't know what it was that I was looking at. It doesn't make sense to me to 
leave it applied on 12.x but not have it applied on main. Unless it can be 
reapplied on main, yes, I think it should be reverted on the 12.x branch.

And I might as well include this here for the benefit of anyone else affected 
by this in the future: adapting Jens Gustedt's trick just a little bit, given 
the below, `MAYBE_COMMA(__VA_ARGS__) __VA_ARGS__` is pretty much a drop-in 
replacement for `__VA_OPT__(,) __VA_ARGS__`, assuming no odd macro arguments 
are used. It's not exactly the same thing as `, ## __VA_ARGS__`, but generally 
close enough.

  #define ARG128(  \
 _0,   _1,   _2,   _3,   _4,   _5,   _6,   _7, \
 _8,   _9,  _10,  _11,  _12,  _13,  _14,  _15, \
_16,  _17,  _18,  _19,  _20,  _21,  _22,  _23, \
_24,  _25,  _26,  _27,  _28,  _29,  _30,  _31, \
_32,  _33,  _34,  _35,  _36,  _37,  _38,  _39, \
_40,  _41,  _42,  _43,  _44,  _45,  _46,  _47, \
_48,  _49,  _50,  _51,  _52,  _53,  _54,  _55, \
_56,  _57,  _58,  _59,  _60,  _61,  _62,  _63, \
_64,  _65,  _66,  _67,  _68,  _69,  _70,  _71, \
_72,  _73,  _74,  _75,  _76,  _77,  _78,  _79, \
_80,  _81,  _82,  _83,  _84,  _85,  _86,  _87, \
_88,  _89,  _90,  _91,  _92,  _93,  _94,  _95, \
_96,  _97,  _98,  _99, _100, _101, _102, _103, \
   _104, _105, _106, _107, _108, _109, _110, _111, \
   _112, _113, _114, _115, _116, _117, _118, _119, \
   _120, _121, _122, _123, _124, _125, _126, _127, \
   ...) _127
  
  #define IF_HAS_COMMA(a, b, ...) \
   /* Expands to a if __VA_ARGS__ contains a comma, b otherwise. */ \
   ARG128(__VA_ARGS__   \
a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, \
a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, \
a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, \
a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, \
a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, \
a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, \
a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, \
a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, b,)
  
  #define CONCAT_5_(a, b, c, d, e) a ## b ## c ## d ## e
  #define CONCAT_5(a, b, c, d, e) CONCAT_5_(a, b, c, d, e)
  
  #define COMMA(...) ,
  #define EMPTY(...)
  
  #define MAYBE_COMMA_1000 ,
  #define MAYBE_COMMA(...) \
/* Expands to , if __VA_ARGS__ is non-empty, nothing otherwise. */ \
IF_HAS_COMMA(EMPTY, COMMA, CONCAT_5(MAYBE_COMMA_, \
  IF_HAS_COMMA(1, 0, COMMA __VA_ARGS__ ()),   \
  IF_HAS_COMMA(1, 0,   __VA_ARGS__   ),   \
  IF_HAS_COMMA(1, 0, COMMA __VA_ARGS__   ),   \
  IF_HAS_COMMA(1, 0,   __VA_ARGS__ (()


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91913

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

[PATCH] D103777: [X32] Add Triple::isX32(), use it.

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk created this revision.
hvdijk added reviewers: MaskRay, craig.topper.
Herald added subscribers: dexonsmith, pengfei, hiraditya, dschuff.
hvdijk requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

So far, support for x86_64-linux-gnux32 has been handled by explicit
comparisons of Triple.getEnvironment() to GNUX32. This worked as long as
x86_64-linux-gnux32 was the only X32 environment to worry about, but we
now have x86_64-linux-muslx32 as well. To support this, this change adds
an isX32() function and uses it. It replaces all checks for GNUX32 or
MuslX32 by isX32(), except for the following:

- Triple::isGNUEnvironment() and Triple::isMusl() are supposed to treat GNUX32 
and MuslX32 differently.
- computeTargetTriple() needs to be able to transform triples to add or remove 
X32 from the environment and needs to map GNU to GNUX32, and Musl to MuslX32.
- getMultiarchTriple() completely lacks any Musl support and retains the 
explicit check for GNUX32 as it can only return x86_64-linux-gnux32.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D103777

Files:
  clang/lib/Basic/Targets/X86.h
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/Linux.cpp
  clang/test/Driver/linux-cross.cpp
  llvm/include/llvm/ADT/Triple.h
  llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
  llvm/lib/Target/X86/MCTargetDesc/X86MCAsmInfo.cpp
  llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
  llvm/lib/Target/X86/X86AsmPrinter.cpp
  llvm/lib/Target/X86/X86RegisterInfo.cpp
  llvm/lib/Target/X86/X86Subtarget.h
  llvm/lib/Target/X86/X86TargetMachine.cpp
  llvm/test/CodeGen/X86/x32-lea-1.ll

Index: llvm/test/CodeGen/X86/x32-lea-1.ll
===
--- llvm/test/CodeGen/X86/x32-lea-1.ll
+++ llvm/test/CodeGen/X86/x32-lea-1.ll
@@ -1,5 +1,6 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc < %s -mtriple=x86_64-linux-gnux32 -O0 | FileCheck %s
+; RUN: llc < %s -mtriple=x86_64-linux-muslx32 -O0 | FileCheck %s
 
 define void @foo(i32** %p) {
 ; CHECK-LABEL: foo:
Index: llvm/lib/Target/X86/X86TargetMachine.cpp
===
--- llvm/lib/Target/X86/X86TargetMachine.cpp
+++ llvm/lib/Target/X86/X86TargetMachine.cpp
@@ -110,9 +110,7 @@
 
   Ret += DataLayout::getManglingComponent(TT);
   // X86 and x32 have 32 bit pointers.
-  if ((TT.isArch64Bit() &&
-   (TT.getEnvironment() == Triple::GNUX32 || TT.isOSNaCl())) ||
-  !TT.isArch64Bit())
+  if (!TT.isArch64Bit() || TT.isX32() || TT.isOSNaCl())
 Ret += "-p:32:32";
 
   // Address spaces for 32 bit signed, 32 bit unsigned, and 64 bit pointers.
Index: llvm/lib/Target/X86/X86Subtarget.h
===
--- llvm/lib/Target/X86/X86Subtarget.h
+++ llvm/lib/Target/X86/X86Subtarget.h
@@ -610,14 +610,12 @@
 
   /// Is this x86_64 with the ILP32 programming model (x32 ABI)?
   bool isTarget64BitILP32() const {
-return In64BitMode && (TargetTriple.getEnvironment() == Triple::GNUX32 ||
-   TargetTriple.isOSNaCl());
+return In64BitMode && (TargetTriple.isX32() || TargetTriple.isOSNaCl());
   }
 
   /// Is this x86_64 with the LP64 programming model (standard AMD64, no x32)?
   bool isTarget64BitLP64() const {
-return In64BitMode && (TargetTriple.getEnvironment() != Triple::GNUX32 &&
-   !TargetTriple.isOSNaCl());
+return In64BitMode && (!TargetTriple.isX32() && !TargetTriple.isOSNaCl());
   }
 
   PICStyles::Style getPICStyle() const { return PICStyle; }
Index: llvm/lib/Target/X86/X86RegisterInfo.cpp
===
--- llvm/lib/Target/X86/X86RegisterInfo.cpp
+++ llvm/lib/Target/X86/X86RegisterInfo.cpp
@@ -62,7 +62,7 @@
 // This matches the simplified 32-bit pointer code in the data layout
 // computation.
 // FIXME: Should use the data layout?
-bool Use64BitReg = TT.getEnvironment() != Triple::GNUX32;
+bool Use64BitReg = !TT.isX32();
 StackPtr = Use64BitReg ? X86::RSP : X86::ESP;
 FramePtr = Use64BitReg ? X86::RBP : X86::EBP;
 BasePtr = Use64BitReg ? X86::RBX : X86::EBX;
Index: llvm/lib/Target/X86/X86AsmPrinter.cpp
===
--- llvm/lib/Target/X86/X86AsmPrinter.cpp
+++ llvm/lib/Target/X86/X86AsmPrinter.cpp
@@ -643,8 +643,7 @@
   OutStreamer->SwitchSection(Nt);
 
   // Emitting note header.
-  const int WordSize =
-  TT.isArch64Bit() && TT.getEnvironment() != Triple::GNUX32 ? 8 : 4;
+  const int WordSize = TT.isArch64Bit() && !TT.isX32() ? 8 : 4;
   emitAlignment(WordSize == 4 ? Align(4) : Align(8));
   OutStreamer->emitIntValue(4, 4 /*size*/); // data size for "GNU\0"
   OutStreamer->emitIntValue(8 + WordSize, 4 /*si

[PATCH] D103777: [X32] Add Triple::isX32(), use it.

[Harald van Dijk 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 rG75521bd9d8d1: [X32] Add Triple::isX32(), use it. (authored 
by hvdijk).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103777

Files:
  clang/lib/Basic/Targets/X86.h
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/Linux.cpp
  clang/test/Driver/linux-cross.cpp
  llvm/include/llvm/ADT/Triple.h
  llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
  llvm/lib/Target/X86/MCTargetDesc/X86MCAsmInfo.cpp
  llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
  llvm/lib/Target/X86/X86AsmPrinter.cpp
  llvm/lib/Target/X86/X86RegisterInfo.cpp
  llvm/lib/Target/X86/X86Subtarget.h
  llvm/lib/Target/X86/X86TargetMachine.cpp
  llvm/test/CodeGen/X86/x32-lea-1.ll

Index: llvm/test/CodeGen/X86/x32-lea-1.ll
===
--- llvm/test/CodeGen/X86/x32-lea-1.ll
+++ llvm/test/CodeGen/X86/x32-lea-1.ll
@@ -1,5 +1,6 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc < %s -mtriple=x86_64-linux-gnux32 -O0 | FileCheck %s
+; RUN: llc < %s -mtriple=x86_64-linux-muslx32 -O0 | FileCheck %s
 
 define void @foo(i32** %p) {
 ; CHECK-LABEL: foo:
Index: llvm/lib/Target/X86/X86TargetMachine.cpp
===
--- llvm/lib/Target/X86/X86TargetMachine.cpp
+++ llvm/lib/Target/X86/X86TargetMachine.cpp
@@ -110,9 +110,7 @@
 
   Ret += DataLayout::getManglingComponent(TT);
   // X86 and x32 have 32 bit pointers.
-  if ((TT.isArch64Bit() &&
-   (TT.getEnvironment() == Triple::GNUX32 || TT.isOSNaCl())) ||
-  !TT.isArch64Bit())
+  if (!TT.isArch64Bit() || TT.isX32() || TT.isOSNaCl())
 Ret += "-p:32:32";
 
   // Address spaces for 32 bit signed, 32 bit unsigned, and 64 bit pointers.
Index: llvm/lib/Target/X86/X86Subtarget.h
===
--- llvm/lib/Target/X86/X86Subtarget.h
+++ llvm/lib/Target/X86/X86Subtarget.h
@@ -610,14 +610,12 @@
 
   /// Is this x86_64 with the ILP32 programming model (x32 ABI)?
   bool isTarget64BitILP32() const {
-return In64BitMode && (TargetTriple.getEnvironment() == Triple::GNUX32 ||
-   TargetTriple.isOSNaCl());
+return In64BitMode && (TargetTriple.isX32() || TargetTriple.isOSNaCl());
   }
 
   /// Is this x86_64 with the LP64 programming model (standard AMD64, no x32)?
   bool isTarget64BitLP64() const {
-return In64BitMode && (TargetTriple.getEnvironment() != Triple::GNUX32 &&
-   !TargetTriple.isOSNaCl());
+return In64BitMode && (!TargetTriple.isX32() && !TargetTriple.isOSNaCl());
   }
 
   PICStyles::Style getPICStyle() const { return PICStyle; }
Index: llvm/lib/Target/X86/X86RegisterInfo.cpp
===
--- llvm/lib/Target/X86/X86RegisterInfo.cpp
+++ llvm/lib/Target/X86/X86RegisterInfo.cpp
@@ -62,7 +62,7 @@
 // This matches the simplified 32-bit pointer code in the data layout
 // computation.
 // FIXME: Should use the data layout?
-bool Use64BitReg = TT.getEnvironment() != Triple::GNUX32;
+bool Use64BitReg = !TT.isX32();
 StackPtr = Use64BitReg ? X86::RSP : X86::ESP;
 FramePtr = Use64BitReg ? X86::RBP : X86::EBP;
 BasePtr = Use64BitReg ? X86::RBX : X86::EBX;
Index: llvm/lib/Target/X86/X86AsmPrinter.cpp
===
--- llvm/lib/Target/X86/X86AsmPrinter.cpp
+++ llvm/lib/Target/X86/X86AsmPrinter.cpp
@@ -643,8 +643,7 @@
   OutStreamer->SwitchSection(Nt);
 
   // Emitting note header.
-  const int WordSize =
-  TT.isArch64Bit() && TT.getEnvironment() != Triple::GNUX32 ? 8 : 4;
+  const int WordSize = TT.isArch64Bit() && !TT.isX32() ? 8 : 4;
   emitAlignment(WordSize == 4 ? Align(4) : Align(8));
   OutStreamer->emitIntValue(4, 4 /*size*/); // data size for "GNU\0"
   OutStreamer->emitIntValue(8 + WordSize, 4 /*size*/); // Elf_Prop size
Index: llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
===
--- llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
+++ llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
@@ -1231,8 +1231,7 @@
   // FIXME: The caller of determineREXPrefix slaps this prefix onto
   // anything that returns non-zero.
   REX |= 0x40; // REX fixed encoding prefix
-  } else if (MO.isExpr() &&
- STI.getTargetTriple().getEnvironment() == Triple::GNUX32) {
+  } else if (MO.isExpr() && STI.getTargetTriple().isX32()) {
 // GOTTPOFF and TLSDESC relocations require a REX prefix to allow
 // linker optimizations: even if the instructions we see may not requir

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D86310#4648446 , @tmgross wrote:

> Is this just waiting for a review?

Yes, I think so. Valid concerns over compatibility were raised, but now that 
strict compatibility with `i128` has already been broken anyway, I no longer 
believe there is any reason not to just apply this as is, preferably soon so as 
to minimise the time that we have the current partially changed `i128` ABI, to 
minimise the chance that people will start to rely on that somewhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

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

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D86310#4648634 , @nikic wrote:

> I'm happy to sign off on the x86-64 part here, but I'm less sure about 
> x86-32. If I understood correctly, the i128 alignment is raised there 
> exclusively to fix the "f128 legalized to i128 libcall" case -- is there any 
> other ABI requirement for i128 alignment on x86-32? Is raising i128 alignment 
> the right way to fix an f128 issue?

GCC does not support `__int128` on x86-32, but clang has the 
`-fforce-enable-int128` option, and when that is used, it gives it the same 
16-byte alignment that it does on x86-64, so even ignoring the `_Float128` 
issue, I think the change is right for x86-32.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

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

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added inline comments.



Comment at: llvm/lib/IR/AutoUpgrade.cpp:5233
+  SmallVector Groups;
+  Regex R("(.*-i64:64)(-.*)");
+  if (R.match(Res, &Groups))

nikic wrote:
> I don't think this will work for the 32-bit targets that don't have `-i64:64`.
Oh, you're right, thanks. That was intentional but wrong: there is a test that 
we do not upgrade data layout strings that do not look sufficiently close to 
valid, and this was intended to address that. But this also avoids it for data 
layout strings that do need upgrading. I'll have to figure out how to handle 
both; will update when I know how.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

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

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

I do not think there is a sensible way to keep 
[`upgrade-datalayout2.ll`](https://github.com/llvm/llvm-project/blob/main/llvm/test/Bitcode/upgrade-datalayout2.ll)
 working, with the way the upgrade logic is structured, and we should rethink 
that test. The change here intends to insert `-i128:128-` into x86 data layouts 
that do not have it. The goal of `upgrade-datalayout2.ll` is to test that data 
layouts that are not valid x86 data layouts do not get upgraded. However, I see 
no sensible logic by which we can say that in this particular case, we should 
not add it.

What's more, none of the data layout upgrades *ever* checked that the data 
layout was a valid x86 data layout, not even D67631 
 which added this test: it is easy to 
construct data layout strings that are not valid x86 data layout strings, that 
would already be upgraded by that very first version of 
`UpgradeDataLayoutString`, despite what the test claimed to check. So if we 
regard it as a bug to upgrade invalid target data layout strings, this is a 
pre-existing bug. Alternatively, we can choose to not regard it as a bug, and 
instead say the test is invalid. I do not know the rationale here, but given 
that it was explicitly said to be intended to work this way, I am on the side 
of seeing it as a pre-existing bug. One that is nearly impossible to fix in the 
current structure.

Now that there can only be one valid data layout string per target, if it is 
intended that `UpgradeDataLayoutString` only upgrade target-valid data layout 
strings, it is a bug for `UpgradeDataLayoutString` to ever produce anything 
other than 1) its input or 2) the target's one valid data layout string. This 
allows a much simpler implementation that completely fixes the bug, but is too 
big to be part of this change. I would like to propose that in this change, we 
change `UpgradeDataLayoutString` to insert `-i128:128-` including in that one 
test, and we XFAIL `upgrade-datalayout2.ll` since the uncovered bug is not 
actually a new bug. In a followup PR, I can then restructure the 
`UpgradeDataLayoutString` logic by removing the function entirely and instead 
having target functions to check whether a given data layout string is a valid 
historic data layout string for the target that should be upgraded, and if so, 
simply clobbering the data layout string with what the target reports is the 
correct data layout string.

Does that seem reasonable? Am I overlooking anything that would make this a 
non-option? Are there good alternatives that I am not seeing right now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

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

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D86310#4516184 , @tmgross wrote:

> Is the compatibility note here only meant to address calls between LLVM and 
> GCC, not generated code? Because of course, struct layout and pass-in-memory 
> function calls are incompatible.

There should be no compatibility issue there between GCC and clang in most 
cases, because clang ensures __int128 is aligned to 16 bytes everywhere, even 
if the LLVM data layout specifies lower alignment. clang's __int128 and LLVM's 
i128 play by different rules, currently. This change would make them play by 
the same rules.

> Rust just uses LLVM's `i128` value directly so it doesn't necessarily need to 
> be called out on its own (think we are in agreement here, just clarifying)

I do think it needs to be called out on its own: Rust makes its

>> [...]
>> QUESTIONS
>>
>> Is the behaviour of LLVM, clang, GCC, and MSVC indeed as I described, or did 
>> I make a mistake anywhere?
>
> I believe that MSVC is in general ambiguous about these details on types that 
> it does not support, but I would assume that being consistent with the Linux 
> ABI is preferred and probably what MSVC would choose if they ever do decide 
> on a specification for this type (unless LLVM has contact with Microsoft that 
> may be able to clarify? They make no guarantees against breaking things in 
> any case.)
>
>> [...]
>
> It probably makes sense to have reasoning for choosing the selected behavior 
> and having something specific to test against, so I'll link what I know.
>
> - From AMD4 ABI Draft 0.99.7 (2014) 
> :
>
>> [paraphrased from Figure 3.1]
>> type - sizeof - alignment - AMD64 architecture
>> long - 8 - 8 - signed eightbyte [I included this in the table for the below 
>> reference]
>> `__int128` - 16 - 16 - signed sixteenbyte
>> signed `__int128` - 16 - 16 - signed sixteenbyte
>> `long double` - 16 - 16 - 80-bit extended (IEEE-754)
>> `__float128` - 16 - 16 - 128-bit extended (IEEE-754)
>> [...]
>> The `__int128` type is stored in little-endian order in memory, i.e., the 64
>> low-order bits are stored at a a lower address than the 64 high-order bits
>> [...]
>> Arguments of type `__int128` offer the same operations as INTEGERs,
>> yet they do not fit into one general purpose register but require two 
>> registers.
>> For classification purposes `__int128` is treated as if it were implemented
>> as:
>>
>>   typedef struct {
>>   long low, high;
>>   } __int128;
>>
>> with the exception that arguments of type `__int128` that are stored in
>> memory must be aligned on a 16-byte boundary
>
>
>
> - K1OM agrees 
> https://www.intel.com/content/dam/develop/external/us/en/documents/k1om-psabi-1-0.pdf
> - These types don't seem to be mentioned anywhere in i386 1997 
> https://www.sco.com/developers/devspecs/abi386-4.pdf
> - Also not in MIPS RISC 1996 
> https://math-atlas.sourceforge.net/devel/assembly/mipsabi32.pdf
> - MIPSpro64 doesn't mention 128-bit integers but does mention 128-bit floats. 
> From page 24 https://math-atlas.sourceforge.net/devel/assembly/mipsabi64.pdf
>
>> Quad-precision floating point parameters (C long double or Fortran REAL*16) 
>> are
>> always 16-byte aligned. This requires that they be passed in even-odd 
>> floating point
>> register pairs, even if doing so requires skipping a register parameter 
>> and/or a
>> 64-bit save area slot. [The 32-bit ABI does not consider long double 
>> parameters,
>> since they were not supported.]
>
>
>
> - From PPC64 section 3.1.4 
> https://math-atlas.sourceforge.net/devel/assembly/PPC-elf64abi-1.7.pdf:
>
>> [paraphrased from table]
>> type - sizeof - alignment
>> `__int128_t` - 16 - quadword
>> `__uint128_t` - 16 - quadword
>> `long double` - 16 - quadword
>
>
>
> - z/Arch: this is the only target that clang seems to align to 8, see [1]. 
> Also from 1.1.2.4 in https://github.com/IBM/s390x-abi/releases/tag/v1.6:
>
>> [paraphrased from table]
>> type - size (bytes) - alignment
>> `__int128` - 16 - 8
>> signed `__int128` - 16 - 8
>> `long double` - 16 - 8
>
> [1]: https://reviews.llvm.org/D130900




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

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

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D86310#4516876 , @pengfei wrote:

> Just FYI. There are a few reports about the compatibility issues, e.g., 
> #41784 .

Thanks. This is a case where clang breaks up `__int128` into 2x `i64` before it 
gets to LLVM. It is therefore not affected by this patch. Your other link also 
references #20283 , which is 
the same issue of clang breaking `__int128` into 2x `i64`.

Although this patch will not fix those issues, it may make it easier to fix 
them later on: it will give clang the ability to use LLVM's `i128` type rather 
than trying to emulate it.

> There's also concern about the alignment difference between `_BitInt(128)` 
> and `__int128`, see #60925 

That references https://gitlab.com/x86-psABIs/x86-64-ABI/-/issues/11, where the 
answer four months ago was basically "it's probably already too late for that" 
with a suggestion to try and post on the mailing list to try and convince 
others that this was important enough to do. Nothing was posted to the mailing 
list, and by now GCC has started implementing what the ABI specifies 
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102989). I think we would need an 
extraordinary rationale if we want to convince others that the ABI should be 
changed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

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

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D86310#4519549 , @tmgross wrote:

> Does this happen on the clang side or the LLVM side?

Definitely on the clang side, but...

> I built rustc against LLVM with your patch ([link to 
> source](llvm.org/docs/LangRef.html#floating-point-types)) and it makes rustc 
> compatible with clang (progress!) but it still seems not compatible with GCC. 
> That is, after the patch rustc now seems to have an identical calling 
> behavior to clang, so I'm thinking that maybe this behavior lies somewhere in 
> LLVM and not the frontend?

...this suggests that potentially the same thing that clang is doing, LLVM is 
also doing independently. In which case, maybe it would be better to fix that 
at the same time: if we decide that LLVM's `i128` should match `__int128`, I'd 
rather have a single change to the ABI to make it match `__int128`, rather than 
incremental changes, because incremental changes make it more likely that 
someone is going to be using the intermediate version and relying on its ABI. 
It'll be a little while before I can look into this but I'll try to come up 
with a patch to apply on top of this if no one else gets to it first.

> Quick ABI check that demonstrates this 
> https://github.com/tgross35/quick-abi-check, the outputs of note (clang-new 
> is built with this patch):

Thanks, this is useful as an extra test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

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

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D86310#4595996 , @tmgross wrote:

> @nikic posted a patch that fixes the register passing at 
> https://reviews.llvm.org/D158169. I think that patch plus this one should 
> resolve all the problems we have

Thanks for the link, that will save a lot of time. I don't think it will 
resolve all the problems, but that it's a significant additional step in the 
right direction. We also need to make clang use `i128` for `__int128` in order 
to actually use this fixed calling convention.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

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

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D86310#4596712 , @tmgross wrote:

> Is clang still doing something wrong? From my testing, it seems like clang 
> and GCC now agree with each other, I am not sure what would still be incorrect

My understanding is that the code clang generates for `__int128` will still 
allow it to be passed half-in-register, half-in-memory, exactly what D158169 
 sets out to fix, because D158169 
 only fixes it for LLVM's `i128` which clang 
bypasses.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

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

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D86310#4596932 , @tmgross wrote:

> Was your failure in https://bugs.llvm.org/show_bug.cgi?id=50198 fixed with 
> these patches?

Yes, it was (at least it was at the time that I initially commented).

> I cannot reproduce that failure for some reason, but it would likely make a 
> good run-pass test.

It's reproducible online, https://godbolt.org/z/j918EeoMv, it would be 
interesting to know why it does not fail for you.

> These two patches do not seem to fix varargs segfaulting, as documented in 
> https://bugs.llvm.org/show_bug.cgi?id=19909 (testing with this code 
> https://godbolt.org/z/WeE7TvrGe) so it seems like that will need a separate 
> fix.

Thanks, and clang appears to avoid the use of the LLVM `va_arg` instruction 
here; we'll have to make sure to adapt that example to the LLVM IR equivalent 
that does use `va_arg` to make sure that's tested as well, and fixed if needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

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

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added inline comments.



Comment at: llvm/lib/IR/AutoUpgrade.cpp:5233
+  SmallVector Groups;
+  Regex R("(.*-i64:64)(-.*)");
+  if (R.match(Res, &Groups))

hvdijk wrote:
> nikic wrote:
> > I don't think this will work for the 32-bit targets that don't have 
> > `-i64:64`.
> Oh, you're right, thanks. That was intentional but wrong: there is a test 
> that we do not upgrade data layout strings that do not look sufficiently 
> close to valid, and this was intended to address that. But this also avoids 
> it for data layout strings that do need upgrading. I'll have to figure out 
> how to handle both; will update when I know how.
This should now be fixed. X86 data layout strings always have their components 
in the same order, `mpifnaS`, where some may be omitted. I make use of this by 
looking for any leading `-m`/`-p`/`-i` components and inserting `-i128:128` 
after the last of those.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

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

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D86310#4653550 , @tmgross wrote:

> Probably would be good to add https://bugs.llvm.org/show_bug.cgi?id=50198 to 
> the test suite if it isn't there already.

That test would not work as an LLVM test directly, but we do already have lit 
tests that cover that, the test changes in here show the fixed alignment of 
f128 too.

This was ready to push pending @efriedma's approval, who rightly pointed out a 
release note was missing but it was otherwise okay. With the release note now 
added, I think that there is nothing stopping this from being pushed, so I 
intend to do so once I am able to rebase one hopefully last time and re-run 
tests to verify no new tests have been added that also require an update. 
Thanks for the feedback, everyone.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

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

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

The buildbot found one more test that needed updating, that was disabled on my 
system. Created https://github.com/llvm/llvm-project/pull/68781 for that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

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

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D86310#4605475 , @tmgross wrote:

> In D86310#4597359 , @hvdijk wrote:
>
>> In D86310#4596841 , @tmgross wrote:
>>
>>> I think that D158169  seems to have fixed 
>>> clang as well; after applying both patches, clang gcc and rustc all seem to 
>>> agree.
>>
>> Interesting. I cannot see how it would, I may be missing something; I will 
>> check when I am able.
>
> D158169  landed today, I confirmed that the 
> current main (with D158169 ) makes Clang 
> <-> GCC works but LLVM still fails without this patch.

I had hoped to avoid the piecewise ABI breakage, but with that already having 
landed, we already have that anyway, so I no longer see a reason to delay this 
until we can also fix `va_arg`.

> Doesn't clang just wind up going through the same tablegen as LLVM, so it 
> makes sense that both would be fixed?

Actually able to look into this now again, and yes, it does. I was sure I'd 
seen clang expand `__int128` so that at the LLVM level, there was no longer any 
`i128`, but it does not happen here, and because it does not happen here, this 
patch does fix it.

>>> Was your failure in https://bugs.llvm.org/show_bug.cgi?id=50198 fixed with 
>>> these patches?
>>
>> Yes, it was (at least it was at the time that I initially commented).
>
> You mean this patch only right - how does that work? Looking closer at your 
> comments there, it doesn't seem like `i128` changes would affect anything if 
> the `f128` return alignment is the source of the problem.

See the source code comment I quoted in 
https://bugs.llvm.org/show_bug.cgi?id=50198#c3: "If the target does not have 
native f128 support, expand it to i128 and we will be generating soft float 
library calls." This applies to x86. `f128` is expanded to `i128`, so any 
changes to the alignment for `i128` automatically apply to `f128` as well.

In D86310#4516911 , @hvdijk wrote:

> In D86310#4516876 , @pengfei wrote:
>
>> There's also concern about the alignment difference between `_BitInt(128)` 
>> and `__int128`, see #60925 
>> 
>
> That references https://gitlab.com/x86-psABIs/x86-64-ABI/-/issues/11, where 
> the answer four months ago was basically "it's probably already too late for 
> that" with a suggestion to try and post on the mailing list to try and 
> convince others that this was important enough to do. Nothing was posted to 
> the mailing list, and by now GCC has started implementing what the ABI 
> specifies (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102989). I think we 
> would need an extraordinary rationale if we want to convince others that the 
> ABI should be changed.

The discussion has since moved to the list 
(https://groups.google.com/g/x86-64-abi/c/-JeR9HgUU20) and it seems as if the 
alignment of `__int128` is fixed, no changes are planned there; if anything 
changes, it will be the alignment of `_BitInt(128)`, and that will be 
independent of this patch.

Based on this, I now do think again the right course of action is to just 
commit this. It still applies to current LLVM without changes, and passes tests.

The point that is still contentious is the handling of IR generated from older 
versions of LLVM that do not have this patch. Personally, I feel that D158169 
 being accepted already answered how to 
handle this. D158169  clearly broke the ABI 
in LLVM: code generated with the current version of LLVM is not binary 
compatible with code generated with older versions of LLVM. But that is 
considered acceptable when the code generated by these older versions of LLVM 
was buggy and we have no reason to expect that there is code out there that 
relies on that bug remaining unfixed. The same logic applies here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

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

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86-64 datalayout

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D86310#4485837 , @tmgross wrote:

> What is the current status of this patch? Are the reviewers here OK with this 
> fix in general but just need to see changes to autoupgrade?



> @craig.topper or @hvdijk since you worked on it, are you interested in doing 
> these changes, or is this patch in need of new authors?

The `TT.isArch64Bit()` thing I commented on is something I could change, if 
desired, but from my perspective, the suggested changes to the upgrade 
mechanism are an unreasonable amount of work considering the benefit is that it 
keeps already broken code equally broken, so I am not planning on working on 
that, sorry.


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

https://reviews.llvm.org/D86310

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

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86-64 datalayout

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

A thought occurs: in older versions of LLVM, the data layout mechanism worked 
differently and permitted targets to declare that they supported multiple 
different data layout strings, by overriding `isCompatibleDataLayout`. This 
mechanism was removed in D67631 . If we 
reinstate that, we can have the X86 target declare that it "supports" data 
layout strings with and without the `-i128:128`, where by "supports", I mean 
the code continues to not generally work in the same way it does not generally 
work now, but the specific limited cases that do work continue to work exactly 
the same ABI-incompatible way. This would have the same result of bug-for-bug 
compatibility with existing modules, but in what I suspect would be a 
significantly simpler way than by going through the module and adding explicit 
alignments everywhere. While I would still prefer to give up on that 
compatibility, if it is a hard requirement, and if this would be an alternative 
way of achieving it, I might possibly be able to update this patch to do just 
that. Would this be acceptable?


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

https://reviews.llvm.org/D86310

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

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86-64 datalayout

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D86310#4496582 , @nikic wrote:

> The main problem with that is that we can't have multiple data layouts for 
> one module, so linking old and new bitcode together would fail.

Good point, but it's worth pointing out that this only applies to linking in 
the LLVM IR sense. Linking in the ELF object file sense would work exactly as 
it would with the explicit alignments added everywhere, as ELF object files do 
not contain that data layout string. Linking in the LLVM IR sense is what 
happens with `clang -flto` though.

> But maybe that's exactly what we want -- after all, it is incompatible. Even 
> if we "correctly" upgraded to preserve behavior of the old bitcode, it would 
> still be incompatible with the new bitcode if i128 crosses the ABI boundary 
> (explicitly or implicitly).

Yeah, that is a tricky question to answer. Let's say this change goes into LLVM 
17, so LLVM 17 X86 data layouts include `i128:128`, and nothing is changed for 
LLVM 16. Let's also say we have a program made up of two source files, `a.c`, 
and `b.c`. Then:

- `clang-16 -c -flto a.c b.c && clang-17 a.o b.o` should ideally be accepted 
and would behave in the same way as `clang-16 -c a.c b.c && clang-16 a.o b.o`.
- `clang-16 -c -flto a.c && clang-17 -c -flto b.c && clang-17 a.o b.o` should 
ideally be accepted if neither `a.o` nor `b.o` use


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

https://reviews.llvm.org/D86310

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

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86-64 datalayout

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D86310#4498551 , @rnk wrote:

> Given that most non-clang frontends want the bug fix (ABI break), who exactly 
> is asking for this level of IR ABI stability?

You were, I thought, or at least that's how I interpreted your earlier comment. 
:) If we're now all in agreement that that level of ABI stability is not 
needed, I can update this patch to address the comment that I had left (it 
should not be limited to 64-bit, it's needed for all X86). I'll probably be 
able to find time for this in the weekend.


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

https://reviews.llvm.org/D86310

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

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86-64 datalayout

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D86310#4498575 , @efriedma wrote:

> https://reviews.llvm.org/D86310#2231136 has an example where IR generated by 
> clang breaks.

clang bases it on the data layout, so when the change here is applied, clang 
already generates correct IR for that example without further changes (using 
`%struct.Y = type <{ i64, %struct.X }>`).


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

https://reviews.llvm.org/D86310

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

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86-64 datalayout

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

@JohnReagan That is a valid concern, and I hope it reassures you that if things 
were working before, I would never be on board with this change. For example, 
it would generally be better if long double were 8-byte-aligned, but the x86 
32-bit ABI specifies that it is 4-byte-aligned, and that is set in stone. I 
would be against any change in LLVM's ABI that changed their alignment, even if 
it would speed up code. I still occasionally run 20-year-old binaries, myself, 
that are dynamically linked to shared object files built with current 
compilers. Compatibility matters, I would not be on board with a change that 
breaks things like that. But that is not what is happening here. For i128, what 
clang implemented matched GCC, what LLVM implemented deviated from GCC/clang, 
but LLVM assumed that its implementation actually did match GCC/clang and code 
crashed as a result. This change would make it so that LLVM starts to also 
match GCC/clang, to change things from something that doesn't work to something 
that does work, and because things crash in current LLVM, I do not believe 
there can be much code out there that relies on the current behaviour. As you 
say, you aren't using i128/f128 yourself either. I hope that when I can update 
the patch, you can check that this does not cause problems for you.

@craig.topper Just to make sure, are you okay with me 'commandeering' this 
change and updating it?


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

https://reviews.llvm.org/D86310

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

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86-64 datalayout

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D86310#4501240 , @rnk wrote:

> In D86310#4501170 , @hvdijk wrote:
>
>> For example, it would generally be better if long double were 
>> 8-byte-aligned, but the x86 32-bit ABI specifies that it is 4-byte-aligned, 
>> and that is set in stone. I would be against any change in LLVM's ABI that 
>> changed their alignment, even if it would speed up code.
>
> That may be your view, but other users rely on the `-malign-double` flag 
> (D19734 ) to get the new behavior, despite 
> the ABI concerns. Specifically, it mattered for users passing structs from 
> CPU to GPU, because the GPU doesn't tolerate misaligned doubles well. With 
> that in mind, I wouldn't describe this ABI rule as being "set in stone", but 
> I understand your perspective.

As long as it is an option, it is fine, that will not cause compatibility 
issues.

> Returning to the patch at hand, it sounds like we have consensus that the 
> next step is to teach auto-upgrade to traverse the module looking for uses of 
> a particular type in structs and IR. That logic could be reused in the future 
> to solve similar problems when we need to adjust the layout of exotic types.

That is not my understanding of the consensus that we have, that is something 
that you asked for, then asked who asked for it, and are now again asking for. 
I do not see anyone else having asked for this, and I repeat that I think it is 
an unreasonable amount of work.


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

https://reviews.llvm.org/D86310

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

[PATCH] D147097: [SYCL] Always set NoUnwind attribute for SYCL.

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk created this revision.
hvdijk added a reviewer: bader.
hvdijk added a project: clang.
Herald added subscribers: Naghasan, Anastasia, ebevhan, yaxunl.
Herald added a project: All.
hvdijk requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added subscribers: cfe-commits, jplehr, sstefan1.

Like CUDA and OpenCL, the SYCL specification says that throwing and
catching exceptions in device functions is not supported, so this change
extends the logic for adding the NoUnwind attribute to SYCL.

The existing convergent.cpp test, which tests that the convergent
attribute is added to functions by default, is renamed and reused to
test that the nounwind attribute is added by default. This test now has
-fexceptions added to it, which the driver adds by default as well.

The obvious question here is why not simply change the driver to remove
-fexceptions. This change follows the direction given by the TODO
comment because removing -fexceptions would also disable the
__EXCEPTIONS macro, which should reflect whether exceptions are enabled
on the host, rather than on the device, to avoid conflicts in types
shared between host and device.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147097

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGenSYCL/convergent.cpp
  clang/test/CodeGenSYCL/function-attrs.cpp


Index: clang/test/CodeGenSYCL/function-attrs.cpp
===
--- clang/test/CodeGenSYCL/function-attrs.cpp
+++ clang/test/CodeGenSYCL/function-attrs.cpp
@@ -1,9 +1,10 @@
 // RUN: %clang_cc1 -fsycl-is-device -emit-llvm -disable-llvm-passes \
-// RUN:  -triple spir64 -emit-llvm %s -o - | FileCheck %s
+// RUN:  -triple spir64 -fexceptions -emit-llvm %s -o - | FileCheck %s
 
-// CHECK-DAG: Function Attrs:
-// CHECK-DAG-SAME: convergent
-// CHECK-DAG-NEXT: define void @_Z3foov
+// CHECK: Function Attrs:
+// CHECK-SAME: convergent
+// CHECK-SAME: nounwind
+// CHECK-NEXT: define dso_local spir_func void @_Z3foov
 void foo() {
   int a = 1;
 }
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -1971,10 +1971,9 @@
   }
 
   // TODO: NoUnwind attribute should be added for other GPU modes HIP,
-  // SYCL, OpenMP offload. AFAIK, none of them support exceptions in device
-  // code.
+  // OpenMP offload. AFAIK, neither of them support exceptions in device code.
   if ((getLangOpts().CUDA && getLangOpts().CUDAIsDevice) ||
-  getLangOpts().OpenCL) {
+  getLangOpts().OpenCL || getLangOpts().SYCLIsDevice) {
 FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);
   }
 


Index: clang/test/CodeGenSYCL/function-attrs.cpp
===
--- clang/test/CodeGenSYCL/function-attrs.cpp
+++ clang/test/CodeGenSYCL/function-attrs.cpp
@@ -1,9 +1,10 @@
 // RUN: %clang_cc1 -fsycl-is-device -emit-llvm -disable-llvm-passes \
-// RUN:  -triple spir64 -emit-llvm %s -o - | FileCheck %s
+// RUN:  -triple spir64 -fexceptions -emit-llvm %s -o - | FileCheck %s
 
-// CHECK-DAG: Function Attrs:
-// CHECK-DAG-SAME: convergent
-// CHECK-DAG-NEXT: define void @_Z3foov
+// CHECK: Function Attrs:
+// CHECK-SAME: convergent
+// CHECK-SAME: nounwind
+// CHECK-NEXT: define dso_local spir_func void @_Z3foov
 void foo() {
   int a = 1;
 }
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -1971,10 +1971,9 @@
   }
 
   // TODO: NoUnwind attribute should be added for other GPU modes HIP,
-  // SYCL, OpenMP offload. AFAIK, none of them support exceptions in device
-  // code.
+  // OpenMP offload. AFAIK, neither of them support exceptions in device code.
   if ((getLangOpts().CUDA && getLangOpts().CUDAIsDevice) ||
-  getLangOpts().OpenCL) {
+  getLangOpts().OpenCL || getLangOpts().SYCLIsDevice) {
 FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147097: [SYCL] Always set NoUnwind attribute for SYCL.

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

Is the rationale I gave in the description correct, or would it be better for 
SYCL device code to unconditionally build without `-fexceptions` and get the 
`nounwind` attribute added that way?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147097

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

[PATCH] D147097: [SYCL] Always set NoUnwind attribute for SYCL.

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

> That's a good question. I haven't looked into this issue deep enough, but I 
> think using -fexceptions requires using delayed diagnostics to avoid false 
> diagnostics during host code analysis.

I am assuming you mean `-fno-exceptions` (or, in `clang -cc1`, the absence of 
`-fexceptions`)? This is a good point. Delayed diagnostics would probably be 
good in general: we currently already emit warnings for host code when 
compiling for device, but as long as the generated warnings are identical for 
the host code as when compiling for host, it is easy enough to ignore; if the 
device compilation were to result in additional warnings for host code, and 
those warnings are incorrect, that would be a rather poor user experience. That 
sounds like a good additional reason to do it the way I had done here, the way 
the existing code comment had indicated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147097

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

[PATCH] D147097: [SYCL] Always set NoUnwind attribute for SYCL.

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk updated this revision to Diff 509335.
hvdijk added a comment.

Apparently the test was already passing without my change, so I updated the 
test to make it a function that did not previously get the nounwind attribute. 
This also revealed that the test was more fragile than I realised: it depends 
on the precise order in which functions are emitted. We always match "Function 
Attrs:" to the first emitted function, and then `CHECK-NEXT` can fail. We'd 
ideally first `CHECK`  for `void @_Z3foov()`  (now `void⋅@_Z3barv()`) and then 
`CHECK-PREV`  for the attributes, but FileCheck does not support that. What we 
can do instead though is find the matching `attributes #n = { ... }`  line and 
verify that everything is in there; updated accordingly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147097

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGenSYCL/convergent.cpp
  clang/test/CodeGenSYCL/function-attrs.cpp


Index: clang/test/CodeGenSYCL/function-attrs.cpp
===
--- clang/test/CodeGenSYCL/function-attrs.cpp
+++ clang/test/CodeGenSYCL/function-attrs.cpp
@@ -1,11 +1,18 @@
 // RUN: %clang_cc1 -fsycl-is-device -emit-llvm -disable-llvm-passes \
-// RUN:  -triple spir64 -emit-llvm %s -o - | FileCheck %s
+// RUN:  -triple spir64 -fexceptions -emit-llvm %s -o - | FileCheck %s
 
-// CHECK-DAG: Function Attrs:
-// CHECK-DAG-SAME: convergent
-// CHECK-DAG-NEXT: define void @_Z3foov
-void foo() {
-  int a = 1;
+int foo();
+
+// CHECK: define dso_local spir_func void @_Z3barv() [[BAR:#[0-9]+]]
+// CHECK: attributes [[BAR]] =
+// CHECK-SAME: convergent
+// CHECK-SAME: nounwind
+void bar() {
+  int a = foo();
+}
+
+int foo() {
+  return 1;
 }
 
 template 
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -1971,10 +1971,9 @@
   }
 
   // TODO: NoUnwind attribute should be added for other GPU modes HIP,
-  // SYCL, OpenMP offload. AFAIK, none of them support exceptions in device
-  // code.
+  // OpenMP offload. AFAIK, neither of them support exceptions in device code.
   if ((getLangOpts().CUDA && getLangOpts().CUDAIsDevice) ||
-  getLangOpts().OpenCL) {
+  getLangOpts().OpenCL || getLangOpts().SYCLIsDevice) {
 FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);
   }
 


Index: clang/test/CodeGenSYCL/function-attrs.cpp
===
--- clang/test/CodeGenSYCL/function-attrs.cpp
+++ clang/test/CodeGenSYCL/function-attrs.cpp
@@ -1,11 +1,18 @@
 // RUN: %clang_cc1 -fsycl-is-device -emit-llvm -disable-llvm-passes \
-// RUN:  -triple spir64 -emit-llvm %s -o - | FileCheck %s
+// RUN:  -triple spir64 -fexceptions -emit-llvm %s -o - | FileCheck %s
 
-// CHECK-DAG: Function Attrs:
-// CHECK-DAG-SAME: convergent
-// CHECK-DAG-NEXT: define void @_Z3foov
-void foo() {
-  int a = 1;
+int foo();
+
+// CHECK: define dso_local spir_func void @_Z3barv() [[BAR:#[0-9]+]]
+// CHECK: attributes [[BAR]] =
+// CHECK-SAME: convergent
+// CHECK-SAME: nounwind
+void bar() {
+  int a = foo();
+}
+
+int foo() {
+  return 1;
 }
 
 template 
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -1971,10 +1971,9 @@
   }
 
   // TODO: NoUnwind attribute should be added for other GPU modes HIP,
-  // SYCL, OpenMP offload. AFAIK, none of them support exceptions in device
-  // code.
+  // OpenMP offload. AFAIK, neither of them support exceptions in device code.
   if ((getLangOpts().CUDA && getLangOpts().CUDAIsDevice) ||
-  getLangOpts().OpenCL) {
+  getLangOpts().OpenCL || getLangOpts().SYCLIsDevice) {
 FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147097: [SYCL] Always set NoUnwind attribute for SYCL.

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk updated this revision to Diff 509355.
hvdijk added a comment.

Forgot to update the call to `foo()` to call `bar()` instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147097

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGenSYCL/convergent.cpp
  clang/test/CodeGenSYCL/function-attrs.cpp


Index: clang/test/CodeGenSYCL/function-attrs.cpp
===
--- clang/test/CodeGenSYCL/function-attrs.cpp
+++ clang/test/CodeGenSYCL/function-attrs.cpp
@@ -1,11 +1,18 @@
 // RUN: %clang_cc1 -fsycl-is-device -emit-llvm -disable-llvm-passes \
-// RUN:  -triple spir64 -emit-llvm %s -o - | FileCheck %s
+// RUN:  -triple spir64 -fexceptions -emit-llvm %s -o - | FileCheck %s
 
-// CHECK-DAG: Function Attrs:
-// CHECK-DAG-SAME: convergent
-// CHECK-DAG-NEXT: define void @_Z3foov
-void foo() {
-  int a = 1;
+int foo();
+
+// CHECK: define dso_local spir_func void @_Z3barv() [[BAR:#[0-9]+]]
+// CHECK: attributes [[BAR]] =
+// CHECK-SAME: convergent
+// CHECK-SAME: nounwind
+void bar() {
+  int a = foo();
+}
+
+int foo() {
+  return 1;
 }
 
 template 
@@ -14,6 +21,6 @@
 }
 
 int main() {
-  kernel_single_task([] { foo(); });
+  kernel_single_task([] { bar(); });
   return 0;
 }
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -1971,10 +1971,9 @@
   }
 
   // TODO: NoUnwind attribute should be added for other GPU modes HIP,
-  // SYCL, OpenMP offload. AFAIK, none of them support exceptions in device
-  // code.
+  // OpenMP offload. AFAIK, neither of them support exceptions in device code.
   if ((getLangOpts().CUDA && getLangOpts().CUDAIsDevice) ||
-  getLangOpts().OpenCL) {
+  getLangOpts().OpenCL || getLangOpts().SYCLIsDevice) {
 FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);
   }
 


Index: clang/test/CodeGenSYCL/function-attrs.cpp
===
--- clang/test/CodeGenSYCL/function-attrs.cpp
+++ clang/test/CodeGenSYCL/function-attrs.cpp
@@ -1,11 +1,18 @@
 // RUN: %clang_cc1 -fsycl-is-device -emit-llvm -disable-llvm-passes \
-// RUN:  -triple spir64 -emit-llvm %s -o - | FileCheck %s
+// RUN:  -triple spir64 -fexceptions -emit-llvm %s -o - | FileCheck %s
 
-// CHECK-DAG: Function Attrs:
-// CHECK-DAG-SAME: convergent
-// CHECK-DAG-NEXT: define void @_Z3foov
-void foo() {
-  int a = 1;
+int foo();
+
+// CHECK: define dso_local spir_func void @_Z3barv() [[BAR:#[0-9]+]]
+// CHECK: attributes [[BAR]] =
+// CHECK-SAME: convergent
+// CHECK-SAME: nounwind
+void bar() {
+  int a = foo();
+}
+
+int foo() {
+  return 1;
 }
 
 template 
@@ -14,6 +21,6 @@
 }
 
 int main() {
-  kernel_single_task([] { foo(); });
+  kernel_single_task([] { bar(); });
   return 0;
 }
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -1971,10 +1971,9 @@
   }
 
   // TODO: NoUnwind attribute should be added for other GPU modes HIP,
-  // SYCL, OpenMP offload. AFAIK, none of them support exceptions in device
-  // code.
+  // OpenMP offload. AFAIK, neither of them support exceptions in device code.
   if ((getLangOpts().CUDA && getLangOpts().CUDAIsDevice) ||
-  getLangOpts().OpenCL) {
+  getLangOpts().OpenCL || getLangOpts().SYCLIsDevice) {
 FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D147097: [SYCL] Always set NoUnwind attribute for SYCL.

[Harald van Dijk 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 rG6b868139458d: [SYCL] Always set NoUnwind attribute for SYCL. 
(authored by hvdijk).

Changed prior to commit:
  https://reviews.llvm.org/D147097?vs=509355&id=509528#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147097

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGenSYCL/convergent.cpp
  clang/test/CodeGenSYCL/function-attrs.cpp


Index: clang/test/CodeGenSYCL/function-attrs.cpp
===
--- /dev/null
+++ clang/test/CodeGenSYCL/function-attrs.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -fsycl-is-device -emit-llvm -disable-llvm-passes \
+// RUN:  -triple spir64 -fexceptions -emit-llvm %s -o - | FileCheck %s
+
+int foo();
+
+// CHECK: define dso_local spir_func void @_Z3barv() [[BAR:#[0-9]+]]
+// CHECK: attributes [[BAR]] =
+// CHECK-SAME: convergent
+// CHECK-SAME: nounwind
+void bar() {
+  int a = foo();
+}
+
+int foo() {
+  return 1;
+}
+
+template 
+__attribute__((sycl_kernel)) void kernel_single_task(const Func &kernelFunc) {
+  kernelFunc();
+}
+
+int main() {
+  kernel_single_task([] { bar(); });
+  return 0;
+}
Index: clang/test/CodeGenSYCL/convergent.cpp
===
--- clang/test/CodeGenSYCL/convergent.cpp
+++ /dev/null
@@ -1,19 +0,0 @@
-// RUN: %clang_cc1 -fsycl-is-device -emit-llvm -disable-llvm-passes \
-// RUN:  -triple spir64 -emit-llvm %s -o - | FileCheck %s
-
-// CHECK-DAG: Function Attrs:
-// CHECK-DAG-SAME: convergent
-// CHECK-DAG-NEXT: define void @_Z3foov
-void foo() {
-  int a = 1;
-}
-
-template 
-__attribute__((sycl_kernel)) void kernel_single_task(const Func &kernelFunc) {
-  kernelFunc();
-}
-
-int main() {
-  kernel_single_task([] { foo(); });
-  return 0;
-}
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -1971,10 +1971,9 @@
   }
 
   // TODO: NoUnwind attribute should be added for other GPU modes HIP,
-  // SYCL, OpenMP offload. AFAIK, none of them support exceptions in device
-  // code.
+  // OpenMP offload. AFAIK, neither of them support exceptions in device code.
   if ((getLangOpts().CUDA && getLangOpts().CUDAIsDevice) ||
-  getLangOpts().OpenCL) {
+  getLangOpts().OpenCL || getLangOpts().SYCLIsDevice) {
 FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);
   }
 


Index: clang/test/CodeGenSYCL/function-attrs.cpp
===
--- /dev/null
+++ clang/test/CodeGenSYCL/function-attrs.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -fsycl-is-device -emit-llvm -disable-llvm-passes \
+// RUN:  -triple spir64 -fexceptions -emit-llvm %s -o - | FileCheck %s
+
+int foo();
+
+// CHECK: define dso_local spir_func void @_Z3barv() [[BAR:#[0-9]+]]
+// CHECK: attributes [[BAR]] =
+// CHECK-SAME: convergent
+// CHECK-SAME: nounwind
+void bar() {
+  int a = foo();
+}
+
+int foo() {
+  return 1;
+}
+
+template 
+__attribute__((sycl_kernel)) void kernel_single_task(const Func &kernelFunc) {
+  kernelFunc();
+}
+
+int main() {
+  kernel_single_task([] { bar(); });
+  return 0;
+}
Index: clang/test/CodeGenSYCL/convergent.cpp
===
--- clang/test/CodeGenSYCL/convergent.cpp
+++ /dev/null
@@ -1,19 +0,0 @@
-// RUN: %clang_cc1 -fsycl-is-device -emit-llvm -disable-llvm-passes \
-// RUN:  -triple spir64 -emit-llvm %s -o - | FileCheck %s
-
-// CHECK-DAG: Function Attrs:
-// CHECK-DAG-SAME: convergent
-// CHECK-DAG-NEXT: define void @_Z3foov
-void foo() {
-  int a = 1;
-}
-
-template 
-__attribute__((sycl_kernel)) void kernel_single_task(const Func &kernelFunc) {
-  kernelFunc();
-}
-
-int main() {
-  kernel_single_task([] { foo(); });
-  return 0;
-}
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -1971,10 +1971,9 @@
   }
 
   // TODO: NoUnwind attribute should be added for other GPU modes HIP,
-  // SYCL, OpenMP offload. AFAIK, none of them support exceptions in device
-  // code.
+  // OpenMP offload. AFAIK, neither of them support exceptions in device code.
   if ((getLangOpts().CUDA && getLangOpts().CUDAIsDevice) ||
-  getLangOpts().OpenCL) {
+  getLangOpts().OpenCL || getLangOpts().SYCLIsDevice) {
 FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140315: [AMDGCN] Update search path for device libraries

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D140315#4077611 , @scchan wrote:

> In D140315#4076614 , @mgorny wrote:
>
>> I'm sorry for reporting it this late (clang's been broken for over two 
>> weeks, so we couldn't periodically test it) but this change is causing test 
>> regressions on Gentoo/amd64:
>
> Thanks for reporting the issue.  I'm working on a patch to fix the test.

I don't think it's actually the test that was at fault, it doesn't look right 
that this diff uses `CLANG_INSTALL_LIBDIR_BASENAME` for an unrelated purpose to 
determining the basename of the libdir, especially considering the comments 
continue to say clang looks in `lib/amdgcn/bitcode` rather than 
`/amdgcn/bitcode`, and your original test specifically assumed that it 
would always be `lib`. Should the code perhaps just be updated to hardcode 
`"lib"` as it did before?

Either way, the test can still fail after D142506 
, `CLANG_INSTALL_LIBDIR_BASENAME` may be any 
arbitrary string and cannot be assumed to be either `lib` or `lib64`. With 
Debian-style multiarch I have `CLANG_INSTALL_LIBDIR=lib/x86_64-linux-gnu`, so 
after updating from Clang 15 to Clang 16 I am now also seeing this test fail.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140315

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

[PATCH] D63423: [Diagnostics] Diagnose misused xor as pow

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

It's bad enough that this warns for

#define A 2
#define B 0
int f() { return A ^ B; }

where as far as the users of A are concerned, A is just some arbitrary value, 
it's just random chance it happens to be two and there is no chance of it being 
misinterpreted as exponentiation, but it's much worse that this cannot be 
worked around by changing the definition of A to 0x2, the only way to suppress 
the warning with a hexadecimal constant is by explicitly using that constant in 
the use, i.e. by changing the function to int f() { return 0x2 ^ B; }, which is 
very much inappropriate when the users of A are not supposed to care about 
which specific value it has.

I'd like to see this changed so that at least the #define A 0x2 case no longer 
triggers the warning, but ideally I'd like to see no warning for this example 
in its current state either. I see in the test case that that warning is 
intentional, but I'm not seeing from the previous discussion whether this case 
was based on real world programmer errors or not. Do you recall? If it was, 
then fair enough, I'll propose to leave that as it is.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63423

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

[PATCH] D63423: [Diagnostics] Diagnose misused xor as pow

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D63423#2732158 , @Quuxplusone wrote:

> In D63423#2732152 , @hvdijk wrote:
>
>> It's bad enough that this warns for
>>
>> #define A 2
>> int f() { return A ^ 1; }
>>
>> where as far as the users of A are concerned...
>
> I see how this warning is arguably overzealous in the //very special case// 
> of "raising" to the constant `1` (since nobody would ever write that by 
> accident). However, if the user of A wants to indicate that they understand 
> this is bitwise-xor, they can simply change the body of their `f` to `return 
> A ^ 0x1;` — hex notation suppresses the warning.  (Changing the definition of 
> `A` as well is perhaps a good idea but not technically required.)  IMO this 
> is good enough and we should leave it.  (What do you think, having seen the 
> `A ^ 0x1` workaround? Does that sufficiently address your needs?)

I missed that hex notation for the RHS also suppresses the warning. Thanks, 
that's good to know. Combined with the fact that the case where LHS and RHS are 
both macros now no longer triggers the warning, that means one of LHS or RHS 
must be an integer literal, so there will always be a viable workaround once 
clang 13 is released. That may be good enough.

>> [...] I'm not seeing from the previous discussion whether this case was 
>> based on real world programmer errors or not
>
> The description links to a couple of tweets showing examples from the wild:
> https://twitter.com/jfbastien/status/1139298419988549632
> https://twitter.com/mikemx7f/status/1139335901790625793

Those are different cases, they literally have `2 ^` in there. In the case I 
was asking about, the `2` comes from a macro expansion, but the `^` does not. 
That is much less likely to be a mistake, and one where I didn't see whether 
the warning for that was based on real-world concerns.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63423

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

[PATCH] D63423: [Diagnostics] Diagnose misused xor as pow

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D63423#2732186 , @xbolva00 wrote:

> I remember that was interest to support macros too :) tbh I cant remember now 
> such real world case where “macro ^ cst” was an issue but.. it was a long 
> time ago ;)
>
> If you are want, you can send patch to to avoid warning in this case, we can 
> discuss it.

Will do; the change is trivial after D97445 : 
it's just changing that to use `||` rather than `&&`. I'll do that and update 
the tests to match. I don't know whether that'll be what we want, but it may be 
easier to talk about once we have a good picture from the test case update what 
it affects.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63423

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

[PATCH] D63423: [Diagnostics] Diagnose misused xor as pow

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D63423#2732199 , @xbolva00 wrote:

> No, we want to preserve warning for 2 ^ MACRO or 10 ^ MACRO.

Perhaps that should warn even if the RHS is in hex form, or is an enumerator 
constant, or is not even constant at all.

libpng: #if-ed out but just waiting for someone to copy and paste: `for 
(i=11;i>=0;--i){ print i, " ", (1 - e(-(2^i)/65536*l(2))) * 2^(32-i), "\n"}`.

https://github.com/mongodb/mongo/blob/master/src/third_party/IntelRDFPMathLib20U1/LIBRARY/float128/mphoc_macros.h:

  #define MPHOC_MAX_CHAR  ((2 ^ (BITS_PER_CHAR - 1)) - 1)
  #define MPHOC_MAX_SHORT ((2 ^ (BITS_PER_SHORT - 1)) - 1)
  #define MPHOC_MAX_INT   ((2 ^ (BITS_PER_INT - 1)) - 1)
  #define MPHOC_MAX_LONG  ((2 ^ (BITS_PER_LONG - 1)) - 1)
  #define MPHOC_MAX_WORD  ((2 ^ (BITS_PER_WORD - 1)) - 1)

(and many more in there)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63423

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

[PATCH] D63423: [Diagnostics] Diagnose misused xor as pow

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D63423#2732209 , @xbolva00 wrote:

>>> Perhaps that should warn even if the RHS is in hex form
>
> It would be kinda strange, since in one clang release we ask users to silence 
> warning with hex form and newer release would warn anyway. Not a fan of this 
> decision.

Fair point, but the clang suggestion is always to turn the LHS into hex form, 
never to turn the RHS in hex form, so if users followed clang's suggested 
silencing, it would continue to work. That's how I didn't even notice that I 
could change the RHS to hex form in the case I asked about.

And fully agreed that if we warn about any cases where we didn't warn before, 
we need to be very careful about the risk of false positives. When I create the 
new RFC patch, I'll try to get details on that and include it in the 
description.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63423

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

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86-64 datalayout

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.
Herald added a subscriber: pengfei.

There is a risk of bitcode incompatibilities with this change, but we already 
have that the code we generate now is incompatible with GCC and results in 
crashes that way too, I don't think there's a perfect fix, I'd like it if we 
could merge this. I came up with roughly the same patch today, based on current 
sources, to fix bug #50198 before finding this one.




Comment at: llvm/lib/IR/AutoUpgrade.cpp:4323
+  // alignment. We'll handle them separately.
+  if (TT.isArch64Bit() && !DL.contains("-i128:128")) {
+auto I = DL.find("-i64:64-");

This needs to not be limited to `TT.isArch64Bit()`. i128 needs 16-byte 
alignment on all targets, and although clang disables `__int128` for X86, we 
still use it for lowering f128.


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

https://reviews.llvm.org/D86310

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

[PATCH] D101851: Make clangd CompletionModel not depend on directory layout.

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk created this revision.
hvdijk added reviewers: sammccall, usaxena95.
hvdijk added a project: clang-tools-extra.
Herald added subscribers: kadircet, arphaman, mgorny.
hvdijk requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.

The current code accounts for two possible layouts, but there is at least a 
third supported layout: clang-tools-extra may also be checked out as 
clang/tools/extra with the releases, which was not yet handled. Rather than 
treating that as a special case, use the location of CompletionModel.cmake to 
handle all three cases. This should address the problems that prompted D96787 
 and the problems that prompted the proposed 
revert D100625 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D101851

Files:
  clang-tools-extra/clangd/quality/CompletionModel.cmake


Index: clang-tools-extra/clangd/quality/CompletionModel.cmake
===
--- clang-tools-extra/clangd/quality/CompletionModel.cmake
+++ clang-tools-extra/clangd/quality/CompletionModel.cmake
@@ -4,8 +4,9 @@
 # ${CMAKE_CURRENT_BINARY_DIR}. The generated header
 # will define a C++ class called ${cpp_class} - which may be a
 # namespace-qualified class name.
+set(CLANGD_QUALITY_DIR ${CMAKE_CURRENT_LIST_DIR})
 function(gen_decision_forest model filename cpp_class)
-  set(model_compiler 
${LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR}/clangd/quality/CompletionModelCodegen.py)
+  set(model_compiler ${CLANGD_QUALITY_DIR}/CompletionModelCodegen.py)
 
   set(output_dir ${CMAKE_CURRENT_BINARY_DIR})
   set(header_file ${output_dir}/${filename}.h)


Index: clang-tools-extra/clangd/quality/CompletionModel.cmake
===
--- clang-tools-extra/clangd/quality/CompletionModel.cmake
+++ clang-tools-extra/clangd/quality/CompletionModel.cmake
@@ -4,8 +4,9 @@
 # ${CMAKE_CURRENT_BINARY_DIR}. The generated header
 # will define a C++ class called ${cpp_class} - which may be a
 # namespace-qualified class name.
+set(CLANGD_QUALITY_DIR ${CMAKE_CURRENT_LIST_DIR})
 function(gen_decision_forest model filename cpp_class)
-  set(model_compiler ${LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR}/clangd/quality/CompletionModelCodegen.py)
+  set(model_compiler ${CLANGD_QUALITY_DIR}/CompletionModelCodegen.py)
 
   set(output_dir ${CMAKE_CURRENT_BINARY_DIR})
   set(header_file ${output_dir}/${filename}.h)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101851: Make clangd CompletionModel not depend on directory layout.

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk updated this revision to Diff 342881.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101851

Files:
  clang-tools-extra/clangd/quality/CompletionModel.cmake


Index: clang-tools-extra/clangd/quality/CompletionModel.cmake
===
--- clang-tools-extra/clangd/quality/CompletionModel.cmake
+++ clang-tools-extra/clangd/quality/CompletionModel.cmake
@@ -4,8 +4,9 @@
 # ${CMAKE_CURRENT_BINARY_DIR}. The generated header
 # will define a C++ class called ${cpp_class} - which may be a
 # namespace-qualified class name.
+set(CLANGD_COMPLETION_MODEL_COMPILER 
${CMAKE_CURRENT_LIST_DIR}/CompletionModelCodegen.py)
 function(gen_decision_forest model filename cpp_class)
-  set(model_compiler 
${LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR}/clangd/quality/CompletionModelCodegen.py)
+  set(model_compiler ${CLANGD_COMPLETION_MODEL_COMPILER})
 
   set(output_dir ${CMAKE_CURRENT_BINARY_DIR})
   set(header_file ${output_dir}/${filename}.h)


Index: clang-tools-extra/clangd/quality/CompletionModel.cmake
===
--- clang-tools-extra/clangd/quality/CompletionModel.cmake
+++ clang-tools-extra/clangd/quality/CompletionModel.cmake
@@ -4,8 +4,9 @@
 # ${CMAKE_CURRENT_BINARY_DIR}. The generated header
 # will define a C++ class called ${cpp_class} - which may be a
 # namespace-qualified class name.
+set(CLANGD_COMPLETION_MODEL_COMPILER ${CMAKE_CURRENT_LIST_DIR}/CompletionModelCodegen.py)
 function(gen_decision_forest model filename cpp_class)
-  set(model_compiler ${LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR}/clangd/quality/CompletionModelCodegen.py)
+  set(model_compiler ${CLANGD_COMPLETION_MODEL_COMPILER})
 
   set(output_dir ${CMAKE_CURRENT_BINARY_DIR})
   set(header_file ${output_dir}/${filename}.h)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101851: Make clangd CompletionModel not depend on directory layout.

[Harald van Dijk via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clangd/quality/CompletionModel.cmake:7
 # namespace-qualified class name.
+set(CLANGD_QUALITY_DIR ${CMAKE_CURRENT_LIST_DIR})
 function(gen_decision_forest model filename cpp_class)

usaxena95 wrote:
> nit: Directly refer to code generator instead of storing quality dir.
> `set(CLANGD_COMPLETION_MODEL_COMPILER 
> ${CMAKE_CURRENT_LIST_DIR}/CompletionModelCodegen.py)`
Alright, done. Testing in progress just to be on the safe side. We could also 
get rid of `model_compiler` and use `${CLANGD_COMPLETION_MODEL_COMPILER}` 
directly, but I felt that looked bad due to the conflicting variable naming 
styles and the long lines, so I left that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101851

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

[PATCH] D101851: Make clangd CompletionModel not depend on directory layout.

[Harald van Dijk via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG7907c46fe619: Make clangd CompletionModel not depend on 
directory layout. (authored by hvdijk).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101851

Files:
  clang-tools-extra/clangd/quality/CompletionModel.cmake


Index: clang-tools-extra/clangd/quality/CompletionModel.cmake
===
--- clang-tools-extra/clangd/quality/CompletionModel.cmake
+++ clang-tools-extra/clangd/quality/CompletionModel.cmake
@@ -4,8 +4,9 @@
 # ${CMAKE_CURRENT_BINARY_DIR}. The generated header
 # will define a C++ class called ${cpp_class} - which may be a
 # namespace-qualified class name.
+set(CLANGD_COMPLETION_MODEL_COMPILER 
${CMAKE_CURRENT_LIST_DIR}/CompletionModelCodegen.py)
 function(gen_decision_forest model filename cpp_class)
-  set(model_compiler 
${LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR}/clangd/quality/CompletionModelCodegen.py)
+  set(model_compiler ${CLANGD_COMPLETION_MODEL_COMPILER})
 
   set(output_dir ${CMAKE_CURRENT_BINARY_DIR})
   set(header_file ${output_dir}/${filename}.h)


Index: clang-tools-extra/clangd/quality/CompletionModel.cmake
===
--- clang-tools-extra/clangd/quality/CompletionModel.cmake
+++ clang-tools-extra/clangd/quality/CompletionModel.cmake
@@ -4,8 +4,9 @@
 # ${CMAKE_CURRENT_BINARY_DIR}. The generated header
 # will define a C++ class called ${cpp_class} - which may be a
 # namespace-qualified class name.
+set(CLANGD_COMPLETION_MODEL_COMPILER ${CMAKE_CURRENT_LIST_DIR}/CompletionModelCodegen.py)
 function(gen_decision_forest model filename cpp_class)
-  set(model_compiler ${LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR}/clangd/quality/CompletionModelCodegen.py)
+  set(model_compiler ${CLANGD_COMPLETION_MODEL_COMPILER})
 
   set(output_dir ${CMAKE_CURRENT_BINARY_DIR})
   set(header_file ${output_dir}/${filename}.h)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100625: [clang-tools-extra] Do not use `LLVM_EXTERNAL_CLANG_TOOLS_EXTRA_SOURCE_DIR`

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.
Herald added a subscriber: cfe-commits.

Apparently Phabricator detected the presence of "revert D100625 
" in my alternative D101851 
's description and marked that as reverting 
this one. Of course, as this hadn't been committed, it cannot have been 
reverted either. Could you confirm that things work for you now, @Abpostelnicu?

In D100625#2697345 , @jpalus wrote:

> Do you mind taking a look at fix proposed in:
>
> https://bugs.llvm.org/show_bug.cgi?id=49990#c3

Sorry for not noticing this before, this is pretty much what I came up with too 
and what's committed now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100625

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

[PATCH] D98574: [Sparc] Define the same macros for -mcpu=v9 as GCC on Linux and the BSDs

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added inline comments.



Comment at: clang/lib/Basic/Targets/Sparc.cpp:246-256
+  if (getTriple().getOS() == llvm::Triple::Linux) {
 Builder.defineMacro("__sparc_v9__");
-Builder.defineMacro("__sparcv9__");
+  } else {
+Builder.defineMacro("__sparcv9");
+// Solaris doesn't need these variants, but the BSDs do.
+if (getTriple().getOS() != llvm::Triple::Solaris) {
+  Builder.defineMacro("__sparc64__");

brad wrote:
> glaubitz wrote:
> > glaubitz wrote:
> > > ro wrote:
> > > > glaubitz wrote:
> > > > > jrtc27 wrote:
> > > > > > This doesn't need changing, we can define more things than GCC to 
> > > > > > keep it simple.
> > > > > Well, my original intent was to match GCC to make sure we're 100% 
> > > > > compatible and I would like to keep it that way.
> > > > I agree with Jessica here: you're creating a complicated maze for no 
> > > > real gain.  Besides, have you checked what `gcc` on the BSDs really 
> > > > does?  They often neglect to get their changes upstream and what's in 
> > > > the gcc repo doesn't necessarily represent what they actually use.
> > > Yes, I have verified that GCC behaves the exact same way as this change 
> > > and I don't see any reason not to mimic the exact same behavior in clang 
> > > for maximum compatibility.
> > FWIW, I meant GCC on the various BSDs. I do not think it's a wise idea to 
> > have clang deviate from what GCC does as only this way we can guarantee 
> > that everything that compiles with GCC will compile with clang.
> > Besides, have you checked what `gcc` on the BSDs really does?  They often 
> > neglect to get their changes upstream and what's in the gcc repo doesn't 
> > necessarily represent what they actually use.
> 
> What is upstream is what we do. There are no local patches that change 
> behavior in this particular area.
(Copying here what I had already replied privately a while back) It worries me 
that this switch statement only handles known operating systems (Linux, 
FreeBSD, NetBSD, OpenBSD, Solaris) when we also have code to allow 
SparcV9TargetInfo to be created without an operating system in 
clang/lib/Basic/Targets.cpp. Either there should be a default case that is 
properly handled, or if that actually cannot happen, there should be an assert 
that it doesn't happen.

I agree with the earlier comments that there should be nothing wrong with 
defining more macros than GCC, if the macros make sense. For the 
SparcV9TargetInfo class, my impression is that the macros make sense. For the 
SparcV8TargetInfo class with a V9 CPU, reading the discussion in D86621, if 
Oracle say that `__sparcv9` is only for 64-bit mode, GCC also only defines it 
for 64-bit mode, glibc assumes that `__sparcv9` implies 64-bit mode, etc. then 
SparcV8TargetInfo should not be defining `__sparcv9`. Your changes to 
SparcV8TargetInfo should be enough to fix bug 49562, right? If so, would it be 
okay to update this diff with just that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98574

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

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86-64 datalayout

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

In D86310#3226142 , @rnk wrote:

> In D86310#2736983 , @hvdijk wrote:
>
>> There is a risk of bitcode incompatibilities with this change, but we 
>> already have that the code we generate now is incompatible with GCC and 
>> results in crashes that way too, I don't think there's a perfect fix, I'd 
>> like it if we could merge this. I came up with roughly the same patch today, 
>> based on current sources, to fix bug #50198 before finding this one.
>
> Who exactly generates GCC-incompatible code, clang, LLVM, or some other 
> frontend? My understanding is that Clang handles most struct layout and 
> alignment concerns in the frontend.

It's usually handled by clang, but when operations get lowered by LLVM to 
libcalls, it's coming from LLVM, and that's happening in the bug I referenced 
in that comment.


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

https://reviews.llvm.org/D86310

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

[PATCH] D52050: WIP: [Driver] Fix architecture triplets and search paths for Linux x32

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

I may be missing something, but I do not understand the problem. What systems, 
other than Debian multi-arch, are you looking to also add support for? My own 
native x32 system uses `(/usr)/libx32` for x32 libraries. Debian uses 
`(/usr)/lib/x86_64-linux-gnux32`. I can understand if some people might use 
`(/usr)/lib` without any `x32` suffix, though I am not aware of anyone doing 
this. Where does `lib32` come from, though? What other systems are you trying 
to account for?

I may have some spare time soon, I can take a look and do some testing as well.




Comment at: clang/lib/Driver/ToolChains/Linux.cpp:217
+  return "lib32";
+else
+  return "libx32";

x86_64-linux-gnux32 never uses `lib32`, does it? It might use `lib` (mostly 
theoretical), or it might use `libx32`. Leaving this unmodified, always 
returning `libx32` for x32, looks to me like it matches x86_64-linux-gnu, which 
always returns `lib64` even though some people put libraries for that in `/lib` 
too.



Comment at: clang/lib/Driver/ToolChains/Linux.cpp:90
   return "x86_64-linux-android";
-// We don't want this for x32, otherwise it will match x86_64 libs
-if (TargetEnvironment != llvm::Triple::GNUX32 &&
-D.getVFS().exists(SysRoot + "/lib/x86_64-linux-gnu"))
-  return "x86_64-linux-gnu";
+if (TargetEnvironment == llvm::Triple::GNUX32) {
+  if (D.getVFS().exists(SysRoot + "/lib/x86_64-linux-gnux32"))

glaubitz wrote:
> MaskRay wrote:
> > I have cleaned up the code a bit. You may need to rebase.
> Yeah, I have done that but not uploaded my latest diff yet (which doesn't 
> work on x32, unfortunately). I will do that now.
This should have a `TargetEnvironment == llvm::Triple::GNUX32` check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D52050

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

[PATCH] D52050: WIP: [Driver] Fix architecture triplets and search paths for Linux x32

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

I am testing the below, on top of c8e56f394af0b9e32c413d62a0e7aebbba3e6b70 
, both in 
a Debian chroot and in my non-Debian system. Initial testing in the Debian 
chroot suggests that this works for simple cases, clang has no problem finding 
/usr/lib/gcc/x86_64-linux-gnux32/10, and also picks up the right crt*.o files 
when using -m32 or -m64. Will do more extensive testing.

  --- a/clang/lib/Driver/ToolChains/Gnu.cpp
  +++ b/clang/lib/Driver/ToolChains/Gnu.cpp
  @@ -2106,7 +2106,10 @@ void 
Generic_GCC::GCCInstallationDetector::AddDefaultGCCPrefixes(
 "x86_64-manbo-linux-gnu", "x86_64-linux-gnu",
 "x86_64-slackware-linux", "x86_64-unknown-linux",
 "x86_64-amazon-linux","x86_64-linux-android"};
  -  static const char *const X32LibDirs[] = {"/libx32"};
  +  static const char *const X32Triples[] = {
  +  "x86_64-linux-gnux32","x86_64-unknown-linux-gnux32",
  +  "x86_64-pc-linux-gnux32"};
  +  static const char *const X32LibDirs[] = {"/libx32", "/lib"};
 static const char *const X86LibDirs[] = {"/lib32", "/lib"};
 static const char *const X86Triples[] = {
 "i586-linux-gnu", "i686-linux-gnu",
  @@ -2337,17 +2340,19 @@ void 
Generic_GCC::GCCInstallationDetector::AddDefaultGCCPrefixes(
   TripleAliases.append(begin(AVRTriples), end(AVRTriples));
   break;
 case llvm::Triple::x86_64:
  -LibDirs.append(begin(X86_64LibDirs), end(X86_64LibDirs));
  -TripleAliases.append(begin(X86_64Triples), end(X86_64Triples));
  -// x32 is always available when x86_64 is available, so adding it as
  -// secondary arch with x86_64 triples
   if (TargetTriple.getEnvironment() == llvm::Triple::GNUX32) {
  -  BiarchLibDirs.append(begin(X32LibDirs), end(X32LibDirs));
  +  LibDirs.append(begin(X32LibDirs), end(X32LibDirs));
  +  TripleAliases.append(begin(X32Triples), end(X32Triples));
  +  BiarchLibDirs.append(begin(X86_64LibDirs), end(X86_64LibDirs));
 BiarchTripleAliases.append(begin(X86_64Triples), end(X86_64Triples));
   } else {
  -  BiarchLibDirs.append(begin(X86LibDirs), end(X86LibDirs));
  -  BiarchTripleAliases.append(begin(X86Triples), end(X86Triples));
  +  LibDirs.append(begin(X86_64LibDirs), end(X86_64LibDirs));
  +  TripleAliases.append(begin(X86_64Triples), end(X86_64Triples));
  +  BiarchLibDirs.append(begin(X32LibDirs), end(X32LibDirs));
  +  BiarchTripleAliases.append(begin(X32Triples), end(X32Triples));
   }
  +BiarchLibDirs.append(begin(X86LibDirs), end(X86LibDirs));
  +BiarchTripleAliases.append(begin(X86Triples), end(X86Triples));
   break;
 case llvm::Triple::x86:
   LibDirs.append(begin(X86LibDirs), end(X86LibDirs));
  @@ -2357,6 +2362,8 @@ void 
Generic_GCC::GCCInstallationDetector::AddDefaultGCCPrefixes(
 TripleAliases.append(begin(X86Triples), end(X86Triples));
 BiarchLibDirs.append(begin(X86_64LibDirs), end(X86_64LibDirs));
 BiarchTripleAliases.append(begin(X86_64Triples), end(X86_64Triples));
  +  BiarchLibDirs.append(begin(X32LibDirs), end(X32LibDirs));
  +  BiarchTripleAliases.append(begin(X32Triples), end(X32Triples));
   }
   break;
 case llvm::Triple::m68k:


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D52050

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

[PATCH] D52050: WIP: [Driver] Fix architecture triplets and search paths for Linux x32

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

I am building with

  cmake -G Ninja ../llvm -DCMAKE_BUILD_TYPE=Release 
-DLLVM_ENABLE_PROJECTS='clang;libcxx;libcxxabi' 
-DLLVM_HOST_TRIPLE=x86_64-pc-linux-gnux32 -DBUILD_SHARED_LIBS=ON 
-DLLVM_LIBDIR_SUFFIX=x32 -DLLVM_ENABLE_RTTI=ON -DLLVM_BUILD_TESTS=ON 
-DLLVM_TARGETS_TO_BUILD=X86

and testing the resulting clang with simple programs, not yet to build clang 
again, but the error you are getting suggests that not even simple programs 
would work. The lack of -DLLVM_HOST_TRIPLE=x86_64-pc-linux-gnux32 in your 
command line is again suspicious and the fact that I did include that may have 
avoided the problem you are seeing for me; I will test leaving that out when I 
finish other tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D52050

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

[PATCH] D52050: WIP: [Driver] Fix architecture triplets and search paths for Linux x32

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk added a comment.

I think the problem is actually the other thing covered before (don't provide 
un-normalised triples as cmake arguments), though the wrong detection of 
LLVM_HOST_TRIPLE will cause other issues too. If we would just update 
config.guess, the default configuration (not specifying either LLVM_HOST_TRIPLE 
or LLVM_DEFAULT_TARGET_TRIPLE) should work much better; I have created D99625 
 for that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D52050

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

[PATCH] D52050: WIP: [Driver] Fix architecture triplets and search paths for Linux x32

[Harald van Dijk via Phabricator via cfe-commits]

hvdijk commandeered this revision.
hvdijk edited reviewers, added: glaubitz; removed: hvdijk.
hvdijk added a comment.

> Feel free to update the diff here with your suggested patch.

Alright, thanks. Updating this requires me to 'commandeer' it, doing that now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D52050

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