[PATCH] D49674: [AArch64] Add Tiny Code Model for AArch64

[Peter Smith via Phabricator via cfe-commits]

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

Looks good to me now that LLVM support has been approved in 
https://reviews.llvm.org/D49673


https://reviews.llvm.org/D49674



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

[PATCH] D46932: [AArch64] Correct inline assembly test case for S modifier [NFC]

[Peter Smith via Phabricator via cfe-commits]

peter.smith created this revision.
peter.smith added reviewers: t.p.northover, manojgupta.
Herald added subscribers: kristof.beyls, eraman, rengolin.
Herald added a reviewer: javed.absar.

The existing test for the AArch64 constraint S uses the A and L modifiers. 
These modifiers were implemented in the original AArch64 backend but were not 
carried forward to the merged backend. The A is associated with ADRP and does 
nothing, the L is associated with :lo12: . Given that A and L are not supported 
by GCC and not supported by the new implementation of constraint S in LLVM (see 
https://reviews.llvm.org/D46745) I've altered the test to put :lo12: directly 
in the string so that A and L are not needed. A recent use of S in the linux 
kernel can be found at 
https://github.com/torvalds/linux/commit/44a497abd621a71c645f06d3d545ae2f46448830


https://reviews.llvm.org/D46932

Files:
  test/CodeGen/aarch64-inline-asm.c


Index: test/CodeGen/aarch64-inline-asm.c
===
--- test/CodeGen/aarch64-inline-asm.c
+++ test/CodeGen/aarch64-inline-asm.c
@@ -44,9 +44,9 @@
 
 void test_constraint_S(void) {
 int *addr;
-asm("adrp %0, %A1\n\t"
-"add %0, %0, %L1" : "=r"(addr) : "S"(&var));
-// CHECK: call i32* asm "adrp $0, ${1:A}\0A\09add $0, $0, ${1:L}", "=r,S"(i64* 
@var)
+asm("adrp %0, %1\n\t"
+"add %0, %0, :lo12:%1" : "=r"(addr) : "S"(&var));
+// CHECK: call i32* asm "adrp $0, $1\0A\09add $0, $0, :lo12:$1", "=r,S"(i64* 
@var)
 }
 
 void test_constraint_Q(void) {


Index: test/CodeGen/aarch64-inline-asm.c
===
--- test/CodeGen/aarch64-inline-asm.c
+++ test/CodeGen/aarch64-inline-asm.c
@@ -44,9 +44,9 @@
 
 void test_constraint_S(void) {
 int *addr;
-asm("adrp %0, %A1\n\t"
-"add %0, %0, %L1" : "=r"(addr) : "S"(&var));
-// CHECK: call i32* asm "adrp $0, ${1:A}\0A\09add $0, $0, ${1:L}", "=r,S"(i64* @var)
+asm("adrp %0, %1\n\t"
+"add %0, %0, :lo12:%1" : "=r"(addr) : "S"(&var));
+// CHECK: call i32* asm "adrp $0, $1\0A\09add $0, $0, :lo12:$1", "=r,S"(i64* @var)
 }
 
 void test_constraint_Q(void) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46932: [AArch64] Correct inline assembly test case for S modifier [NFC]

[Peter Smith via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC332606: [AArch64] Correct inline assembly test case for S 
modifier [NFC] (authored by psmith, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D46932

Files:
  test/CodeGen/aarch64-inline-asm.c


Index: test/CodeGen/aarch64-inline-asm.c
===
--- test/CodeGen/aarch64-inline-asm.c
+++ test/CodeGen/aarch64-inline-asm.c
@@ -44,9 +44,9 @@
 
 void test_constraint_S(void) {
 int *addr;
-asm("adrp %0, %A1\n\t"
-"add %0, %0, %L1" : "=r"(addr) : "S"(&var));
-// CHECK: call i32* asm "adrp $0, ${1:A}\0A\09add $0, $0, ${1:L}", "=r,S"(i64* 
@var)
+asm("adrp %0, %1\n\t"
+"add %0, %0, :lo12:%1" : "=r"(addr) : "S"(&var));
+// CHECK: call i32* asm "adrp $0, $1\0A\09add $0, $0, :lo12:$1", "=r,S"(i64* 
@var)
 }
 
 void test_constraint_Q(void) {


Index: test/CodeGen/aarch64-inline-asm.c
===
--- test/CodeGen/aarch64-inline-asm.c
+++ test/CodeGen/aarch64-inline-asm.c
@@ -44,9 +44,9 @@
 
 void test_constraint_S(void) {
 int *addr;
-asm("adrp %0, %A1\n\t"
-"add %0, %0, %L1" : "=r"(addr) : "S"(&var));
-// CHECK: call i32* asm "adrp $0, ${1:A}\0A\09add $0, $0, ${1:L}", "=r,S"(i64* @var)
+asm("adrp %0, %1\n\t"
+"add %0, %0, :lo12:%1" : "=r"(addr) : "S"(&var));
+// CHECK: call i32* asm "adrp $0, $1\0A\09add $0, $0, :lo12:$1", "=r,S"(i64* @var)
 }
 
 void test_constraint_Q(void) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39179: [AArch64] Fix PR34625 -mtune without -mcpu should not set -target-cpu

[Peter Smith via Phabricator via cfe-commits]

peter.smith created this revision.
Herald added subscribers: javed.absar, rengolin, aemerson.

When -mtune is used on AArch64 the -target-cpu is passed the value of the cpu 
given to -mtune. As well as setting micro-architectural features of the -mtune 
cpu, this will also add the architectural features such as support for 
instructions. This can result in the backend using instructions that are 
supported in the -mtune cpu but not supported in the target architecture. For 
example use of the v8.1-a LSE extensions with -march=v8.

  

This change removes the setting of -target-cpu for -mtune, the -mcpu must be 
used to set -target-cpu. This has the effect of removing all non-hard coded 
benefits of mtune but it does produce correct output when -mtune cpu with a 
later architecture than v8 is used. I've kept the hard-coded behaviour of 
-mtune for some Apple CPU codenames and have added in a call to check the 
-mtune cpu as there are some tests that depend on it.

Further work will be needed to implement -mtune properly, so that it affects 
only micro-architectural features.

fixes PR34625


https://reviews.llvm.org/D39179

Files:
  lib/Driver/ToolChains/Arch/AArch64.cpp
  test/Driver/aarch64-cpus.c

Index: test/Driver/aarch64-cpus.c
===
--- test/Driver/aarch64-cpus.c
+++ test/Driver/aarch64-cpus.c
@@ -21,182 +21,205 @@
 // RUN: %clang -target aarch64 -mcpu=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=CA35 %s
 // RUN: %clang -target aarch64 -mlittle-endian -mcpu=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=CA35 %s
 // RUN: %clang -target aarch64_be -mlittle-endian -mcpu=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=CA35 %s
-// RUN: %clang -target aarch64 -mtune=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=CA35 %s
-// RUN: %clang -target aarch64 -mlittle-endian -mtune=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=CA35 %s
-// RUN: %clang -target aarch64_be -mlittle-endian -mtune=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=CA35 %s
+// RUN: %clang -target aarch64 -mtune=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=CA35-TUNE %s
+// RUN: %clang -target aarch64 -mlittle-endian -mtune=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=CA35-TUNE %s
+// RUN: %clang -target aarch64_be -mlittle-endian -mtune=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=CA35-TUNE %s
 // CA35: "-cc1"{{.*}} "-triple" "aarch64{{.*}}" "-target-cpu" "cortex-a35"
+// CA35-TUNE: "-cc1"{{.*}} "-triple" "aarch64{{.*}}" "-target-cpu" "generic"
 
 // RUN: %clang -target arm64 -mcpu=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=ARM64-CA35 %s
 // RUN: %clang -target arm64 -mlittle-endian -mcpu=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=ARM64-CA35 %s
-// RUN: %clang -target arm64 -mtune=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=ARM64-CA35 %s
-// RUN: %clang -target arm64 -mlittle-endian -mtune=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=ARM64-CA35 %s
+// RUN: %clang -target arm64 -mtune=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=ARM64-CA35-TUNE %s
+// RUN: %clang -target arm64 -mlittle-endian -mtune=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=ARM64-CA35-TUNE %s
 // ARM64-CA35: "-cc1"{{.*}} "-triple" "arm64{{.*}}" "-target-cpu" "cortex-a35"
-
+// ARM64-CA35-TUNE: "-cc1"{{.*}} "-triple" "arm64{{.*}}" "-target-cpu" "generic"
 
 // RUN: %clang -target aarch64 -mcpu=cortex-a53 -### -c %s 2>&1 | FileCheck -check-prefix=CA53 %s
 // RUN: %clang -target aarch64 -mlittle-endian -mcpu=cortex-a53 -### -c %s 2>&1 | FileCheck -check-prefix=CA53 %s
 // RUN: %clang -target aarch64_be -mlittle-endian -mcpu=cortex-a53 -### -c %s 2>&1 | FileCheck -check-prefix=CA53 %s
-// RUN: %clang -target aarch64 -mtune=cortex-a53 -### -c %s 2>&1 | FileCheck -check-prefix=CA53 %s
-// RUN: %clang -target aarch64_be -mlittle-endian -mtune=cortex-a53 -### -c %s 2>&1 | FileCheck -check-prefix=CA53 %s
+// RUN: %clang -target aarch64 -mtune=cortex-a53 -### -c %s 2>&1 | FileCheck -check-prefix=CA53-TUNE %s
+// RUN: %clang -target aarch64_be -mlittle-endian -mtune=cortex-a53 -### -c %s 2>&1 | FileCheck -check-prefix=CA53-TUNE %s
 // CA53: "-cc1"{{.*}} "-triple" "aarch64{{.*}}" "-target-cpu" "cortex-a53"
+// CA53-TUNE: "-cc1"{{.*}} "-triple" "aarch64{{.*}}" "-target-cpu" "generic"
 
 // RUN: %clang -target arm64 -mcpu=cortex-a53 -### -c %s 2>&1 | FileCheck -check-prefix=ARM64-CA53 %s
 // RUN: %clang -target arm64 -mlittle-endian -mcpu=cortex-a53 -### -c %s 2>&1 | FileCheck -check-prefix=ARM64-CA53 %s
-// RUN: %clang -target arm64 -mtune=cortex-a53 -### -c %s 2>&1 | FileCheck -check-prefix=ARM64-CA53 %s
-// RUN: %clang -target arm64 -mlittle-endian -mtune=cortex-a53 -### -c %s 2>&1 | FileCheck -check-prefix=ARM64-CA53 %s
+// RUN: %clang -target arm64 -mtune=cortex-a53 -### -c %s 2>&1 | FileCheck -check-prefix=ARM64-CA53-TUNE %s
+// RUN: %clang -target arm64 -mlittle-endian -mtune=cortex-a53 -### -c %s 2>&1 

[PATCH] D39179: [AArch64] Fix PR34625 -mtune without -mcpu should not set -target-cpu

[Peter Smith via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL316424: [AArch64] Fix PR34625 -mtune without -mcpu should 
not set -target-cpu (authored by psmith).

Changed prior to commit:
  https://reviews.llvm.org/D39179?vs=119837&id=120025#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D39179

Files:
  cfe/trunk/lib/Driver/ToolChains/Arch/AArch64.cpp
  cfe/trunk/test/Driver/aarch64-cpus.c

Index: cfe/trunk/test/Driver/aarch64-cpus.c
===
--- cfe/trunk/test/Driver/aarch64-cpus.c
+++ cfe/trunk/test/Driver/aarch64-cpus.c
@@ -21,182 +21,205 @@
 // RUN: %clang -target aarch64 -mcpu=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=CA35 %s
 // RUN: %clang -target aarch64 -mlittle-endian -mcpu=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=CA35 %s
 // RUN: %clang -target aarch64_be -mlittle-endian -mcpu=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=CA35 %s
-// RUN: %clang -target aarch64 -mtune=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=CA35 %s
-// RUN: %clang -target aarch64 -mlittle-endian -mtune=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=CA35 %s
-// RUN: %clang -target aarch64_be -mlittle-endian -mtune=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=CA35 %s
+// RUN: %clang -target aarch64 -mtune=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=CA35-TUNE %s
+// RUN: %clang -target aarch64 -mlittle-endian -mtune=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=CA35-TUNE %s
+// RUN: %clang -target aarch64_be -mlittle-endian -mtune=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=CA35-TUNE %s
 // CA35: "-cc1"{{.*}} "-triple" "aarch64{{.*}}" "-target-cpu" "cortex-a35"
+// CA35-TUNE: "-cc1"{{.*}} "-triple" "aarch64{{.*}}" "-target-cpu" "generic"
 
 // RUN: %clang -target arm64 -mcpu=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=ARM64-CA35 %s
 // RUN: %clang -target arm64 -mlittle-endian -mcpu=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=ARM64-CA35 %s
-// RUN: %clang -target arm64 -mtune=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=ARM64-CA35 %s
-// RUN: %clang -target arm64 -mlittle-endian -mtune=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=ARM64-CA35 %s
+// RUN: %clang -target arm64 -mtune=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=ARM64-CA35-TUNE %s
+// RUN: %clang -target arm64 -mlittle-endian -mtune=cortex-a35 -### -c %s 2>&1 | FileCheck -check-prefix=ARM64-CA35-TUNE %s
 // ARM64-CA35: "-cc1"{{.*}} "-triple" "arm64{{.*}}" "-target-cpu" "cortex-a35"
-
+// ARM64-CA35-TUNE: "-cc1"{{.*}} "-triple" "arm64{{.*}}" "-target-cpu" "generic"
 
 // RUN: %clang -target aarch64 -mcpu=cortex-a53 -### -c %s 2>&1 | FileCheck -check-prefix=CA53 %s
 // RUN: %clang -target aarch64 -mlittle-endian -mcpu=cortex-a53 -### -c %s 2>&1 | FileCheck -check-prefix=CA53 %s
 // RUN: %clang -target aarch64_be -mlittle-endian -mcpu=cortex-a53 -### -c %s 2>&1 | FileCheck -check-prefix=CA53 %s
-// RUN: %clang -target aarch64 -mtune=cortex-a53 -### -c %s 2>&1 | FileCheck -check-prefix=CA53 %s
-// RUN: %clang -target aarch64_be -mlittle-endian -mtune=cortex-a53 -### -c %s 2>&1 | FileCheck -check-prefix=CA53 %s
+// RUN: %clang -target aarch64 -mtune=cortex-a53 -### -c %s 2>&1 | FileCheck -check-prefix=CA53-TUNE %s
+// RUN: %clang -target aarch64_be -mlittle-endian -mtune=cortex-a53 -### -c %s 2>&1 | FileCheck -check-prefix=CA53-TUNE %s
 // CA53: "-cc1"{{.*}} "-triple" "aarch64{{.*}}" "-target-cpu" "cortex-a53"
+// CA53-TUNE: "-cc1"{{.*}} "-triple" "aarch64{{.*}}" "-target-cpu" "generic"
 
 // RUN: %clang -target arm64 -mcpu=cortex-a53 -### -c %s 2>&1 | FileCheck -check-prefix=ARM64-CA53 %s
 // RUN: %clang -target arm64 -mlittle-endian -mcpu=cortex-a53 -### -c %s 2>&1 | FileCheck -check-prefix=ARM64-CA53 %s
-// RUN: %clang -target arm64 -mtune=cortex-a53 -### -c %s 2>&1 | FileCheck -check-prefix=ARM64-CA53 %s
-// RUN: %clang -target arm64 -mlittle-endian -mtune=cortex-a53 -### -c %s 2>&1 | FileCheck -check-prefix=ARM64-CA53 %s
+// RUN: %clang -target arm64 -mtune=cortex-a53 -### -c %s 2>&1 | FileCheck -check-prefix=ARM64-CA53-TUNE %s
+// RUN: %clang -target arm64 -mlittle-endian -mtune=cortex-a53 -### -c %s 2>&1 | FileCheck -check-prefix=ARM64-CA53-TUNE %s
 // ARM64-CA53: "-cc1"{{.*}} "-triple" "arm64{{.*}}" "-target-cpu" "cortex-a53"
+// ARM64-CA53-TUNE: "-cc1"{{.*}} "-triple" "arm64{{.*}}" "-target-cpu" "generic"
 
 // RUN: %clang -target aarch64 -mcpu=cortex-a55 -### -c %s 2>&1 | FileCheck -check-prefix=CA55 %s
 // RUN: %clang -target aarch64 -mlittle-endian -mcpu=cortex-a55 -### -c %s 2>&1 | FileCheck -check-prefix=CA55 %s
 // RUN: %clang -target aarch64_be -mlittle-endian -mcpu=cortex-a55 -### -c %s 2>&1 | FileCheck -check-prefix=CA55 %s
-// RUN: %clang -target aarch64 -mtune=cortex-a55 -### -c %s 2>&1 | FileCheck -check-prefix=CA55 %s
-// RUN: %clang -target aarch64_be -mlittle-endian -mtune=cortex-a55 -### -c %s 2>&1 | FileCheck -check-prefix=CA55 %s

[PATCH] D34513: [NFC] Update to account for DiagnosticRenderer use of FullSourceLoc

[Peter Smith via Phabricator via cfe-commits]

peter.smith created this revision.

https://reviews.llvm.org/D31709 [NFC] Refactor DiagnosticRenderer to use 
FullSourceLoc was committed in r305684 and reverted in 305688 as clang-tidy and 
clang-query failed to build. This change updates the extra tools to use the new 
interface.

With this change https://reviews.llvm.org/D31709 can be recommitted


https://reviews.llvm.org/D34513

Files:
  clang-query/Query.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.cpp

Index: clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -36,10 +36,9 @@
   : DiagnosticRenderer(LangOpts, DiagOpts), Error(Error) {}
 
 protected:
-  void emitDiagnosticMessage(SourceLocation Loc, PresumedLoc PLoc,
+  void emitDiagnosticMessage(FullSourceLoc Loc, PresumedLoc PLoc,
  DiagnosticsEngine::Level Level, StringRef Message,
  ArrayRef Ranges,
- const SourceManager *SM,
  DiagOrStoredDiag Info) override {
 // Remove check name from the message.
 // FIXME: Remove this once there's a better way to pass check names than
@@ -49,35 +48,35 @@
 if (Message.endswith(CheckNameInMessage))
   Message = Message.substr(0, Message.size() - CheckNameInMessage.size());
 
-auto TidyMessage = Loc.isValid()
-   ? tooling::DiagnosticMessage(Message, *SM, Loc)
-   : tooling::DiagnosticMessage(Message);
+auto TidyMessage =
+Loc.isValid()
+? tooling::DiagnosticMessage(Message, Loc.getManager(), Loc)
+: tooling::DiagnosticMessage(Message);
 if (Level == DiagnosticsEngine::Note) {
   Error.Notes.push_back(TidyMessage);
   return;
 }
 assert(Error.Message.Message.empty() && "Overwriting a diagnostic message");
 Error.Message = TidyMessage;
   }
 
-  void emitDiagnosticLoc(SourceLocation Loc, PresumedLoc PLoc,
+  void emitDiagnosticLoc(FullSourceLoc Loc, PresumedLoc PLoc,
  DiagnosticsEngine::Level Level,
- ArrayRef Ranges,
- const SourceManager &SM) override {}
+ ArrayRef Ranges) override {}
 
-  void emitCodeContext(SourceLocation Loc, DiagnosticsEngine::Level Level,
+  void emitCodeContext(FullSourceLoc Loc, DiagnosticsEngine::Level Level,
SmallVectorImpl &Ranges,
-   ArrayRef Hints,
-   const SourceManager &SM) override {
+   ArrayRef Hints) override {
 assert(Loc.isValid());
 for (const auto &FixIt : Hints) {
   CharSourceRange Range = FixIt.RemoveRange;
   assert(Range.getBegin().isValid() && Range.getEnd().isValid() &&
  "Invalid range in the fix-it hint.");
   assert(Range.getBegin().isFileID() && Range.getEnd().isFileID() &&
  "Only file locations supported in fix-it hints.");
 
-  tooling::Replacement Replacement(SM, Range, FixIt.CodeToInsert);
+  tooling::Replacement Replacement(Loc.getManager(), Range,
+   FixIt.CodeToInsert);
   llvm::Error Err = Error.Fix[Replacement.getFilePath()].add(Replacement);
   // FIXME: better error handling (at least, don't let other replacements be
   // applied).
@@ -89,16 +88,13 @@
 }
   }
 
-  void emitIncludeLocation(SourceLocation Loc, PresumedLoc PLoc,
-   const SourceManager &SM) override {}
+  void emitIncludeLocation(FullSourceLoc Loc, PresumedLoc PLoc) override {}
 
-  void emitImportLocation(SourceLocation Loc, PresumedLoc PLoc,
-  StringRef ModuleName,
-  const SourceManager &SM) override {}
+  void emitImportLocation(FullSourceLoc Loc, PresumedLoc PLoc,
+  StringRef ModuleName) override {}
 
-  void emitBuildingModuleLocation(SourceLocation Loc, PresumedLoc PLoc,
-  StringRef ModuleName,
-  const SourceManager &SM) override {}
+  void emitBuildingModuleLocation(FullSourceLoc Loc, PresumedLoc PLoc,
+  StringRef ModuleName) override {}
 
   void endDiagnostic(DiagOrStoredDiag D,
  DiagnosticsEngine::Level Level) override {
@@ -419,11 +415,12 @@
   Errors.back());
   SmallString<100> Message;
   Info.FormatDiagnostic(Message);
-  SourceManager *Sources = nullptr;
-  if (Info.hasSourceManager())
-Sources = &Info.getSourceManager();
-  Converter.emitDiagnostic(Info.getLocation(), DiagLevel, Message,
-   Info.getRanges(), Info.getFixItHints(), Sources);
+  FullSourceLoc Loc =
+  (Info.getLocation().isInvalid())
+  ? FullSourceLoc()
+  : FullSourceLoc(Info.getLocation(), Info.getSourceManag

[PATCH] D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Ping. Does anyone have any changes they'd like me to make?


https://reviews.llvm.org/D52784



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

[PATCH] D53121: [Driver] Add defaults for Android ARM FPUs.

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

It looks like we might be able to simplify a bit, and I think there probably 
could be some more test cases but no objections from me.




Comment at: lib/Driver/ToolChains/Arch/ARM.cpp:360
 
+  unsigned ArchVersion;
+  if (ArchName.empty())

Do you need to parse the arch version here? I would expect the -march=armv7 to 
be reflected in getARMSubArchVersionNumber(Triple)

If reduce this to ArchVersion = getARMSubArchVersionNumber(Triple) and add a 
test with -target arm-linux-androideabi21 -march=armv7 then everything still 
passes.

If I'm right you should be able to simplify this and perhaps roll it into the 
if (Triple.isAndroid() && ArchVersion >= 7) below. If I'm wrong can we add a 
test case that fails without the ArchVersion = 
llvm::ARM::parseArchVersion(ArchName) ?

It will also be worth adding tests for a generic target with -march 


Repository:
  rC Clang

https://reviews.llvm.org/D53121



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

[PATCH] D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Thanks for the comments. I agree with you that the -EB and -EL flags need to be 
passed to the linker as well, I kind of expected that ld.bfd would infer it 
from the endianness of the first object but it doesn't seem to do that. If it's 
ok I'll do that in a separate patch?

I hope I've been able to explain the test. I'm on the fence about passing in 
the triple as a parameter, I have a mild preference for the way it is but if 
you'd like me to change it I can.




Comment at: lib/Driver/ToolChains/Gnu.cpp:543-549
+static const char* GetEndianArg(bool IsBigEndian, const ArgList &Args) {
+  if (Arg *A = Args.getLastArg(options::OPT_mlittle_endian,
+   options::OPT_mbig_endian)) {
+  IsBigEndian = !A->getOption().matches(options::OPT_mlittle_endian);
+  }
+  return (IsBigEndian) ? "-EB" : "-EL";
+}

nickdesaulniers wrote:
> If you passed `llvm::Triple` instead of `bool`, then I think you could do 
> something like:
> 
> ```
> static const char* GetEndianArg(const llvm::Triple &triple, const ArgList 
> &Args) {
>   const bool IsBigEndian = triple == llvm::Triple::armeb ||
>triple == llvm::Triple::thumbeb ||
>triple == llvm::Triple::aarch64_be ||
>Args.getLastArg(options::OPT_mlittle_endian,
>
> options::OPT_mbig_endian)->getOption().matches(options::OPT_mbig_endian);
>   return IsBigEndian ? "-EB" : "-EL";
> }
> ```
> 
> Might encapsulate the logic from the call sites better, but that's a minor 
> nit.
I did think about separating it from the triple. In the end I thought it best 
to follow the existing switch on the triple and put up with a bit of 
duplication.

As it stands I think the above wouldn't quite work as we could have 
--target=armeb-linux-gnueabi -mlittle-endian which would get short-circuited to 
big endian if all the tests are done at once. 

I think it would be possible to do something like:

```
bool IsBigEndian = getTriple().getArch() == llvm::Triple::armeb ||
 getTriple().getArch() == llvm::Triple::thumbeb 
||
 getTriple().getArch() == 
llvm::Triple::aarch64_be;
if (Arg *A = Args.getLastArg(options::OPT_mlittle_endian, 
options::OPT_mbig_endian))
  IsBigEndian = !A->getOption().matches(options::OPT_mlittle_endian);
return IsBigEndian ? "-EB" : "-EL"; 
```
but we'd only want to call it for Arm and AArch64 so I don't think it saves us 
much.





Comment at: lib/Driver/ToolChains/Gnu.cpp:544-547
+  if (Arg *A = Args.getLastArg(options::OPT_mlittle_endian,
+   options::OPT_mbig_endian)) {
+  IsBigEndian = !A->getOption().matches(options::OPT_mlittle_endian);
+  }

nickdesaulniers wrote:
> Just so I understand this check, even if we didn't have a BE triple, we set 
> `IsBigEndian` if `-mlittle-endian` was not set?  Is `-mlittle-endian` a 
> default set flag, or does this set `"-EB"` even if no `-m*-endian` flag is 
> specified (I think we want little-endian to be the default, and IIUC, this 
> would change that)?
I stole the logic from ToolChain::ComputeLLVMTriple in ToolChain.cpp.

On entry to the function IsBigEndian will be set to true if the triple is one 
of armeb, thumbeb or aarch64eb. Otherwise it will be false.

The getLastArg(options::OPT_mlittle_endian, options_mbig_endian) will return 
NULL if neither was set, so we'd default to the value in the triple.




https://reviews.llvm.org/D52784



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

[PATCH] D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

On reflection after looking at what would be needed for the linker 
tools::gnutools::Linker::ConstructJob() I think it may be better to move the 
triple detection into a separate function. I'll work on that and will hopefully 
post an update soon.


https://reviews.llvm.org/D52784



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

[PATCH] D53121: [Driver] Add defaults for Android ARM FPUs.

[Peter Smith via Phabricator via cfe-commits]

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

Looks good to me. By generic target I just meant 
--target=arm-linux-androideabi21 with a -march to specify the revision, which 
you've got in the test.


Repository:
  rC Clang

https://reviews.llvm.org/D53121



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

[PATCH] D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler and linker

[Peter Smith via Phabricator via cfe-commits]

peter.smith updated this revision to Diff 169386.
peter.smith added a comment.

I've decided to roll the linker changes in with the assembler ones as the 
linker use case affects the design. It turns out that only Arm needs to check 
to see if the -mbig-endian and -mlittle-endian flags override the triple as 
computeTargetTriple() accounts for them for AArch64.

I renamed arm::appendEBLinkFlags to arm::appendBE8LinkFlag as it exclusively 
adds "--be8" for Armv7 and above targets.


https://reviews.llvm.org/D52784

Files:
  lib/Driver/ToolChains/Arch/ARM.cpp
  lib/Driver/ToolChains/Arch/ARM.h
  lib/Driver/ToolChains/Gnu.cpp
  lib/Driver/ToolChains/NetBSD.cpp
  test/Driver/linux-as.c
  test/Driver/linux-ld.c

Index: test/Driver/linux-ld.c
===
--- test/Driver/linux-ld.c
+++ test/Driver/linux-ld.c
@@ -1759,6 +1759,7 @@
 // RUN:   | FileCheck --check-prefix=CHECK-ARMEB %s
 // CHECK-ARMEB: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
 // CHECK-ARMEB-NOT: "--be8"
+// CHECK-ARMEB: "-EB"
 // CHECK-ARMEB: "-m" "armelfb_linux_eabi"
 
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
@@ -1768,16 +1769,64 @@
 // RUN:   | FileCheck --check-prefix=CHECK-ARMV7EB %s
 // CHECK-ARMV7EB: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
 // CHECK-ARMV7EB: "--be8"
+// CHECK-ARMV7EB: "-EB"
 // CHECK-ARMV7EB: "-m" "armelfb_linux_eabi"
 
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN: --target=armv7-unknown-linux \
+// RUN: -mbig-endian \
+// RUN: --gcc-toolchain="" \
+// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-ARMV7EB %s
+
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN: --target=armv7-unknown-linux \
+// RUN: --gcc-toolchain="" \
+// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-ARMV7EL %s
+// CHECK-ARMV7EL: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
+// CHECK-ARMV7EL: "-EL"
+// CHECK-ARMV7EL: "-m" "armelf_linux_eabi"
+
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN: --target=armebv7-unknown-linux \
+// RUN: -mlittle-endian \
+// RUN: --gcc-toolchain="" \
+// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-ARMV7EL %s
+
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: --target=aarch64_be-unknown-linux \
 // RUN: --gcc-toolchain="" \
 // RUN: --sysroot=%S/Inputs/basic_linux_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-AARCH64BE %s
 // CHECK-AARCH64BE: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
+// CHECK-AARCH64BE: "-EB"
 // CHECK-AARCH64BE: "-m" "aarch64linuxb"
 
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN: --target=aarch64-unknown-linux \
+// RUN: -mbig-endian \
+// RUN: --gcc-toolchain="" \
+// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-AARCH64BE %s
+
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN: --target=aarch64-unknown-linux \
+// RUN: --gcc-toolchain="" \
+// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-AARCH64LE %s
+// CHECK-AARCH64LE: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
+// CHECK-AARCH64LE: "-EL"
+// CHECK-AARCH64LE: "-m" "aarch64linux"
+
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN: --target=aarch64_be-unknown-linux \
+// RUN: -mlittle-endian \
+// RUN: --gcc-toolchain="" \
+// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-AARCH64LE %s
+
 // Check dynamic-linker for musl-libc
 // RUN: %clang %s -### -o %t.o 2>&1 \
 // RUN: --target=i386-pc-linux-musl \
Index: test/Driver/linux-as.c
===
--- test/Driver/linux-as.c
+++ test/Driver/linux-as.c
@@ -3,129 +3,160 @@
 // RUN: %clang -target arm-linux -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARM %s
-// CHECK-ARM: as{{(.exe)?}}" "-mfloat-abi=soft"
+// CHECK-ARM: as{{(.exe)?}}" "-EL" "-mfloat-abi=soft"
 //
 // RUN: %clang -target arm-linux -mcpu=cortex-a8 -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARM-MCPU %s
-// CHECK-ARM-MCPU: as{{(.exe)?}}" "-mfloat-abi=soft" "-mcpu=cortex-a8"
+// CHECK-ARM-MCPU: as{{(.exe)?}}" "-EL" "-mfloat-abi=soft" "-mcpu=cortex-a8"
 //
 // RUN: %clang -target arm-linux -mfpu=neon -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARM-MFPU %s
-// CHECK-ARM-MFPU: as{{(.exe)?}}" "-mfloat-abi=soft" "-mfpu=neon"
+// CHECK-ARM-MFPU: as{{(.exe)?}}" "-EL" "-mfloat-abi=soft" "-mfpu=neon"
 //
 // RUN: %clang -target arm-linux -march=armv7-a -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=

[PATCH] D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler and linker

[Peter Smith via Phabricator via cfe-commits]

peter.smith marked 7 inline comments as done.
peter.smith added a comment.

Thanks very much for the comments. I'll post an update shortly.




Comment at: lib/Driver/ToolChains/Gnu.cpp:357-364
+const char* EndianFlag = "-EL";
+if (isArmBigEndian(Triple, Args)) {
+  EndianFlag = "-EB";
+  arm::appendBE8LinkFlag(Args, CmdArgs, Triple);
+}
+else if (Arch == llvm::Triple::aarch64_be)
+  EndianFlag = "-EB";

nickdesaulniers wrote:
> nickdesaulniers wrote:
> > ```
> > bool IsBigEndian = isArmBigEndian(Triple, Args);
> > if (IsBigEndian)
> >   arm::appendBE8LinkFlag(Args, CmdArgs, Triple);
> > IsBigEndian |= Arch == llvm::Triple::aarch64_be;
> > CmdArgs.push_back(IsBigEndian ? "-EB" : "-EL");
> > ```
> `IsBigEndian |= Arch == llvm::Triple::aarch64_be;`
> 
> should be:
> 
> `IsBigEndian = IsBigEndian || Arch == llvm::Triple::aarch64_be;`
> 
> in order to not evaluate `Arch == llvm::Triple::aarch64_b` if `IsBigEndian` 
> is already true.
Thanks for the suggestion. One thing it highlighted was that isArmBigEndian 
could return true for an aarch64_be arch with -mbig-endian so I've rewritten 
isArmBigEndian to always return false if the architecture isn't Arm and have 
added some test cases to check that "--be8" doesn't sneak in.



Comment at: lib/Driver/ToolChains/Gnu.cpp:362
+}
+else if (Arch == llvm::Triple::aarch64_be)
+  EndianFlag = "-EB";

nickdesaulniers wrote:
> is having the `else if` on its own line what the formatter chose?
I'd forgot to run clang-format over that part of the code. I've adopted the 
snippet below which replaces it.



Comment at: lib/Driver/ToolChains/Gnu.cpp:703
   case llvm::Triple::aarch64_be: {
+if (getToolChain().getTriple().isLittleEndian())
+  CmdArgs.push_back("-EL");

nickdesaulniers wrote:
> earlier (L362), you check the endianess of the triple with:
> 
> ```
> Arch == llvm::Triple::aarch64_be
> ```
> where `Arch` is `ToolChain.getArch()`.
> 
> I don't have a preference, but these two seem inconsistent.  Can we either 
> check the explicit `llvm::Triple::` or call 
> `getToolChain().getTriple().isLittleEndian()` in both, rather than mix?
I originally took that part from the Mips code, I've replaced it with a check 
against aarch64_be which is more consistent with the other Arm and AArch64 code.


https://reviews.llvm.org/D52784



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

[PATCH] D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler and linker

[Peter Smith via Phabricator via cfe-commits]

peter.smith updated this revision to Diff 169689.
peter.smith marked 3 inline comments as done.
peter.smith added a comment.

Updated diff to reflect review comments. Main changes are:

- isArmBigEndian always returns false if the target architecture isn't Arm.
- Added tests to make sure "--be8" doesn't get added by mistake (would have 
been in previous patch for aarch64_be arch with -mbig-endian flag.


https://reviews.llvm.org/D52784

Files:
  lib/Driver/ToolChains/Arch/ARM.cpp
  lib/Driver/ToolChains/Arch/ARM.h
  lib/Driver/ToolChains/Gnu.cpp
  lib/Driver/ToolChains/NetBSD.cpp
  test/Driver/linux-as.c
  test/Driver/linux-ld.c

Index: test/Driver/linux-ld.c
===
--- test/Driver/linux-ld.c
+++ test/Driver/linux-ld.c
@@ -1759,6 +1759,7 @@
 // RUN:   | FileCheck --check-prefix=CHECK-ARMEB %s
 // CHECK-ARMEB: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
 // CHECK-ARMEB-NOT: "--be8"
+// CHECK-ARMEB: "-EB"
 // CHECK-ARMEB: "-m" "armelfb_linux_eabi"
 
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
@@ -1768,16 +1769,88 @@
 // RUN:   | FileCheck --check-prefix=CHECK-ARMV7EB %s
 // CHECK-ARMV7EB: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
 // CHECK-ARMV7EB: "--be8"
+// CHECK-ARMV7EB: "-EB"
 // CHECK-ARMV7EB: "-m" "armelfb_linux_eabi"
 
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN: --target=armv7-unknown-linux \
+// RUN: -mbig-endian \
+// RUN: --gcc-toolchain="" \
+// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-ARMV7EB %s
+
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN: --target=armebv7-unknown-linux \
+// RUN: -mbig-endian \
+// RUN: --gcc-toolchain="" \
+// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-ARMV7EB %s
+
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN: --target=armv7-unknown-linux \
+// RUN: --gcc-toolchain="" \
+// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-ARMV7EL %s
+// CHECK-ARMV7EL: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
+// CHECK-ARMV7EL-NOT: "--be8"
+// CHECK-ARMV7EL: "-EL"
+// CHECK-ARMV7EL: "-m" "armelf_linux_eabi"
+
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN: --target=armebv7-unknown-linux \
+// RUN: -mlittle-endian \
+// RUN: --gcc-toolchain="" \
+// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-ARMV7EL %s
+
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN: --target=armv7-unknown-linux \
+// RUN: -mlittle-endian \
+// RUN: --gcc-toolchain="" \
+// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-ARMV7EL %s
+
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
 // RUN: --target=aarch64_be-unknown-linux \
 // RUN: --gcc-toolchain="" \
 // RUN: --sysroot=%S/Inputs/basic_linux_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-AARCH64BE %s
 // CHECK-AARCH64BE: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
+// CHECK-AARCH64BE-NOT: "--be8"
+// CHECK-AARCH64BE: "-EB"
 // CHECK-AARCH64BE: "-m" "aarch64linuxb"
 
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN: --target=aarch64-unknown-linux \
+// RUN: -mbig-endian \
+// RUN: --gcc-toolchain="" \
+// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-AARCH64BE %s
+
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN: --target=aarch64_be-unknown-linux \
+// RUN: -mbig-endian \
+// RUN: --gcc-toolchain="" \
+// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-AARCH64BE %s
+
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN: --target=aarch64-unknown-linux \
+// RUN: --gcc-toolchain="" \
+// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-AARCH64LE %s
+// CHECK-AARCH64LE: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
+// CHECK-AARCH64LE-NOT: "--be8"
+// CHECK-AARCH64LE: "-EL"
+// CHECK-AARCH64LE: "-m" "aarch64linux"
+
+// RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
+// RUN: --target=aarch64_be-unknown-linux \
+// RUN: -mlittle-endian \
+// RUN: --gcc-toolchain="" \
+// RUN: --sysroot=%S/Inputs/basic_linux_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-AARCH64LE %s
+
 // Check dynamic-linker for musl-libc
 // RUN: %clang %s -### -o %t.o 2>&1 \
 // RUN: --target=i386-pc-linux-musl \
Index: test/Driver/linux-as.c
===
--- test/Driver/linux-as.c
+++ test/Driver/linux-as.c
@@ -3,129 +3,160 @@
 // RUN: %clang -target arm-linux -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-pre

[PATCH] D53272: Add target requirement to profile remap test.

[Peter Smith via Phabricator via cfe-commits]

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

Looks good to me. REQUIRES: x86-registered-target is used in the same directory 
for tests that depend on specific targets.


Repository:
  rC Clang

https://reviews.llvm.org/D53272



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

[PATCH] D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler and linker

[Peter Smith via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC344597: [ARM][AArch64] Pass through endian flags to 
assembler and linker. (authored by psmith, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D52784?vs=169689&id=169799#toc

Repository:
  rC Clang

https://reviews.llvm.org/D52784

Files:
  lib/Driver/ToolChains/Arch/ARM.cpp
  lib/Driver/ToolChains/Arch/ARM.h
  lib/Driver/ToolChains/Gnu.cpp
  lib/Driver/ToolChains/NetBSD.cpp
  test/Driver/linux-as.c
  test/Driver/linux-ld.c

Index: lib/Driver/ToolChains/Arch/ARM.h
===
--- lib/Driver/ToolChains/Arch/ARM.h
+++ lib/Driver/ToolChains/Arch/ARM.h
@@ -29,7 +29,7 @@
 StringRef getLLVMArchSuffixForARM(llvm::StringRef CPU, llvm::StringRef Arch,
   const llvm::Triple &Triple);
 
-void appendEBLinkFlags(const llvm::opt::ArgList &Args,
+void appendBE8LinkFlag(const llvm::opt::ArgList &Args,
llvm::opt::ArgStringList &CmdArgs,
const llvm::Triple &Triple);
 enum class ReadTPMode {
Index: lib/Driver/ToolChains/Arch/ARM.cpp
===
--- lib/Driver/ToolChains/Arch/ARM.cpp
+++ lib/Driver/ToolChains/Arch/ARM.cpp
@@ -643,7 +643,7 @@
   return llvm::ARM::getSubArch(ArchKind);
 }
 
-void arm::appendEBLinkFlags(const ArgList &Args, ArgStringList &CmdArgs,
+void arm::appendBE8LinkFlag(const ArgList &Args, ArgStringList &CmdArgs,
 const llvm::Triple &Triple) {
   if (Args.hasArg(options::OPT_r))
 return;
Index: lib/Driver/ToolChains/Gnu.cpp
===
--- lib/Driver/ToolChains/Gnu.cpp
+++ lib/Driver/ToolChains/Gnu.cpp
@@ -228,6 +228,29 @@
   // The types are (hopefully) good enough.
 }
 
+// On Arm the endianness of the output file is determined by the target and
+// can be overridden by the pseudo-target flags '-mlittle-endian'/'-EL' and
+// '-mbig-endian'/'-EB'. Unlike other targets the flag does not result in a
+// normalized triple so we must handle the flag here.
+static bool isArmBigEndian(const llvm::Triple &Triple,
+   const ArgList &Args) {
+  bool IsBigEndian = false;
+  switch (Triple.getArch()) {
+  case llvm::Triple::armeb:
+  case llvm::Triple::thumbeb:
+IsBigEndian = true;
+  case llvm::Triple::arm:
+  case llvm::Triple::thumb:
+if (Arg *A = Args.getLastArg(options::OPT_mlittle_endian,
+   options::OPT_mbig_endian))
+  IsBigEndian = !A->getOption().matches(options::OPT_mlittle_endian);
+break;
+  default:
+break;
+  }
+  return IsBigEndian;
+}
+
 static const char *getLDMOption(const llvm::Triple &T, const ArgList &Args) {
   switch (T.getArch()) {
   case llvm::Triple::x86:
@@ -240,10 +263,9 @@
 return "aarch64linuxb";
   case llvm::Triple::arm:
   case llvm::Triple::thumb:
-return "armelf_linux_eabi";
   case llvm::Triple::armeb:
   case llvm::Triple::thumbeb:
-return "armelfb_linux_eabi";
+return isArmBigEndian(T, Args) ? "armelfb_linux_eabi" : "armelf_linux_eabi";
   case llvm::Triple::ppc:
 return "elf32ppclinux";
   case llvm::Triple::ppc64:
@@ -337,8 +359,13 @@
   if (Args.hasArg(options::OPT_s))
 CmdArgs.push_back("-s");
 
-  if (Arch == llvm::Triple::armeb || Arch == llvm::Triple::thumbeb)
-arm::appendEBLinkFlags(Args, CmdArgs, Triple);
+  if (Triple.isARM() || Triple.isThumb() || Triple.isAArch64()) {
+bool IsBigEndian = isArmBigEndian(Triple, Args);
+if (IsBigEndian)
+  arm::appendBE8LinkFlag(Args, CmdArgs, Triple);
+IsBigEndian = IsBigEndian || Arch == llvm::Triple::aarch64_be;
+CmdArgs.push_back(IsBigEndian ? "-EB" : "-EL");
+  }
 
   // Most Android ARM64 targets should enable the linker fix for erratum
   // 843419. Only non-Cortex-A53 devices are allowed to skip this flag.
@@ -640,6 +667,7 @@
   case llvm::Triple::thumb:
   case llvm::Triple::thumbeb: {
 const llvm::Triple &Triple2 = getToolChain().getTriple();
+CmdArgs.push_back(isArmBigEndian(Triple2, Args) ? "-EB" : "-EL");
 switch (Triple2.getSubArch()) {
 case llvm::Triple::ARMSubArch_v7:
   CmdArgs.push_back("-mfpu=neon");
@@ -672,6 +700,8 @@
   }
   case llvm::Triple::aarch64:
   case llvm::Triple::aarch64_be: {
+CmdArgs.push_back(
+getToolChain().getArch() == llvm::Triple::aarch64_be ? "-EB" : "-EL");
 Args.AddLastArg(CmdArgs, options::OPT_march_EQ);
 normalizeCPUNamesForAssembler(Args, CmdArgs);
 
Index: lib/Driver/ToolChains/NetBSD.cpp
===
--- lib/Driver/ToolChains/NetBSD.cpp
+++ lib/Driver/ToolChains/NetBSD.cpp
@@ -164,7 +164,7 @@
 break;
   case llvm::Triple::armeb:
   case llvm::Triple::thumbeb:
-arm::appendEBLinkFlags(Args, CmdArgs, ToolChain.getEffectiveTriple());
+arm::appendBE8LinkFlag(Args, CmdArgs, Tool

[PATCH] D45240: [ARM] Compute a target feature which corresponds to the ARM version.

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

In https://reviews.llvm.org/D45240#1267846, @stefson wrote:

> hey there, I've run into problems with stripping static-libs on arm when 
> using llvm/clang-7. Could you imagine this patch being at fault?
>
>   strip: armv7a-unknown-linux-gnueabihf-strip --strip-unneeded -R .comment -R 
> .GCC.command.line -R .note.gnu.gold-version
>   lib/libbz2.so.1.0.6
>   usr/bin/bzip2recover
>   bin/bzip2
>   usr/lib/libbz2.a
>armv7a-unknown-linux-gnueabihf-strip: 
> /var/tmp/portage/app-arch/bzip2-1.0.6-r10/image/usr/lib/stImUpsE/bzlib.o: 
> Failed to find link section for section 11
>armv7a-unknown-linux-gnueabihf-strip: 
> /var/tmp/portage/app-arch/bzip2-1.0.6-r10/image/usr/lib/stImUpsE/bzlib.o: 
> Failed to find link section for section 11


It doesn't seem that likely to me. The error you are seeing looks like a broken 
object file, the patch here will affect the type of features a target might use 
during code generation but it shouldn't affect strip.

I suggest dumping the ELF file with readelf or objdump and look at the section 
output for section 11. Some sections have a special meaning for the sh_link 
field (http://www.sco.com/developers/gabi/latest/ch4.sheader.html#sh_link) it 
looks like strip is complaining that either a section is missing a sh_link 
field that it should have or it has an invalid index.

It will also be worth seeing if the objects in that library have been processed 
by any other tool such as objcpy to see if it has introduced an error.


Repository:
  rL LLVM

https://reviews.llvm.org/D45240



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

[PATCH] D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler and linker

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Thanks for pointing that out, I'm out of office today, will look at describing 
the intention to fall through when I get back in on Monday.




Comment at: lib/Driver/ToolChains/Gnu.cpp:241
+  case llvm::Triple::thumbeb:
+IsBigEndian = true;
+  case llvm::Triple::arm:

xbolva00 wrote:
> Gnu.cpp:241:17: warning: this statement may fall through 
> [-Wimplicit-fallthrough=]
>  IsBigEndian = true;
The fall through is intentional in this case.


Repository:
  rC Clang

https://reviews.llvm.org/D52784



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

[PATCH] D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler and linker

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Added LLVM_FALLTHROUGH; in r344890.


Repository:
  rC Clang

https://reviews.llvm.org/D52784



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

[PATCH] D56650: [lld] [ELF] Support customizing behavior on target triple

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

I think we need to be very careful here. If I've understood this correctly then 
if this functionality is used for anything critical then a manually supplied 
target will be needed when doing cross-linking. For example my host LLD is 
x86_64, is just called ld.lld and will have an inferred x86_64 target triple. 
If someone customises the behaviour of LLD on the triple in a way that doesn't 
get caught by the test suite then we could get some strange breakages when 
doing cross-linking. I personally would prefer to see any option like this not 
try and auto-infer the target unless it can do it reliably and accurately from 
the input objects and I don't know if that is possible for all supported 
targets.

I think this might be better raised on lllvm-dev as I suspect that we need to 
give this wider visibility. I'm not totally opposed to this as I can see that 
--target has some advantages over adding extra emulations, or relying on the 
--target in the compiler driver but I think we need to be careful to define how 
this will interact with the emulation, and what the bounds of what we can 
customise with the option are?


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

https://reviews.llvm.org/D56650



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

[PATCH] D55029: set default max-page-size to 4KB in lld for Android Aarch64

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Yes this is fine. The effects are entirely within the Android target.

The change in LLD to a 64k page size was made in D25079 
. The main reason given was that a sufficient 
number of linux distributions including Redhat had chosen a 64k page size. GNU 
ld had changed its default page-size to 64k so that applications and libraries 
would always be compatible with distributions that had chosen 4k page size but 
not necessarily vice versa. So it was essentially a safe default.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55029



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

[PATCH] D55029: set default max-page-size to 4KB in lld for Android Aarch64

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

In D55029#1313120 , @ruiu wrote:

> LGTM. Please commit.
>
> Peter, I wonder if you are fine with the default 64KiB page size with lld, 
> especially given that lld always round up the text segment size to the 
> maximum page size on disk and fill the padding with trap instructions. On 
> average, that should increase the executable size by 32 KiB compared to other 
> linkers. I don't think that size is necessarily bad, because we are doing 
> that for a security purpose, but I wonder if people are OK with that.


I think the default is fine at 64KiB . Going back to 4KiB risks breaking 
programs that currently use default options on platforms that have chosen 64KiB 
which I think is a step too far. So far the concern about ELF file size has 
come from Android, where we have a separate target in clang where it is fairly 
easy to pass a 4KiB page size by default. There is a chance that this may 
change in the future as more general AArch64 linux platforms start being 
deployed on devices with limited storage. I guess at that point we could 
consider implementing common-page-size if it were a problem to pass 4KiB page 
size.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55029



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

[PATCH] D58320: [Darwin] Introduce a new flag, -fapple-link-rtlib that forces linking of the builtins library.

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

I've no objections to adding the command line as a Darwin only option. 
Implementation looks fine to me although I've not got any prior experience with 
Darwin.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58320



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

[PATCH] D60472: [AArch64][PowerPC][Driver] Allow setting crypto feature through -mcrypto for ARM/AArch64

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

I'm not in favour of adding AArch64 support to -mcrypto -mnocrypto for a few 
reasons:

- Arm are trying to keep the options for controlling target features as 
consistent as possible with GCC and this option isn't supported in GCC 
(https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html)
- There is http://lists.llvm.org/pipermail/llvm-dev/2018-September/126346.html 
which is Arm's proposal for how target options can be better supported in 
Clang. I think that supporting -mcrypto as well would add more complexity as 
there are interactions between the options.
- Arm 32-bit also supports crypto so this would need to be added to Arm as well 
for consistency.

Can you expand on why you need -m[no]crypto?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60472



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

[PATCH] D60472: [AArch64][PowerPC][Driver] Allow setting crypto feature through -mcrypto for ARM/AArch64

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

In D60472#1461336 , @manojgupta wrote:

> The motivation for this change is to make "crypto" setting an additive option 
> e.g. like "-mavx" used in many media packages.  Some packages in Chrome want 
> to enable crypto conditionally for a few files to allow crypto feature to be 
> used based on runtime cpu detection. They set "-march=armv8+crypto" flag but 
> it gets overridden by the global "-march=armv8a" flag set by the build system 
> in Chrome OS because the target cpu does not support crypto causing 
> compile-time errors. 
>  Ability to specify "-mcrypto"  standalone makes it an additive option and 
> ensures that it it is not lost. i.e. this will help in decoupling  "-mcrypto" 
> from "-march" so that they could be set independently. The current additive 
> alternate is  '-Xclang -target-feature -Xclang "+crypto" ' which is ugly.


Is that not a limitation of the build system? I'd expect a package to be able 
to locally override a global default rather than vice-versa. Although crypto 
might be the area of concern here and there may be a conveniently named option 
for PPC, where does this stop? Crypto is not the only architectural extension, 
for example see https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html . To 
maintain a consistent interface we'd need command line options for all the 
extensions. May I encourage you to reply to the RFC on command line options 
that I mentioned earlier if it doesn't work for you? I think the extensions 
need to be considered as a whole and not just individually.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60472



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

[PATCH] D52595: [ARM] Alter test to account for change to armv6k default CPU

[Peter Smith via Phabricator via cfe-commits]

peter.smith created this revision.
peter.smith added reviewers: falstaff84, nickdesaulniers, rengolin, labrinea.
Herald added a reviewer: javed.absar.
Herald added subscribers: chrib, kristof.beyls.

Review https://reviews.llvm.org/D52594 will change the default in llvm for 
armv6k from the non-existent cpu arm1176jf-s to mpcore. The tests in 
arm-cortex-cpus.c need to be updated to account for this change.

The full discussion of why the change has been made can be found in 
https://reviews.llvm.org/D52594 and https://reviews.llvm.org/D18086


https://reviews.llvm.org/D52595

Files:
  test/Driver/arm-cortex-cpus.c


Index: test/Driver/arm-cortex-cpus.c
===
--- test/Driver/arm-cortex-cpus.c
+++ test/Driver/arm-cortex-cpus.c
@@ -73,11 +73,11 @@
 
 // RUN: %clang -target armv6k -### -c %s 2>&1 | FileCheck 
-check-prefix=CHECK-V6K %s
 // RUN: %clang -target arm -march=armv6k -### -c %s 2>&1 | FileCheck 
-check-prefix=CHECK-V6K %s
-// CHECK-V6K: "-cc1"{{.*}} "-triple" "armv6k-{{.*}} "-target-cpu" "arm1176j-s"
+// CHECK-V6K: "-cc1"{{.*}} "-triple" "armv6k-{{.*}} "-target-cpu" "mpcore"
 
 // RUN: %clang -target armv6k -mthumb -### -c %s 2>&1 | FileCheck 
-check-prefix=CHECK-V6K-THUMB %s
 // RUN: %clang -target arm -march=armv6k -mthumb -### -c %s 2>&1 | FileCheck 
-check-prefix=CHECK-V6K-THUMB %s
-// CHECK-V6K-THUMB: "-cc1"{{.*}} "-triple" "thumbv6k-{{.*}} "-target-cpu" 
"arm1176j-s"
+// CHECK-V6K-THUMB: "-cc1"{{.*}} "-triple" "thumbv6k-{{.*}} "-target-cpu" 
"mpcore"
 
 // RUN: %clang -target armv6t2 -### -c %s 2>&1 | FileCheck 
-check-prefix=CHECK-V6T2 %s
 // RUN: %clang -target arm -march=armv6t2 -### -c %s 2>&1 | FileCheck 
-check-prefix=CHECK-V6T2 %s


Index: test/Driver/arm-cortex-cpus.c
===
--- test/Driver/arm-cortex-cpus.c
+++ test/Driver/arm-cortex-cpus.c
@@ -73,11 +73,11 @@
 
 // RUN: %clang -target armv6k -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V6K %s
 // RUN: %clang -target arm -march=armv6k -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V6K %s
-// CHECK-V6K: "-cc1"{{.*}} "-triple" "armv6k-{{.*}} "-target-cpu" "arm1176j-s"
+// CHECK-V6K: "-cc1"{{.*}} "-triple" "armv6k-{{.*}} "-target-cpu" "mpcore"
 
 // RUN: %clang -target armv6k -mthumb -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V6K-THUMB %s
 // RUN: %clang -target arm -march=armv6k -mthumb -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V6K-THUMB %s
-// CHECK-V6K-THUMB: "-cc1"{{.*}} "-triple" "thumbv6k-{{.*}} "-target-cpu" "arm1176j-s"
+// CHECK-V6K-THUMB: "-cc1"{{.*}} "-triple" "thumbv6k-{{.*}} "-target-cpu" "mpcore"
 
 // RUN: %clang -target armv6t2 -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V6T2 %s
 // RUN: %clang -target arm -march=armv6t2 -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V6T2 %s
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52595: [ARM] Alter test to account for change to armv6k default CPU

[Peter Smith via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL343304: [ARM] Alter test to account for change to armv6k 
default CPU (authored by psmith, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D52595?vs=167264&id=167442#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D52595

Files:
  cfe/trunk/test/Driver/arm-cortex-cpus.c


Index: cfe/trunk/test/Driver/arm-cortex-cpus.c
===
--- cfe/trunk/test/Driver/arm-cortex-cpus.c
+++ cfe/trunk/test/Driver/arm-cortex-cpus.c
@@ -73,11 +73,11 @@
 
 // RUN: %clang -target armv6k -### -c %s 2>&1 | FileCheck 
-check-prefix=CHECK-V6K %s
 // RUN: %clang -target arm -march=armv6k -### -c %s 2>&1 | FileCheck 
-check-prefix=CHECK-V6K %s
-// CHECK-V6K: "-cc1"{{.*}} "-triple" "armv6k-{{.*}} "-target-cpu" "arm1176j-s"
+// CHECK-V6K: "-cc1"{{.*}} "-triple" "armv6k-{{.*}} "-target-cpu" "mpcore"
 
 // RUN: %clang -target armv6k -mthumb -### -c %s 2>&1 | FileCheck 
-check-prefix=CHECK-V6K-THUMB %s
 // RUN: %clang -target arm -march=armv6k -mthumb -### -c %s 2>&1 | FileCheck 
-check-prefix=CHECK-V6K-THUMB %s
-// CHECK-V6K-THUMB: "-cc1"{{.*}} "-triple" "thumbv6k-{{.*}} "-target-cpu" 
"arm1176j-s"
+// CHECK-V6K-THUMB: "-cc1"{{.*}} "-triple" "thumbv6k-{{.*}} "-target-cpu" 
"mpcore"
 
 // RUN: %clang -target armv6t2 -### -c %s 2>&1 | FileCheck 
-check-prefix=CHECK-V6T2 %s
 // RUN: %clang -target arm -march=armv6t2 -### -c %s 2>&1 | FileCheck 
-check-prefix=CHECK-V6T2 %s


Index: cfe/trunk/test/Driver/arm-cortex-cpus.c
===
--- cfe/trunk/test/Driver/arm-cortex-cpus.c
+++ cfe/trunk/test/Driver/arm-cortex-cpus.c
@@ -73,11 +73,11 @@
 
 // RUN: %clang -target armv6k -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V6K %s
 // RUN: %clang -target arm -march=armv6k -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V6K %s
-// CHECK-V6K: "-cc1"{{.*}} "-triple" "armv6k-{{.*}} "-target-cpu" "arm1176j-s"
+// CHECK-V6K: "-cc1"{{.*}} "-triple" "armv6k-{{.*}} "-target-cpu" "mpcore"
 
 // RUN: %clang -target armv6k -mthumb -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V6K-THUMB %s
 // RUN: %clang -target arm -march=armv6k -mthumb -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V6K-THUMB %s
-// CHECK-V6K-THUMB: "-cc1"{{.*}} "-triple" "thumbv6k-{{.*}} "-target-cpu" "arm1176j-s"
+// CHECK-V6K-THUMB: "-cc1"{{.*}} "-triple" "thumbv6k-{{.*}} "-target-cpu" "mpcore"
 
 // RUN: %clang -target armv6t2 -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V6T2 %s
 // RUN: %clang -target arm -march=armv6t2 -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V6T2 %s
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52595: [ARM] Alter test to account for change to armv6k default CPU

[Peter Smith via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC343304: [ARM] Alter test to account for change to armv6k 
default CPU (authored by psmith, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D52595

Files:
  test/Driver/arm-cortex-cpus.c


Index: test/Driver/arm-cortex-cpus.c
===
--- test/Driver/arm-cortex-cpus.c
+++ test/Driver/arm-cortex-cpus.c
@@ -73,11 +73,11 @@
 
 // RUN: %clang -target armv6k -### -c %s 2>&1 | FileCheck 
-check-prefix=CHECK-V6K %s
 // RUN: %clang -target arm -march=armv6k -### -c %s 2>&1 | FileCheck 
-check-prefix=CHECK-V6K %s
-// CHECK-V6K: "-cc1"{{.*}} "-triple" "armv6k-{{.*}} "-target-cpu" "arm1176j-s"
+// CHECK-V6K: "-cc1"{{.*}} "-triple" "armv6k-{{.*}} "-target-cpu" "mpcore"
 
 // RUN: %clang -target armv6k -mthumb -### -c %s 2>&1 | FileCheck 
-check-prefix=CHECK-V6K-THUMB %s
 // RUN: %clang -target arm -march=armv6k -mthumb -### -c %s 2>&1 | FileCheck 
-check-prefix=CHECK-V6K-THUMB %s
-// CHECK-V6K-THUMB: "-cc1"{{.*}} "-triple" "thumbv6k-{{.*}} "-target-cpu" 
"arm1176j-s"
+// CHECK-V6K-THUMB: "-cc1"{{.*}} "-triple" "thumbv6k-{{.*}} "-target-cpu" 
"mpcore"
 
 // RUN: %clang -target armv6t2 -### -c %s 2>&1 | FileCheck 
-check-prefix=CHECK-V6T2 %s
 // RUN: %clang -target arm -march=armv6t2 -### -c %s 2>&1 | FileCheck 
-check-prefix=CHECK-V6T2 %s


Index: test/Driver/arm-cortex-cpus.c
===
--- test/Driver/arm-cortex-cpus.c
+++ test/Driver/arm-cortex-cpus.c
@@ -73,11 +73,11 @@
 
 // RUN: %clang -target armv6k -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V6K %s
 // RUN: %clang -target arm -march=armv6k -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V6K %s
-// CHECK-V6K: "-cc1"{{.*}} "-triple" "armv6k-{{.*}} "-target-cpu" "arm1176j-s"
+// CHECK-V6K: "-cc1"{{.*}} "-triple" "armv6k-{{.*}} "-target-cpu" "mpcore"
 
 // RUN: %clang -target armv6k -mthumb -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V6K-THUMB %s
 // RUN: %clang -target arm -march=armv6k -mthumb -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V6K-THUMB %s
-// CHECK-V6K-THUMB: "-cc1"{{.*}} "-triple" "thumbv6k-{{.*}} "-target-cpu" "arm1176j-s"
+// CHECK-V6K-THUMB: "-cc1"{{.*}} "-triple" "thumbv6k-{{.*}} "-target-cpu" "mpcore"
 
 // RUN: %clang -target armv6t2 -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V6T2 %s
 // RUN: %clang -target arm -march=armv6t2 -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V6T2 %s
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52705: First-pass at ARM hard/soft-float multilib driver (NOT WORKING YET)

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

I think that you may be better off posting a RFC to llvm-dev to get discussion 
on the best approach here. It would also be good to step back a bit and 
consider the requirements, as it stands it looks like this might be a solution 
for just one particular multilib layout and we might be able to get some input 
on some other use cases that others may have. I personally would like to enable 
clang to be able to query a gcc installation to find out (and potentially 
cache) its multlib directories as I've been told for the Arm embedded toolchain 
that the directory structure is not fixed and may change at any point in the 
future.

Another unrelated thing that may be worth doing is to check for other places 
where the target might affect the code-generation independently of -mfloat-abi, 
I'm thinking in particular of builtin functions as this has changed a bit over 
the past few years.




Comment at: lib/Driver/ToolChains/Gnu.cpp:1441
 
+static bool findArmEABIMultilibs(const Driver &D,
+ const llvm::Triple &TargetTriple,

When I saw EABIMultilibs I first thought of multilib support for arm-none-eabi 
, I think that this may be worth renaming to something like ArmLinuxGNUEABI as 
this could be specific to Linux Distributions. 



Comment at: lib/Driver/ToolChains/Gnu.cpp:1451
+  bool DefaultHardFloat = TargetTriple.isHardFloatEABI();
+  llvm::Triple AltTriple(DefaultHardFloat ?
+ TargetTriple.getSoftFloatArchVariant() :

AltTriple doesn't seem to be used anywhere in the rest of the function?



Comment at: lib/Driver/ToolChains/Gnu.cpp:1454
+ TargetTriple.getHardFloatArchVariant());
+  // We assume the Debian libdir/includedir arrangement for armel/armhf,
+  // since Debian and its descendents are apparently the only common Linux

Another place where this is used, and clang can't yet handle, is the GCC 
Embedded toolchain arm-none-eabi which includes both hard and soft float abi 
libraries. These have a different structure to the one presented here though.   



Comment at: lib/Driver/ToolChains/Gnu.cpp:1458
+  // SuSE and RedHat seem to stick with hard-float only and no libdir suffix.
+  // TODO: this (and a lot of other code here) could be generalized via the
+  // output of "gcc  --print-multi-os-dir".

I definitely agree here. It seems to me to be quite fragile to hard-code in 
directory structures of gcc toolchains or linux distributions that can, and do, 
vary over time.


Repository:
  rC Clang

https://reviews.llvm.org/D52705



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

[PATCH] D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler

[Peter Smith via Phabricator via cfe-commits]

peter.smith created this revision.
peter.smith added reviewers: rengolin, compnerd, olista01.
Herald added subscribers: chrib, kristof.beyls, srhines.
Herald added a reviewer: javed.absar.

The big-endian arm32 Linux builds are currently failing when the -mbig-endian 
and -fno-use-integrated-as flags are used and the assembler default is little 
endian. We are not registering that the clang -mbig-endian flag needs to be 
passed through to the assembler to override its default target.

  

The patch always passes through -EL or -BE to the assembler, taking into 
account the target and the -mbig-endian and -mlittle-endian flag.

  

Fixes pr38770

FIXME: While fixing this I note that giving the endianness in march does not 
override the endianness in the triple. This is not specific to 
-fno-integrated-as so if I write --target=arm-linux-gnueabihf -march=armebv7-a 
then the output is little endian. I've updated the targets in the test so that 
the target endian matches the march endianness.

I think that this behaviour is wrong for a couple of reasons:

- -march=armebv7-a should either give an error message if it disagrees with the 
triple or should override the triple.
- The GNU assembler doesn't understand eb in march so we should ideally not 
pass it through, or rely on people to not use march that way.

This can be fixed but I've not done so in this patch.


https://reviews.llvm.org/D52784

Files:
  lib/Driver/ToolChains/Gnu.cpp
  test/Driver/linux-as.c

Index: test/Driver/linux-as.c
===
--- test/Driver/linux-as.c
+++ test/Driver/linux-as.c
@@ -3,129 +3,140 @@
 // RUN: %clang -target arm-linux -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARM %s
-// CHECK-ARM: as{{(.exe)?}}" "-mfloat-abi=soft"
+// CHECK-ARM: as{{(.exe)?}}" "-EL" "-mfloat-abi=soft"
 //
 // RUN: %clang -target arm-linux -mcpu=cortex-a8 -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARM-MCPU %s
-// CHECK-ARM-MCPU: as{{(.exe)?}}" "-mfloat-abi=soft" "-mcpu=cortex-a8"
+// CHECK-ARM-MCPU: as{{(.exe)?}}" "-EL" "-mfloat-abi=soft" "-mcpu=cortex-a8"
 //
 // RUN: %clang -target arm-linux -mfpu=neon -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARM-MFPU %s
-// CHECK-ARM-MFPU: as{{(.exe)?}}" "-mfloat-abi=soft" "-mfpu=neon"
+// CHECK-ARM-MFPU: as{{(.exe)?}}" "-EL" "-mfloat-abi=soft" "-mfpu=neon"
 //
 // RUN: %clang -target arm-linux -march=armv7-a -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARM-MARCH %s
-// CHECK-ARM-MARCH: as{{(.exe)?}}" "-mfloat-abi=soft" "-march=armv7-a"
+// CHECK-ARM-MARCH: as{{(.exe)?}}" "-EL" "-mfloat-abi=soft" "-march=armv7-a"
+//
+// RUN: %clang -target armeb-linux -mlittle-endian -mcpu=cortex-a8 -mfpu=neon -march=armv7-a -### \
+// RUN:   -no-integrated-as -c %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-ARM-ALL %s
+// CHECK-ARM-ALL: as{{(.exe)?}}" "-EL" "-mfloat-abi=soft" "-march=armv7-a" "-mcpu=cortex-a8" "-mfpu=neon"
 //
 // RUN: %clang -target arm-linux -mcpu=cortex-a8 -mfpu=neon -march=armv7-a -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARM-ALL %s
-// CHECK-ARM-ALL: as{{(.exe)?}}" "-mfloat-abi=soft" "-march=armv7-a" "-mcpu=cortex-a8" "-mfpu=neon"
 //
-// RUN: %clang -target arm-linux -mcpu=cortex-a8 -mfpu=neon -march=armebv7-a -### \
+// RUN: %clang -target armeb-linux -mcpu=cortex-a8 -mfpu=neon -march=armebv7-a -### \
+// RUN:   -no-integrated-as -c %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-ARMEB-ALL %s
+// CHECK-ARMEB-ALL: as{{(.exe)?}}" "-EB" "-mfloat-abi=soft" "-march=armebv7-a" "-mcpu=cortex-a8" "-mfpu=neon"
+//
+// RUN: %clang -target arm-linux -mcpu=cortex-a8 -mfpu=neon -march=armebv7-a -mbig-endian -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARMEB-ALL %s
-// CHECK-ARMEB-ALL: as{{(.exe)?}}" "-mfloat-abi=soft" "-march=armebv7-a" "-mcpu=cortex-a8" "-mfpu=neon"
 //
 // RUN: %clang -target thumb-linux -mcpu=cortex-a8 -mfpu=neon -march=thumbv7-a -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-THUMB-ALL %s
-// CHECK-THUMB-ALL: as{{(.exe)?}}" "-mfloat-abi=soft" "-march=thumbv7-a" "-mcpu=cortex-a8" "-mfpu=neon"
+// CHECK-THUMB-ALL: as{{(.exe)?}}" "-EL" "-mfloat-abi=soft" "-march=thumbv7-a" "-mcpu=cortex-a8" "-mfpu=neon"
 //
-// RUN: %clang -target thumb-linux -mcpu=cortex-a8 -mfpu=neon -march=thumbebv7-a -### \
+// RUN: %clang -target thumbeb-linux -mcpu=cortex-a8 -mfpu=neon -march=thumbebv7-a -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-THUMBEB-ALL %s
-// CHECK-THUMBEB-ALL: as{{(.exe)?}}" "-mfloat-abi=soft" "-march=thumbebv7-a" "-mcpu=cortex-a8" "-mfpu=neon"
+// CHECK-THUMBEB-ALL: as{{(.exe)?}}" "-EB" "-mfloat-abi=soft" "-march=thumbebv7-a" "-mcpu=cortex-a8" "-mfpu=neo

[PATCH] D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

In https://reviews.llvm.org/D52784#1252569, @rengolin wrote:

> Hi Peter,
>
> Looks ok. Why did you need to change all cmdlines to have -EL? I imagine you 
> just need one for each case, everything else remains the default (which 
> should still work).


I thought that in general we wouldn't know what the default endianness of the 
assembler we are targeting is so it was safest to be explicit. For a 
cross-toolchain it is probably inherent in the name, but in theory we could be 
running clang on a native big-endian system where the assembler is just called 
as.

> Also, it would be interesting to know what happens on cases like "aarch64_be 
> -EL" et al, and have negative tests for them.

Ok will add some more tests.

> cheers,
> --renato




https://reviews.llvm.org/D52784



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

[PATCH] D52784: [ARM][AArch64] Pass through endianness flags to the GNU assembler

[Peter Smith via Phabricator via cfe-commits]

peter.smith updated this revision to Diff 167955.
peter.smith added a comment.

Updated diff to add more tests, including some that use -mlittle-endian when 
the target is big-endian.


https://reviews.llvm.org/D52784

Files:
  lib/Driver/ToolChains/Gnu.cpp
  test/Driver/linux-as.c

Index: test/Driver/linux-as.c
===
--- test/Driver/linux-as.c
+++ test/Driver/linux-as.c
@@ -3,129 +3,160 @@
 // RUN: %clang -target arm-linux -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARM %s
-// CHECK-ARM: as{{(.exe)?}}" "-mfloat-abi=soft"
+// CHECK-ARM: as{{(.exe)?}}" "-EL" "-mfloat-abi=soft"
 //
 // RUN: %clang -target arm-linux -mcpu=cortex-a8 -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARM-MCPU %s
-// CHECK-ARM-MCPU: as{{(.exe)?}}" "-mfloat-abi=soft" "-mcpu=cortex-a8"
+// CHECK-ARM-MCPU: as{{(.exe)?}}" "-EL" "-mfloat-abi=soft" "-mcpu=cortex-a8"
 //
 // RUN: %clang -target arm-linux -mfpu=neon -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARM-MFPU %s
-// CHECK-ARM-MFPU: as{{(.exe)?}}" "-mfloat-abi=soft" "-mfpu=neon"
+// CHECK-ARM-MFPU: as{{(.exe)?}}" "-EL" "-mfloat-abi=soft" "-mfpu=neon"
 //
 // RUN: %clang -target arm-linux -march=armv7-a -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARM-MARCH %s
-// CHECK-ARM-MARCH: as{{(.exe)?}}" "-mfloat-abi=soft" "-march=armv7-a"
+// CHECK-ARM-MARCH: as{{(.exe)?}}" "-EL" "-mfloat-abi=soft" "-march=armv7-a"
+//
+// RUN: %clang -target armeb-linux -mlittle-endian -mcpu=cortex-a8 -mfpu=neon -march=armv7-a -### \
+// RUN:   -no-integrated-as -c %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-ARM-ALL %s
+// CHECK-ARM-ALL: as{{(.exe)?}}" "-EL" "-mfloat-abi=soft" "-march=armv7-a" "-mcpu=cortex-a8" "-mfpu=neon"
 //
 // RUN: %clang -target arm-linux -mcpu=cortex-a8 -mfpu=neon -march=armv7-a -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARM-ALL %s
-// CHECK-ARM-ALL: as{{(.exe)?}}" "-mfloat-abi=soft" "-march=armv7-a" "-mcpu=cortex-a8" "-mfpu=neon"
 //
-// RUN: %clang -target arm-linux -mcpu=cortex-a8 -mfpu=neon -march=armebv7-a -### \
+// RUN: %clang -target armeb-linux -mlittle-endian -mcpu=cortex-a8 -mfpu=neon -march=armv7-a -### \
+// RUN:   -no-integrated-as -c %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-ARM-ALL %s
+//
+// RUN: %clang -target armeb-linux -mcpu=cortex-a8 -mfpu=neon -march=armebv7-a -### \
+// RUN:   -no-integrated-as -c %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-ARMEB-ALL %s
+// CHECK-ARMEB-ALL: as{{(.exe)?}}" "-EB" "-mfloat-abi=soft" "-march=armebv7-a" "-mcpu=cortex-a8" "-mfpu=neon"
+//
+// RUN: %clang -target arm-linux -mcpu=cortex-a8 -mfpu=neon -march=armebv7-a -mbig-endian -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARMEB-ALL %s
-// CHECK-ARMEB-ALL: as{{(.exe)?}}" "-mfloat-abi=soft" "-march=armebv7-a" "-mcpu=cortex-a8" "-mfpu=neon"
 //
 // RUN: %clang -target thumb-linux -mcpu=cortex-a8 -mfpu=neon -march=thumbv7-a -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-THUMB-ALL %s
-// CHECK-THUMB-ALL: as{{(.exe)?}}" "-mfloat-abi=soft" "-march=thumbv7-a" "-mcpu=cortex-a8" "-mfpu=neon"
+// CHECK-THUMB-ALL: as{{(.exe)?}}" "-EL" "-mfloat-abi=soft" "-march=thumbv7-a" "-mcpu=cortex-a8" "-mfpu=neon"
+//
+// RUN: %clang -target thumbeb-linux -mcpu=cortex-a8 -mfpu=neon -march=thumbv7-a -mlittle-endian -### \
+// RUN:   -no-integrated-as -c %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-THUMB-ALL %s
 //
-// RUN: %clang -target thumb-linux -mcpu=cortex-a8 -mfpu=neon -march=thumbebv7-a -### \
+// RUN: %clang -target thumbeb-linux -mcpu=cortex-a8 -mfpu=neon -march=thumbebv7-a -### \
+// RUN:   -no-integrated-as -c %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-THUMBEB-ALL %s
+// CHECK-THUMBEB-ALL: as{{(.exe)?}}" "-EB" "-mfloat-abi=soft" "-march=thumbebv7-a" "-mcpu=cortex-a8" "-mfpu=neon"
+//
+// RUN: %clang -target thumb-linux -mcpu=cortex-a8 -mfpu=neon -march=thumbebv7-a -mbig-endian -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-THUMBEB-ALL %s
-// CHECK-THUMBEB-ALL: as{{(.exe)?}}" "-mfloat-abi=soft" "-march=thumbebv7-a" "-mcpu=cortex-a8" "-mfpu=neon"
 //
 // RUN: %clang -target armv7-linux -mcpu=cortex-a8 -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARM-TARGET %s
-// CHECK-ARM-TARGET: as{{(.exe)?}}" "-mfpu=neon" "-mfloat-abi=soft" "-mcpu=cortex-a8"
+// CHECK-ARM-TARGET: as{{(.exe)?}}" "-EL" "-mfpu=neon" "-mfloat-abi=soft" "-mcpu=cortex-a8"
 //
 // RUN: %clang -target armebv7-linux -mcpu=cortex-a8 -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARMEB-TARGET %s
-// CHECK-ARMEB-TARGET: as{{(.exe)?}}" "-mfpu=neon" "-mfloat-abi=soft" "-mcpu=co

[PATCH] D58320: [Darwin] Introduce a new flag, -flink-builtins-rt that forces linking of the builtins library.

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

The implementation changes in the Darwin toolchain look fine to me, although 
with respect to the command line option I think Petr Hosek's message on cfe-dev 
is interesting:

> GCC implements -nolibc which could be used to achieve the same effect when 
> combined with -nostartfiles (and -nostdlib++ when compiling C++). I'd prefer 
> that approach not only because it improves compatibility with with GCC, but 
> also because it matches existing flag scheme which is subtractive rather than 
> additive (i.e. -nodefaultlibs, -nostdlib, -nostdlib++, -nostartfiles). Clang 
> already defines this flag but the only toolchain that currently supports it 
> is DragonFly.

Looking at https://gcc.gnu.org/onlinedocs/gcc/Link-Options.html (quoted here 
for convenience)

> -nostartfiles
>  Do not use the standard system startup files when linking. The standard 
> system libraries are used normally, unless -nostdlib, -nolibc, or 
> -nodefaultlibs is used.
> -nolibc
>  Do not use the C library or system libraries tightly coupled with it when 
> linking. Still link with the startup files, libgcc or toolchain provided 
> language support libraries such as libgnat, libgfortran or libstdc++ unless 
> options preventing their inclusion are used as well. This typically removes 
> -lc from the link command line, as well as system libraries that normally go 
> with it and become meaningless when absence of a C library is assumed, for 
> example -lpthread or -lm in some configurations. This is intended for 
> bare-board targets when there is indeed no C library available.

It does seem like these options accomplish what -flink_builtins_rt do with the 
added advantage of being more portable with gcc. If they don't work for you it 
will be worth double checking with Petr.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58320



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

[PATCH] D58314: [Driver] Sync ARM behavior between clang-as and gas.

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

My main concern is that this changes the default behaviour for 
arm-linux-gnueabi and arm-linux-gnueabihf targets. I've put some suggestions on 
what I think the behaviour should be.




Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:277
+  }
+  return "";
+}

I'm a bit worried that we've changed the default behaviour for gnueabi[hf] 
targets here. 
For example with:
```
.text
vmov.32 d13[1], r6 ; Needs VFPv2
vadd.i32 d0, d0, d0 ; Needs Neon
```
I get with --target=armv7a-linux (no environment, -mfloat-abi will disable 
floating point, and neon)
```
clang: warning: unknown platform, assuming -mfloat-abi=soft
neon.s:2:9: error: instruction requires: VFP2
vmov.32 d13[1],r6
^
neon.s:3:9: error: instruction requires: NEON
vadd.i32 d0, d0, d0
^
```
With the target=armv7a-linux-gnueabi armv7a-linux-gnueabihf or explicitly 
adding -mfloat-abi=softfp the integrated assembler will happily assemble it.
GNU needs -mfpu=neon to assemble the file:

```
arm-linux-gnueabihf-as -march=armv7-a neon.s 
neon.s: Assembler messages:
neon.s:2: Error: selected processor does not support ARM mode `vmov.32 
d13[1],r6'
neon.s:3: Error: selected processor does not support ARM mode `vadd.i32 
d0,d0,d0'
```
It is a similar story for armv8 and crypto.

I think we should have something like:
```
if (Triple.isLinux() && getARMSubArchVersionNumber(Triple) >= 8)
   return "crypto-neon-fp-armv8";
if (Triple.isAndroid() || Triple.isLinux() && 
getARMSubArchVersionNumber(Triple) >= 7)
return "neon";
return "";
```



Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:389
+std::string DefaultFPU = getDefaultFPUName(Triple);
+if (DefaultFPU != "") {
+  if (!llvm::ARM::getFPUFeatures(llvm::ARM::parseFPU(DefaultFPU), 
Features))

I'm wondering whether you need this bit of code anymore? In D53121 there needed 
to be a switch between vfpv3-d16 and neon based on Android version. With 
--target=armv7a-linux-android or --target=arm-linux-android -march=armv7a or 
any v7a -mcpu applicable to Android then you'll get feature Neon by default and 
won't need to do this? We could then move getDefaultFPUName out of ARM.cpp



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:692
+}
+if (!hasMOrWaMArg(Args, options::OPT_mfpu_EQ, "-mfpu=")) {
+std::string DefaultFPU = arm::getDefaultFPUName(Triple);

I think we'd not want to do this for -mfloat-abi=soft as this disables the FPU 
in the integrated assembler. It seems like -mfloat-abi has no effect at all on 
the gnu assembler, it will happily assemble neon instructions with 
-mfloat-abi=soft -mfpu=neon.
 



Comment at: clang/test/Driver/linux-as.c:3
 //
 // RUN: %clang -target arm-linux -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \

the target arm-linux (effectively arm-linux-unknown) defaults to 
-mfloat-abi=soft which disables the FPU for the integrated assembler. While 
these test cases are not wrong, the number of v7a + linux targets without an 
FPU using entirely software floating point is likely to be very small. We 
should have some more that have arm-linux-gnueabi and arm-linux-gnueabihf.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58314



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

[PATCH] D58429: [CodeGen] Enable the complex-math test for arm

[Peter Smith via Phabricator via cfe-commits]

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

LGTM. Thanks for the fix. My apologies for missing that at the time, it looks 
like a cut and paste error on my part.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58429



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

[PATCH] D58314: [Driver] Sync ARM behavior between clang-as and gas.

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Thanks for the update. To summarise I'd like to keep the integrated and 
non-integrated assembler in synch for Linux like Android, even at the risk of 
adding an fpu when it isn't needed. I think that given how difficult it is to 
not use NEON, I think the mfloat-abi=soft guard you've put on will be 
sufficient. By the way, I'm hoping we aren't talking past each other with the 
default for armv7-a. I've tried to put as much of what I understand in the 
comments and I hope the answers make sense, or you'll be able to correct me 
where I'm wrong. If the timezone delayed comments aren't working well it may be 
worth finding me on IRC, I usually leave the office about 6:30pm but I can 
easily arrange a time and log on later if you wanted to discuss.

I also spotted something else on line 251 of  ARM.cpp with respect to Android 
that might be worth looking at. I've left a comment although it isn't related 
to this patch.




Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:277
+  }
+  return "";
+}

danalbert wrote:
> peter.smith wrote:
> > I'm a bit worried that we've changed the default behaviour for gnueabi[hf] 
> > targets here. 
> > For example with:
> > ```
> > .text
> > vmov.32 d13[1], r6 ; Needs VFPv2
> > vadd.i32 d0, d0, d0 ; Needs Neon
> > ```
> > I get with --target=armv7a-linux (no environment, -mfloat-abi will disable 
> > floating point, and neon)
> > ```
> > clang: warning: unknown platform, assuming -mfloat-abi=soft
> > neon.s:2:9: error: instruction requires: VFP2
> > vmov.32 d13[1],r6
> > ^
> > neon.s:3:9: error: instruction requires: NEON
> > vadd.i32 d0, d0, d0
> > ^
> > ```
> > With the target=armv7a-linux-gnueabi armv7a-linux-gnueabihf or explicitly 
> > adding -mfloat-abi=softfp the integrated assembler will happily assemble it.
> > GNU needs -mfpu=neon to assemble the file:
> > 
> > ```
> > arm-linux-gnueabihf-as -march=armv7-a neon.s 
> > neon.s: Assembler messages:
> > neon.s:2: Error: selected processor does not support ARM mode `vmov.32 
> > d13[1],r6'
> > neon.s:3: Error: selected processor does not support ARM mode `vadd.i32 
> > d0,d0,d0'
> > ```
> > It is a similar story for armv8 and crypto.
> > 
> > I think we should have something like:
> > ```
> > if (Triple.isLinux() && getARMSubArchVersionNumber(Triple) >= 8)
> >return "crypto-neon-fp-armv8";
> > if (Triple.isAndroid() || Triple.isLinux() && 
> > getARMSubArchVersionNumber(Triple) >= 7)
> > return "neon";
> > return "";
> > ```
> I suppose it depends on which direction you want the behavior change to go. I 
> assumed those samples _shouldn't_ assemble since they're not enabling NEON. 
> The fact that the direct `arm-linux-gnueabihf-as` doesn't enable NEON by 
> default makes me assume that NEON is not an assumed feature of the gnueabihf 
> environment.
> 
> It's not up to me whether NEON is available by default for ARMv7 for 
> non-Android, but I do think that the behavior should be consistent regardless 
> of the assembler being used. Right now we have no FPU by default with the 
> integrated assembler and NEON by default with GAS. This change makes GAS have 
> the same behavior as the integrated assembler, since I assume that is the 
> better traveled path and afaict is the one that has had more effort put in to 
> it.
> 
> If that's not right, I can change this so that the integrated assembler 
> _also_ gets FPU features that are not necessarily available for the given 
> architecture, but I wanted to clarify that that is what you're asking for 
> first.
> I suppose it depends on which direction you want the behavior change to go. I 
> assumed those samples _shouldn't_ assemble since they're not enabling NEON. 
> The fact that the direct arm-linux-gnueabihf-as doesn't enable NEON by 
> default makes me assume that NEON is not an assumed feature of the gnueabihf 
> environment.

It is true that LLVM and GNU have unfortunately chosen a different default for 
armv7-a and NEON and armv8-a and crypo. I agree that NEON is not an assumed 
feature of either a gnueabi or gnueabihf GCC toolchain. My understanding is 
that GNU chose to not include any extensions in the base architecture and LLVM 
chose the most common configuration for the default.

>  Right now we have no FPU by default with the integrated assembler and NEON 
> by default with GAS. This change makes GAS have the same behavior as the 
> integrated assembler, since I assume that is the better traveled path and 
> afaict is the one that has had more effort put in to it.

Are you sure that we have no FPU by default with the integrated assembler for 
armv7-a and armv8-a? I've put an explanation in the next comment that explains 
how it gets set even when there is no +neon on the -cc1 or -cc1as command line. 

> If that's not right, I can change this so that the integrated assembler 
> _also_ gets FPU features that are not necessarily available for the given 
> arch

[PATCH] D64415: Consistent types and naming for AArch64 target features (NFC)

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

FWIW I think this looks reasonable. The ARM equivalent uses bitfields such as

  unsigned CRC : 1;
  unsigned Crypto : 1;
  unsigned DSP : 1;
  unsigned Unaligned : 1;
  unsigned DotProd : 1;

Which would make more sense than using unsigned for each individual field. 
Several other targets that I looked at used bool and the Has 
convention so I think this brings AArch64 more in line with non ARM Targets at 
the cost of losing a little coherence with ARM where the name Crypto remains. I 
don't think that this is particularly important as the two have already started 
to diverge with HasDotProd.


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

https://reviews.llvm.org/D64415



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

[PATCH] D65000: [ARM] Set default alignment to 64bits

[Peter Smith via Phabricator via cfe-commits]

peter.smith added reviewers: srhines, danalbert.
peter.smith added a comment.

I think that this may not apply for Android as AFAIK their ABI still requires 
128-bit alignment in some cases. Adding some more reviewers from Android.




Comment at: lib/Basic/Targets/ARM.cpp:311
 
   // Maximum alignment for ARM NEON data types should be 64-bits (AAPCS)
   if (IsAAPCS && (Triple.getEnvironment() != llvm::Triple::Android))

I think that Android can require a higher alignment in some cases (See below). 
I think that this was explained in D33205


Repository:
  rC Clang

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

https://reviews.llvm.org/D65000



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

[PATCH] D65000: [ARM] Set default alignment to 64bits

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Thanks for the update. Will be worth adding some reviewers from Apple to see if 
this change should be IsAAPCS only. I've no more further comments myself 
besides a small nit on style.




Comment at: lib/Basic/Targets/ARM.cpp:331
+   // but Android ABI uses 128-bit alignment as default
+  DefaultAlignForAttributeAligned =
+   (Triple.getEnvironment() == llvm::Triple::Android)?128:64;

Nit: suggest running clang-format over the conditional expression I think that 
spaces are needed around ? and :

The previous bit of code used
```
if (IsAAPCS && (Triple.getEnvironment() != llvm::Triple::Android))
MaxVectorAlign = 64;
```
The IsAAPCS could be important here as on Macho targets IsAAPCS may not be 
true, see calls to setABI in ARMTargetInfo::ARMTargetInfo.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65000



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

[PATCH] D65000: [ARM] Set default alignment to 64bits

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

test case missing A8 aside this looks ok to me. Would like to see if there are 
any comments from the Pacific time zone.




Comment at: test/CodeGen/ARM/exception-alignment.cpp:8
+// A16-NEXT: store <2 x i64> , <2 x i64>* [[BC]], align 16
+#include 
+

Are you missing the A8 case? presumably:
```
store <2 x i64> , <2 x i64>* [[BC]], align 8
```



Repository:
  rC Clang

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

https://reviews.llvm.org/D65000



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

[PATCH] D65000: [ARM] Set default alignment to 64bits

[Peter Smith via Phabricator via cfe-commits]

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

I've not seen any objections so I've approved LGTM.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65000



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

[PATCH] D67608: [ARM] Preserve fpu behaviour for '-crypto'

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

After retesting on a failing example and experimenting a bit, I think that we 
should only preserve +crypto with -fno-integrated-as. It would also be good to 
mention D66018  and maybe add the original 
author as a reviewer.




Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:499
 << "crypto" << 
llvm::ARM::getArchName(llvm::ARM::parseArch(ArchSuffix));
-  Features.push_back("-crypto");
+  //-mfpu=crypto-neon-fp-armv8 does allow +sha2 / +aes / +crypto features,
+  // even if compiling for an cpu armv7a, if explicitly defined by the 
user,

Looking into this in more detail I think the comment can be made more specific. 
It seems like the main reason to keep +crypto is that when the 
non-integrated-assembler is used, then we get the directive:
```
.fpu crypto-neon-fp-armv8
```
In arm-none-eabi-as the assembler will support crypto instructions. Clang will 
not as any use of crypto instructions will fail due lack of v8 support.

```
With -fno-integrated-as -mfpu=crypto-neon-fp-armv8 some assemblers such as the 
GNU assembler will permit the use of crypto instructions as the fpu will 
override the architecture. We keep the crypto feature in this case to preserve 
compatibility. In all other cases we remove the crypto feature. 
```



Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:502
+  // so do not deactivate them.
+  if ((llvm::find(Features, "+fp-armv8") == Features.end()) ||
+  (llvm::find(Features, "+neon") == Features.end()))

I think we should only do this for -fno-integrated-as as the integrated 
assembler will fail if give a crypto instruction even with this change. With 
the integrated assembler we get:

```
error: instruction requires: armv8
aese.8 q8, q9
```



Comment at: clang/test/Driver/arm-features.c:84
+// Check +crypto does not affect -march=armv7a -mfpu=crypto-neon-fp-armv8, but 
it does warn that +crypto has no effect
+// RUN: %clang -target arm-arm-none-eabi -march=armv7a 
-mfpu=crypto-neon-fp-armv8 -### -c %s 2>&1 | FileCheck 
-check-prefixes=CHECK-WARNONLY,ALL %s
+// RUN: %clang -target arm-arm-none-eabi -march=armv7a+aes 
-mfpu=crypto-neon-fp-armv8 -### -c %s 2>&1 | FileCheck 
-check-prefixes=CHECK-WARNONLY,ALL,CHECK-HASAES %s

arm-arm-none-eabi is a vendor specific triple? Better to use arm-none-eabi 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67608



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

[PATCH] D67608: [ARM] Preserve fpu behaviour for '-crypto'

[Peter Smith via Phabricator via cfe-commits]

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

Thanks for the update. That looks good to me. Will be good to wait for a day 
before committing to give US timezone a chance to object.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67608



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

[PATCH] D80828: [Clang][A32/T32][Linux] -O2 implies -fomit-frame-pointer

[Peter Smith via Phabricator via cfe-commits]

psmith added a comment.

FWIW I'm happy for Clang to match GCC behaviour for Linux (non-Android) targets 
with respect to the frame pointer. Outside of Android I would expect most 
developers of linux applications to build with both GCC and clang so they 
should already have the -fno-omit-frame-pointer if they are using it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80828



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

[PATCH] D80828: [Clang][A32/T32][Linux] -O1 implies -fomit-frame-pointer

[Peter Smith via Phabricator via cfe-commits]

psmith accepted this revision.
psmith added a comment.

LGTM from an Arm person now that the Android changes have been made.




Comment at: clang/lib/Driver/ToolChains/Clang.cpp:22
 #include "InputInfo.h"
+#include "MSP430.h"
 #include "PS4CPU.h"

nickdesaulniers wrote:
> Note to reviewers: the linter really wanted to touch this header inclusion 
> list, since I add "llvm/Support/Compiler.h" below.  Hopefully it's ok to just 
> include with this patch?
One possible solution here is to commit the reordering separately using an 
[NFC] refactoring commit, no separate review required, then commit the rest of 
the patch after that. Anyone doing a source code search will only pick up 
significant changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80828



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

[PATCH] D78511: [Driver][doc] Document option -mtune as a no-op. NFC.

[Peter Smith via Phabricator via cfe-commits]

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

That wording looks good to me. I've accepted the change, but worth waiting a 
day or so to see if there are any objections or suggestions.


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

https://reviews.llvm.org/D78511



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

[PATCH] D78565: [clang][doc] Clang ARM CPU command line argument reference

[Peter Smith via Phabricator via cfe-commits]

psmith added a comment.

One question I can't answer and I think would need wider review, is whether 
this is type of material (common options for specific CPUs) is suited for the 
Clang Documentation or whether it would be better hosted by Arm itself, for 
example on developer.arm.com? I think that a more reference like Arm CPU 
options like https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html would work 
better in the Clang documentation.

I think the material is useful, especially for people new to these CPUs.




Comment at: clang/docs/ClangARMCPUsCLI.rst:27
+
+.. option:: --target=arm-arm-none-eabi -mcpu=cortex-m33 -mfpu=fp-armv8-sp-d16 
-mfloat-abi=hard -mthumb
+

I think you mean arm-arm-none-eabi for an upstream triple selecting the 
bare-metal driver?


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

https://reviews.llvm.org/D78565



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

[PATCH] D85239: [DOCS] Add more detail to stack protector documentation

[Peter Smith via Phabricator via cfe-commits]

psmith created this revision.
psmith added reviewers: jfb, Shayne, emaste, kristof.beyls, mattdr, ojhunt, 
probinson, reames, serge-sans-paille, dim.
Herald added a subscriber: dexonsmith.
psmith requested review of this revision.

The Clang -fstack-protector documentation mentions what functions are 
considered vulnerable but does not mention the details of the
implementation such as the use of a global variable for the guard value. This 
brings the documentation more in line with the GCC
documentation at: 
https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html and gives 
someone using the option more idea about what is protected.

This partly addresses https://bugs.llvm.org/show_bug.cgi?id=42764


https://reviews.llvm.org/D85239

Files:
  clang/docs/ClangCommandLineReference.rst


Index: clang/docs/ClangCommandLineReference.rst
===
--- clang/docs/ClangCommandLineReference.rst
+++ clang/docs/ClangCommandLineReference.rst
@@ -2136,7 +2136,7 @@
 
 .. option:: -fstack-protector, -fno-stack-protector
 
-Enable stack protectors for some functions vulnerable to stack smashing. This 
uses a loose heuristic which considers functions vulnerable if they contain a 
char (or 8bit integer) array or constant sized calls to alloca, which are of 
greater size than ssp-buffer-size (default: 8 bytes). All variable sized calls 
to alloca are considered vulnerable
+Enable stack protectors for some functions vulnerable to stack smashing. This 
uses a loose heuristic which considers functions vulnerable if they contain a 
char (or 8bit integer) array or constant sized calls to alloca, which are of 
greater size than ssp-buffer-size (default: 8 bytes). All variable sized calls 
to alloca are considered vulnerable. A function with a stack protector has a 
guard value added to the stack frame that is checked on function exit. The 
guard value must be positioned in the stack frame such that a buffer overflow 
from a vulnerable variable will overwrite to the guard value before overwriting 
the function's return address. The reference stack guard value is stored in a 
global variable.
 
 .. option:: -fstack-protector-all
 


Index: clang/docs/ClangCommandLineReference.rst
===
--- clang/docs/ClangCommandLineReference.rst
+++ clang/docs/ClangCommandLineReference.rst
@@ -2136,7 +2136,7 @@
 
 .. option:: -fstack-protector, -fno-stack-protector
 
-Enable stack protectors for some functions vulnerable to stack smashing. This uses a loose heuristic which considers functions vulnerable if they contain a char (or 8bit integer) array or constant sized calls to alloca, which are of greater size than ssp-buffer-size (default: 8 bytes). All variable sized calls to alloca are considered vulnerable
+Enable stack protectors for some functions vulnerable to stack smashing. This uses a loose heuristic which considers functions vulnerable if they contain a char (or 8bit integer) array or constant sized calls to alloca, which are of greater size than ssp-buffer-size (default: 8 bytes). All variable sized calls to alloca are considered vulnerable. A function with a stack protector has a guard value added to the stack frame that is checked on function exit. The guard value must be positioned in the stack frame such that a buffer overflow from a vulnerable variable will overwrite to the guard value before overwriting the function's return address. The reference stack guard value is stored in a global variable.
 
 .. option:: -fstack-protector-all
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85239: [DOCS] Add more detail to stack protector documentation

[Peter Smith via Phabricator via cfe-commits]

psmith added a comment.

In D85239#2194604 , @MaskRay wrote:

> This file is auto generated. The real documentation should go to 
> `clang/include/clang/Driver/Options.td`

Thanks for pointing that out! I regenerated the ClangCommandLineReference.rst 
from Options.td but it looks like I'm not the only one that missed the comment 
on the top line. There are 27 differences in the file including some that are 
not in Options.td. I'm loathe to lose work by regenerating the whole file so 
I've included just the diff created by the change to Options.td. At least if 
the file is synched up again then we'll keep the edits.

Assuming this is still OK I'll commit tomorrow.


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

https://reviews.llvm.org/D85239

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

[PATCH] D85239: [DOCS] Add more detail to stack protector documentation

[Peter Smith via Phabricator via cfe-commits]

psmith updated this revision to Diff 283201.
psmith added a comment.
Herald added a subscriber: dang.

uploaded diff with both Options.td and ClangCommandLineReference.rst


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

https://reviews.llvm.org/D85239

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/include/clang/Driver/Options.td


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1801,10 +1801,15 @@
"as well as any calls to alloca or the taking of an address from a 
local variable">;
 def fstack_protector : Flag<["-"], "fstack-protector">, Group,
   HelpText<"Enable stack protectors for some functions vulnerable to stack 
smashing. "
-   "This uses a loose heuristic which considers functions vulnerable "
-   "if they contain a char (or 8bit integer) array or constant sized 
calls to "
-   "alloca, which are of greater size than ssp-buffer-size (default: 8 
bytes). "
-   "All variable sized calls to alloca are considered vulnerable">;
+   "This uses a loose heuristic which considers functions vulnerable 
if they "
+   "contain a char (or 8bit integer) array or constant sized calls to 
alloca "
+   ", which are of greater size than ssp-buffer-size (default: 8 
bytes). All "
+   "variable sized calls to alloca are considered vulnerable. A 
function with"
+   "a stack protector has a guard value added to the stack frame that 
is "
+   "checked on function exit. The guard value must be positioned in 
the "
+   "stack frame such that a buffer overflow from a vulnerable variable 
will "
+   "overwrite the guard value before overwriting the function's return 
"
+   "address. The reference stack guard value is stored in a global 
variable.">;
 def ftrivial_auto_var_init : Joined<["-"], "ftrivial-auto-var-init=">, 
Group,
   Flags<[CC1Option, CoreOption]>, HelpText<"Initialize trivial automatic stack 
variables: uninitialized (default)"
   " | pattern">, Values<"uninitialized,pattern">;
Index: clang/docs/ClangCommandLineReference.rst
===
--- clang/docs/ClangCommandLineReference.rst
+++ clang/docs/ClangCommandLineReference.rst
@@ -2136,7 +2136,7 @@
 
 .. option:: -fstack-protector, -fno-stack-protector
 
-Enable stack protectors for some functions vulnerable to stack smashing. This 
uses a loose heuristic which considers functions vulnerable if they contain a 
char (or 8bit integer) array or constant sized calls to alloca, which are of 
greater size than ssp-buffer-size (default: 8 bytes). All variable sized calls 
to alloca are considered vulnerable
+Enable stack protectors for some functions vulnerable to stack smashing. This 
uses a loose heuristic which considers functions vulnerable if they contain a 
char (or 8bit integer) array or constant sized calls to alloca , which are of 
greater size than ssp-buffer-size (default: 8 bytes). All variable sized calls 
to alloca are considered vulnerable. A function witha stack protector has a 
guard value added to the stack frame that is checked on function exit. The 
guard value must be positioned in the stack frame such that a buffer overflow 
from a vulnerable variable will overwrite the guard value before overwriting 
the function's return address. The reference stack guard value is stored in a 
global variable.
 
 .. option:: -fstack-protector-all
 


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1801,10 +1801,15 @@
"as well as any calls to alloca or the taking of an address from a local variable">;
 def fstack_protector : Flag<["-"], "fstack-protector">, Group,
   HelpText<"Enable stack protectors for some functions vulnerable to stack smashing. "
-   "This uses a loose heuristic which considers functions vulnerable "
-   "if they contain a char (or 8bit integer) array or constant sized calls to "
-   "alloca, which are of greater size than ssp-buffer-size (default: 8 bytes). "
-   "All variable sized calls to alloca are considered vulnerable">;
+   "This uses a loose heuristic which considers functions vulnerable if they "
+   "contain a char (or 8bit integer) array or constant sized calls to alloca "
+   ", which are of greater size than ssp-buffer-size (default: 8 bytes). All "
+   "variable sized calls to alloca are considered vulnerable. A function with"
+   "a stack protector has a guard value added to the stack frame that is "
+   "checked on function exit. The guard value must be positioned in the "
+   "stack frame such that a buffer overflow from a vu

[PATCH] D85239: [DOCS] Add more detail to stack protector documentation

[Peter Smith via Phabricator via cfe-commits]

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



Comment at: clang/docs/ClangCommandLineReference.rst:2139
 
-Enable stack protectors for some functions vulnerable to stack smashing. This 
uses a loose heuristic which considers functions vulnerable if they contain a 
char (or 8bit integer) array or constant sized calls to alloca, which are of 
greater size than ssp-buffer-size (default: 8 bytes). All variable sized calls 
to alloca are considered vulnerable
+Enable stack protectors for some functions vulnerable to stack smashing. This 
uses a loose heuristic which considers functions vulnerable if they contain a 
char (or 8bit integer) array or constant sized calls to alloca, which are of 
greater size than ssp-buffer-size (default: 8 bytes). All variable sized calls 
to alloca are considered vulnerable. A function with a stack protector has a 
guard value added to the stack frame that is checked on function exit. The 
guard value must be positioned in the stack frame such that a buffer overflow 
from a vulnerable variable will overwrite to the guard value before overwriting 
the function's return address. The reference stack guard value is stored in a 
global variable.
 

probinson wrote:
> "overwrite to the guard variable" -> "overwrite the guard variable"
Thanks, now updated


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

https://reviews.llvm.org/D85239

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

[PATCH] D85239: [DOCS] Add more detail to stack protector documentation

[Peter Smith via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
psmith marked an inline comment as done.
Closed by commit rG839d974ee0e4: [DOCS] Add more detail to stack protector 
documentation (authored by psmith).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85239

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/include/clang/Driver/Options.td


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1801,10 +1801,15 @@
"as well as any calls to alloca or the taking of an address from a 
local variable">;
 def fstack_protector : Flag<["-"], "fstack-protector">, Group,
   HelpText<"Enable stack protectors for some functions vulnerable to stack 
smashing. "
-   "This uses a loose heuristic which considers functions vulnerable "
-   "if they contain a char (or 8bit integer) array or constant sized 
calls to "
-   "alloca, which are of greater size than ssp-buffer-size (default: 8 
bytes). "
-   "All variable sized calls to alloca are considered vulnerable">;
+   "This uses a loose heuristic which considers functions vulnerable 
if they "
+   "contain a char (or 8bit integer) array or constant sized calls to 
alloca "
+   ", which are of greater size than ssp-buffer-size (default: 8 
bytes). All "
+   "variable sized calls to alloca are considered vulnerable. A 
function with"
+   "a stack protector has a guard value added to the stack frame that 
is "
+   "checked on function exit. The guard value must be positioned in 
the "
+   "stack frame such that a buffer overflow from a vulnerable variable 
will "
+   "overwrite the guard value before overwriting the function's return 
"
+   "address. The reference stack guard value is stored in a global 
variable.">;
 def ftrivial_auto_var_init : Joined<["-"], "ftrivial-auto-var-init=">, 
Group,
   Flags<[CC1Option, CoreOption]>, HelpText<"Initialize trivial automatic stack 
variables: uninitialized (default)"
   " | pattern">, Values<"uninitialized,pattern">;
Index: clang/docs/ClangCommandLineReference.rst
===
--- clang/docs/ClangCommandLineReference.rst
+++ clang/docs/ClangCommandLineReference.rst
@@ -2136,7 +2136,7 @@
 
 .. option:: -fstack-protector, -fno-stack-protector
 
-Enable stack protectors for some functions vulnerable to stack smashing. This 
uses a loose heuristic which considers functions vulnerable if they contain a 
char (or 8bit integer) array or constant sized calls to alloca, which are of 
greater size than ssp-buffer-size (default: 8 bytes). All variable sized calls 
to alloca are considered vulnerable
+Enable stack protectors for some functions vulnerable to stack smashing. This 
uses a loose heuristic which considers functions vulnerable if they contain a 
char (or 8bit integer) array or constant sized calls to alloca , which are of 
greater size than ssp-buffer-size (default: 8 bytes). All variable sized calls 
to alloca are considered vulnerable. A function witha stack protector has a 
guard value added to the stack frame that is checked on function exit. The 
guard value must be positioned in the stack frame such that a buffer overflow 
from a vulnerable variable will overwrite the guard value before overwriting 
the function's return address. The reference stack guard value is stored in a 
global variable.
 
 .. option:: -fstack-protector-all
 


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1801,10 +1801,15 @@
"as well as any calls to alloca or the taking of an address from a local variable">;
 def fstack_protector : Flag<["-"], "fstack-protector">, Group,
   HelpText<"Enable stack protectors for some functions vulnerable to stack smashing. "
-   "This uses a loose heuristic which considers functions vulnerable "
-   "if they contain a char (or 8bit integer) array or constant sized calls to "
-   "alloca, which are of greater size than ssp-buffer-size (default: 8 bytes). "
-   "All variable sized calls to alloca are considered vulnerable">;
+   "This uses a loose heuristic which considers functions vulnerable if they "
+   "contain a char (or 8bit integer) array or constant sized calls to alloca "
+   ", which are of greater size than ssp-buffer-size (default: 8 bytes). All "
+   "variable sized calls to alloca are considered vulnerable. A function with"
+   "a stack protector has a guard value added to the stack frame that is "
+   "checked 

[PATCH] D85239: [DOCS] Add more detail to stack protector documentation

[Peter Smith via Phabricator via cfe-commits]

psmith added a comment.

I agree it's not what I'd like. I'm not sure how to react to MaskRays comment. 
The top of the ClangCommandLineReference.rst says:

  ---
  NOTE: This file is automatically generated by running clang-tblgen
  -gen-opt-docs. Do not edit this file by hand!!
  ---

This uses Options.td to generate the file. I don't know how true this is 
anymore given that both files are checked in. They may have been separated. 
I've seen at least one instance in the log where the whole file has been 
regenerated.

I'm happy to revert the Options.td change if there is another way?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85239

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

[PATCH] D133109: [LLVM][ARM] Remove options for armv2, 2A, 3 and 3M

[Peter Smith via Phabricator via cfe-commits]

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

LGTM. GCC no longer supports Arm architecture prior to v4 so it is likely the 
alternative of adding support for v3 is not worth it. The only Arm machines 
running v2 are likely to be the Acorn Archimedes family, these have their own 
object file format so are unlikely to benefit from LLVM support.

Agree that the openmp reference looks harmless.

Will be worth waiting to see if Nick has any objections.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133109

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

[PATCH] D71688: [AArch64] Add -mtls-size option for ELF targets

[Peter Smith via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG10c11e4e2d05: This option allows selecting the TLS size in 
the local exec TLS model, which is… (authored by kawashima-fj, committed by 
peter.smith).
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71688

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Driver/tls-size.c
  llvm/include/llvm/CodeGen/CommandFlags.inc
  llvm/include/llvm/Target/TargetOptions.h
  llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
  llvm/lib/Target/AArch64/AArch64ISelLowering.h
  llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
  llvm/test/CodeGen/AArch64/arm64-tls-execs.ll
  llvm/test/CodeGen/AArch64/arm64-tls-initial-exec.ll
  llvm/test/CodeGen/AArch64/arm64-tls-local-exec.ll

Index: llvm/test/CodeGen/AArch64/arm64-tls-local-exec.ll
===
--- /dev/null
+++ llvm/test/CodeGen/AArch64/arm64-tls-local-exec.ll
@@ -0,0 +1,106 @@
+; Test each TLS size option
+; RUN: llc -mtriple=arm64-none-linux-gnu -verify-machineinstrs -show-mc-encoding -tls-size=12 < %s | FileCheck %s --check-prefix=CHECK-12
+; RUN: llc -mtriple=arm64-none-linux-gnu -filetype=obj < %s -tls-size=12 | llvm-objdump -r - | FileCheck --check-prefix=CHECK-12-RELOC %s
+; RUN: llc -mtriple=arm64-none-linux-gnu -verify-machineinstrs -show-mc-encoding -code-model=tiny -tls-size=24 < %s | FileCheck %s --check-prefix=CHECK-24
+; RUN: llc -mtriple=arm64-none-linux-gnu -filetype=obj < %s -code-model=tiny -tls-size=24 | llvm-objdump -r - | FileCheck --check-prefix=CHECK-24-RELOC %s
+; RUN: llc -mtriple=arm64-none-linux-gnu -verify-machineinstrs -show-mc-encoding -code-model=small -tls-size=32 < %s | FileCheck %s --check-prefix=CHECK-32
+; RUN: llc -mtriple=arm64-none-linux-gnu -filetype=obj < %s -code-model=small -tls-size=32 | llvm-objdump -r - | FileCheck --check-prefix=CHECK-32-RELOC %s
+; RUN: llc -mtriple=arm64-none-linux-gnu -verify-machineinstrs -show-mc-encoding -code-model=large -tls-size=48 < %s | FileCheck %s --check-prefix=CHECK-48
+; RUN: llc -mtriple=arm64-none-linux-gnu -filetype=obj < %s -code-model=large -tls-size=48 | llvm-objdump -r - | FileCheck --check-prefix=CHECK-48-RELOC %s
+;
+; Test the maximum TLS size for each code model (fallback to a smaller size from the specified size)
+; RUN: llc -mtriple=arm64-none-linux-gnu -verify-machineinstrs -show-mc-encoding -tls-size=32 < %s | FileCheck %s --check-prefix=CHECK-32
+; RUN: llc -mtriple=arm64-none-linux-gnu -filetype=obj < %s -tls-size=32 | llvm-objdump -r - | FileCheck --check-prefix=CHECK-32-RELOC %s
+; RUN: llc -mtriple=arm64-none-linux-gnu -verify-machineinstrs -show-mc-encoding -code-model=tiny -tls-size=32 < %s | FileCheck %s --check-prefix=CHECK-24
+; RUN: llc -mtriple=arm64-none-linux-gnu -filetype=obj < %s -code-model=tiny -tls-size=32 | llvm-objdump -r - | FileCheck --check-prefix=CHECK-24-RELOC %s
+; RUN: llc -mtriple=arm64-none-linux-gnu -verify-machineinstrs -show-mc-encoding -code-model=small -tls-size=48 < %s | FileCheck %s --check-prefix=CHECK-32
+; RUN: llc -mtriple=arm64-none-linux-gnu -filetype=obj < %s -code-model=small -tls-size=48 | llvm-objdump -r - | FileCheck --check-prefix=CHECK-32-RELOC %s
+;
+; Test the default TLS size for each code model
+; RUN: llc -mtriple=arm64-none-linux-gnu -verify-machineinstrs -show-mc-encoding < %s | FileCheck --check-prefix=CHECK-24 %s
+; RUN: llc -mtriple=arm64-none-linux-gnu -filetype=obj < %s | llvm-objdump -r - | FileCheck --check-prefix=CHECK-24-RELOC %s
+; RUN: llc -mtriple=arm64-none-linux-gnu -verify-machineinstrs -show-mc-encoding -code-model=tiny < %s | FileCheck %s --check-prefix=CHECK-24
+; RUN: llc -mtriple=arm64-none-linux-gnu -filetype=obj < %s -code-model=tiny | llvm-objdump -r - | FileCheck --check-prefix=CHECK-24-RELOC %s
+; RUN: llc -mtriple=arm64-none-linux-gnu -verify-machineinstrs -show-mc-encoding -code-model=small < %s | FileCheck %s --check-prefix=CHECK-24
+; RUN: llc -mtriple=arm64-none-linux-gnu -filetype=obj < %s -code-model=small | llvm-objdump -r - | FileCheck --check-prefix=CHECK-24-RELOC %s
+; RUN: llc -mtriple=arm64-none-linux-gnu -verify-machineinstrs -show-mc-encoding -code-model=large < %s | FileCheck %s --check-prefix=CHECK-24
+; RUN: llc -mtriple=arm64-none-linux-gnu -filetype=obj < %s -code-model=large | llvm-objdump -r - | FileCheck --check-prefix=CHECK-24-RELOC %s
+
+@local_exec_var = thread_local(localexec) global i32 0
+
+define i32 @test_local_exec() {
+; CHECK-LABEL: test_local_exec:
+  %val = load i32, i32* @local_exec_var
+
+; CHECK-12: mrs x[[R1:[0-9]+]], TPIDR_EL0
+; CHECK-12: add x[[R2:[0-9]+]], x[[R1]], :tprel_lo12:local_exec_var
+; CHECK-12: ldr w0, [x[[R

[PATCH] D71688: [AArch64] Add -mtls-size option for ELF targets

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Committed as 10c11e4e2d05cf0e8f8251f50d84ce77eb1e9b8d 
 , I've 
used the new attribution process https://llvm.org/docs/DeveloperPolicy.html so 
you should show up as the author of the patch in the commit log. I'll comment 
here if there are any problems with buildbots.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71688



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

[PATCH] D91760: [Driver] Default Generic_GCC aarch64 to use -fasynchronous-unwind-tables

[Peter Smith via Phabricator via cfe-commits]

psmith added a comment.

Personally I'm in favour of clang and gcc behaving the same way unless there is 
a good reason not to. I've shared the review internally to see if anyone has 
any concerns. May be worth informing the clang built linux community to see if 
they will need to make any alterations as a result.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91760

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

[PATCH] D91760: [Driver] Default Generic_GCC aarch64 to use -fasynchronous-unwind-tables

[Peter Smith via Phabricator via cfe-commits]

psmith added a comment.

Radio silence so far; I think no news is good news applies in this case. I'm 
happy to say no objections.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91760

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

[PATCH] D101873: [clang] Support clang -fpic -fno-semantic-interposition for AArch64

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Doing this on 32-bit Arm would make me nervous as STT_FUNC symbols encode the 
state of Arm/Thumb in the bottom bit, but STT_NOTYPE symbols do not. In 
principle it could be done but extra care would have to be taken to make sure 
no state changes were required. For example caller and callee would need to be 
in the same state. I'm not entirely sure that LLD's current range-extension 
thunks would work to STT_NOTYPE symbols as they use BX IP, which would always 
state change to Arm due to the STT_NOTYPE symbol having the bottom bit clear. 
This is fixable but is the additional complexity and possible fragility of 
older tools worth it?

https://github.com/ARM-software/abi-aa/blob/main/aaelf32/aaelf32.rst#554symbol-names


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101873

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

[PATCH] D112600: [ARM] Use hardware TLS register in Thumb2 mode when -mtp=cp15 is passed

[Peter Smith via Phabricator via cfe-commits]

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

LGTM as this as CP15 can be used on architecture v6k and above, which maps to 
IsThumb2.

As an aside from this patch, the Arm state could be considered too permissive 
as it will permit -mtls=cp15 on architectures that wouldn't have the 
coprocessor register like arm7tdmi, although GCC also permits this so I guess 
we're in the set this option with care territory.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112600

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

[PATCH] D112600: [ARM] Use hardware TLS register in Thumb2 mode when -mtp=cp15 is passed

[Peter Smith via Phabricator via cfe-commits]

peter.smith added inline comments.



Comment at: clang/test/CodeGen/arm-tphard.c:10
+}
+

nickdesaulniers wrote:
> ardb wrote:
> > nickdesaulniers wrote:
> > > Let's make this a test under llvm/test/CodeGen/, using IR:
> > > ```
> > > ; RUN: llc --mtriple=armv7-linux-gnueabihf -o - %s | FileCheck %s
> > > ; RUN: llc --mtriple=thumbv7-linux-gnu -o - %s | FileCheck %s
> > > define dso_local i8* @tphard() "target-features"="+read-tp-hard" {
> > > // CHECK-NOT: __aeabi_read_tp
> > >   %1 = tail call i8* @llvm.thread.pointer()
> > >   ret i8* %1
> > > }
> > > 
> > > declare i8* @llvm.thread.pointer()
> > > ```
> > > 
> > > Let's make this a test under llvm/test/CodeGen/, using IR:
> > 
> > Why is that better?
> > 
> > > ```
> > > ; RUN: llc --mtriple=armv7-linux-gnueabihf -o - %s | FileCheck %s
> > > ; RUN: llc --mtriple=thumbv7-linux-gnu -o - %s | FileCheck %s
> > 
> > Are you sure using this triple forces generation of Thumb2 code? It didn't 
> > seem to when I tried.
> > 
> > > define dso_local i8* @tphard() "target-features"="+read-tp-hard" {
> > > // CHECK-NOT: __aeabi_read_tp
> > 
> > Are you sure __aeabi_read_tp will appear in the IR for soft TP?
> > 
> > >   %1 = tail call i8* @llvm.thread.pointer()
> > >   ret i8* %1
> > > }
> > > 
> > > declare i8* @llvm.thread.pointer()
> > > ```
> > > 
> > 
> > 
> > Why is that better?
> 
> Because the front-end isn't really involved in this back end code gen bug, so 
> we should just be testing the back end.
> 
> > Are you sure using this triple forces generation of Thumb2 code? It didn't 
> > seem to when I tried.
> 
> Good question; I guess for thumb2 there's no command line flag passed to the 
> compiler that says "I would like thumb[1] as opposed to thumb2?"  Maybe 
> @peter.smith can provide some color on that? Is it simply armv7 for thumb2 vs 
> v6 for thumb[1], or something else?
> 
> > Are you sure __aeabi_read_tp will appear in the IR for soft TP?
> 
> `__aeabi_read_tp` will never appear in the IR (unless someone explicitly 
> called it`. Instead, we're testing that the intrinsic 
> (`@llvm.thread.pointer`) is lowered to either that libcall or `mrc`.  
> `__aeabi_read_tp` may appear in the object file or assembler code generated 
> from that intrinsic call.
> 
> But also, it looks like there's already coverage for `__aeabi_read_tp` being 
> generated for soft TP. See also llvm/test/CodeGen/ARM/readtp.ll. There's also 
> thread_pointer.ll in that same dir (and a few more tests mentioning 
> `__aeabi_read_tp` that all look like candidates to add these tests to rather 
> than creating a new test file, perhaps.
> > Are you sure using this triple forces generation of Thumb2 code? It didn't 
> > seem to when I tried.
> 
> Good question; I guess for thumb2 there's no command line flag passed to the 
> compiler that says "I would like thumb[1] as opposed to thumb2?"  Maybe 
> @peter.smith can provide some color on that? Is it simply armv7 for thumb2 vs 
> v6 for thumb[1], or something else?

As I understand it, "Thumb2 Technology" to give it the marketing name is still 
the Thumb (T32) ISA, it just has access to a lot more instructions than Thumb. 
In the backend this means that the compiler is free to select instructions with 
the Thumb2 predicate. It doesn't have to if there is an equivalent 2-byte sized 
Thumb instruction that does the job.

In theory to get Thumb only code -march=armv6 might work, armv6 is the 
intersection of the A, R and M profiles that include no Thumb 2.

The architecture for Thumb 2 is close to v7+ although like most things Arm it 
is more complicated:
* Armv6k as implemented by one CPU Arm1165t2-s (which I wouldn't expect to see 
running Linux). All other Arm v6 CPUs including the Arm 1176jzf-s (original 
Raspberry Pi) do not have Thumb 2.
* All Arm v7 CPUs have Thumb 2, including M, R and A profiles.
* All Arm v8 R and A profile CPUs have Thumb 2 as do Armv8-M.mainline from M 
profile. The Armv8-M.baseline CPUs do not (these are the smallest 
microcontrollers). 




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112600

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

[PATCH] D112768: [ARM] implement support for TLS register based stack protector

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a reviewer: ostannard.
peter.smith added a comment.

Adding ostannard to reviewers list. I'm going to be on vacation next week and 
Oliver is more familiar with this area than I am.

To prevent the option in Clang for targets that don't support Thumb2 it may be 
worth looking into clang/lib/Basic/Targets/ARM.cpp for example 
https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/Targets/ARM.cpp#L174


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112768

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

[PATCH] D114116: [clang][ARM] relax -mtp=cp15 for ARMv6 non-thumb cases

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Apologies had to go on a bit of a dive through the documentation. I've put some 
comments inline and some links to other documents that may help.




Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:150
 
+// The backend does not have support for hard thread pointers when targeting
+// Thumb1.

Will be worth saying `CP15 C13 ThreadID` somewhere to make it easier to look up 
in the documentation.



Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:155
+  llvm::ARM::ArchKind AK = llvm::ARM::parseArch(Triple.getArchName());
+  return Ver >= 7 || AK == llvm::ARM::ArchKind::ARMV6T2 ||
+ (Ver == 6 && Triple.isARM());

Are we restricting based on whether the threadid register is present or whether 
the instructions are available to access the cp15 registers?

If we're going by whether the CPU has the register present then it will be
* A and R profile (not M profile, even the ones that have Thumb2)
* V6K (includes ARM1176 but not ARM1156t2-s which has Thumb-2!) and V7+ (A and 
R profile)

If we're going by the instructions to write to CP15 then it is:
* Arm state (everything)
* Thumb2 (v7 + v6t2)

The above seems to be a blend of the two. Is it worth choosing one form or the 
other? GCC seems to use the latter. I guess using this option falls into the I 
know what I'm doing area that accessing named system registers comes into. If 
the kernel supports it the stricter version may help catch more mistakes though.

The v7 A/R Arm ARM https://developer.arm.com/documentation/ddi0403/ed
has in `D12.7.21 CP15 c13, Context ID support`
``` An ARMv6K implementation requires the Software Thread ID registers 
described in VMSA CP15 c13
register summary, Process, context and thread ID registers on page B3-1474. ```

The Arm 1156-s (the only v6t2 processor) TRM 
https://developer.arm.com/documentation/ddi0338/g/system-control-coprocessor/system-control-processor-registers/c13--process-id-register?lang=en
 which shows only one process ID register under opcode 1 accessed via:
```
MRC p15, 0, , c13, c0, 1 ;Read Process ID Register
```
Whereas the ThreadID register is opcode 3 on CPUs that are v6k and v7.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114116

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

[PATCH] D114116: [clang][ARM] relax -mtp=cp15 for ARMv6 non-thumb cases

[Peter Smith via Phabricator via cfe-commits]

peter.smith added inline comments.



Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:155
+  llvm::ARM::ArchKind AK = llvm::ARM::parseArch(Triple.getArchName());
+  return Ver >= 7 || AK == llvm::ARM::ArchKind::ARMV6T2 ||
+ (Ver == 6 && Triple.isARM());

nickdesaulniers wrote:
> ardb wrote:
> > peter.smith wrote:
> > > Are we restricting based on whether the threadid register is present or 
> > > whether the instructions are available to access the cp15 registers?
> > > 
> > > If we're going by whether the CPU has the register present then it will be
> > > * A and R profile (not M profile, even the ones that have Thumb2)
> > > * V6K (includes ARM1176 but not ARM1156t2-s which has Thumb-2!) and V7+ 
> > > (A and R profile)
> > > 
> > > If we're going by the instructions to write to CP15 then it is:
> > > * Arm state (everything)
> > > * Thumb2 (v7 + v6t2)
> > > 
> > > The above seems to be a blend of the two. Is it worth choosing one form 
> > > or the other? GCC seems to use the latter. I guess using this option 
> > > falls into the I know what I'm doing area that accessing named system 
> > > registers comes into. If the kernel supports it the stricter version may 
> > > help catch more mistakes though.
> > > 
> > > The v7 A/R Arm ARM https://developer.arm.com/documentation/ddi0403/ed
> > > has in `D12.7.21 CP15 c13, Context ID support`
> > > ``` An ARMv6K implementation requires the Software Thread ID registers 
> > > described in VMSA CP15 c13
> > > register summary, Process, context and thread ID registers on page 
> > > B3-1474. ```
> > > 
> > > The Arm 1156-s (the only v6t2 processor) TRM 
> > > https://developer.arm.com/documentation/ddi0338/g/system-control-coprocessor/system-control-processor-registers/c13--process-id-register?lang=en
> > >  which shows only one process ID register under opcode 1 accessed via:
> > > ```
> > > MRC p15, 0, , c13, c0, 1 ;Read Process ID Register
> > > ```
> > > Whereas the ThreadID register is opcode 3 on CPUs that are v6k and v7.
> > The primary reason for tightening these checks was to avoid an assert in 
> > the backend when using -mtp=cp15 with a Thumb1 target, so I'd say this is 
> > more about whether the ISA has the opcode to begin with, rather than 
> > whether CPU x implements it or not.
> > Arm state (everything)
> 
> 
> Does that mean that `-march=armv5 -marm` has access/support for "CP15 C13 
> ThreadID" access?
The co-processor instructions are the same, but the effect of the instructions 
will depend on the CPU. For example on armv5 we can write the instruction to 
read/write to CP15 C13 with the ThreadID opcode. However on no armv5 
implementation will the CP15 C13 have a Thread ID register. 

The more complex stricter check will make sure that the implementations have 
the ThreadID register. As Ard mentions, the GCC intent seems to be whether the 
instruction is encodable rather than check what the CPU supports. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114116

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

[PATCH] D114116: [clang][ARM] relax -mtp=cp15 for non-thumb cases

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

I've made a suggestion to disallow v8-m.baseline (does not have Thumb 2 but has 
number > 7) and to simplify the expression.




Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:155
+  llvm::ARM::ArchKind AK = llvm::ARM::parseArch(Triple.getArchName());
+  return Ver >= 7 || AK == llvm::ARM::ArchKind::ARMV6T2 ||
+ (Ver == 6 && Triple.isARM());

nickdesaulniers wrote:
> peter.smith wrote:
> > nickdesaulniers wrote:
> > > ardb wrote:
> > > > peter.smith wrote:
> > > > > Are we restricting based on whether the threadid register is present 
> > > > > or whether the instructions are available to access the cp15 
> > > > > registers?
> > > > > 
> > > > > If we're going by whether the CPU has the register present then it 
> > > > > will be
> > > > > * A and R profile (not M profile, even the ones that have Thumb2)
> > > > > * V6K (includes ARM1176 but not ARM1156t2-s which has Thumb-2!) and 
> > > > > V7+ (A and R profile)
> > > > > 
> > > > > If we're going by the instructions to write to CP15 then it is:
> > > > > * Arm state (everything)
> > > > > * Thumb2 (v7 + v6t2)
> > > > > 
> > > > > The above seems to be a blend of the two. Is it worth choosing one 
> > > > > form or the other? GCC seems to use the latter. I guess using this 
> > > > > option falls into the I know what I'm doing area that accessing named 
> > > > > system registers comes into. If the kernel supports it the stricter 
> > > > > version may help catch more mistakes though.
> > > > > 
> > > > > The v7 A/R Arm ARM https://developer.arm.com/documentation/ddi0403/ed
> > > > > has in `D12.7.21 CP15 c13, Context ID support`
> > > > > ``` An ARMv6K implementation requires the Software Thread ID 
> > > > > registers described in VMSA CP15 c13
> > > > > register summary, Process, context and thread ID registers on page 
> > > > > B3-1474. ```
> > > > > 
> > > > > The Arm 1156-s (the only v6t2 processor) TRM 
> > > > > https://developer.arm.com/documentation/ddi0338/g/system-control-coprocessor/system-control-processor-registers/c13--process-id-register?lang=en
> > > > >  which shows only one process ID register under opcode 1 accessed via:
> > > > > ```
> > > > > MRC p15, 0, , c13, c0, 1 ;Read Process ID Register
> > > > > ```
> > > > > Whereas the ThreadID register is opcode 3 on CPUs that are v6k and v7.
> > > > The primary reason for tightening these checks was to avoid an assert 
> > > > in the backend when using -mtp=cp15 with a Thumb1 target, so I'd say 
> > > > this is more about whether the ISA has the opcode to begin with, rather 
> > > > than whether CPU x implements it or not.
> > > > Arm state (everything)
> > > 
> > > 
> > > Does that mean that `-march=armv5 -marm` has access/support for "CP15 C13 
> > > ThreadID" access?
> > The co-processor instructions are the same, but the effect of the 
> > instructions will depend on the CPU. For example on armv5 we can write the 
> > instruction to read/write to CP15 C13 with the ThreadID opcode. However on 
> > no armv5 implementation will the CP15 C13 have a Thread ID register. 
> > 
> > The more complex stricter check will make sure that the implementations 
> > have the ThreadID register. As Ard mentions, the GCC intent seems to be 
> > whether the instruction is encodable rather than check what the CPU 
> > supports. 
> I think this thread is resolved, so marking as done. Please reopen it though 
> if I misunderstood.
I think the discussion about whether the MRC/MCR instructions are available is 
closed. I think the expression isn't quite right as > 7 will permit 
v8-m.baseline, which is the evolution of cortex-m0.

To match GCC I think you want the equivalent of ARM || hasThumb2Instructions() 

I think you can do that with the equivalent of ArmTargetInfo::supportsThumb2() 
from lib/Basic/Targets/ARM.cpp
```
bool ARMTargetInfo::supportsThumb2() const {
  return CPUAttr.equals("6T2") ||
 (ArchVersion >= 7 && !CPUAttr.equals("8M_BASE"));
}
```
Where CPUAttr.equals("8M_BASE") is equivalent to 
`llvm::ARM::ArchKind::ARMV8MBaseline` so I think you can use
```
return Triple.isARM() || (Ver >= 7 && AK != 
llvm::ARM::ArchKind::ARMV8MBaseline);
```

ARMV6K and ARMV6KZ can only MRC and MCR in ARM mode so are covered by 
Triple.isARM().




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114116

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

[PATCH] D114116: [clang][ARM] relax -mtp=cp15 for non-thumb cases

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Apologies, missed a couple of small things out. Otherwise looks good to me.




Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:150
 
+// The backend does not have support for hard thread pointers when targeting
+// Thumb1.

peter.smith wrote:
> Will be worth saying `CP15 C13 ThreadID` somewhere to make it easier to look 
> up in the documentation.
Now we're permitting anything with the co-processor instructions I think the 
comment would be better expressed as:
``` We follow GCC and support when the backend has support for the MRC/MCR 
instructions that are used to set the hard thread pointer ("CP15 C13 //Thread 
id").```



Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:155
+  llvm::ARM::ArchKind AK = llvm::ARM::parseArch(Triple.getArchName());
+  return Ver >= 7 || AK == llvm::ARM::ArchKind::ARMV6T2 ||
+ (Ver == 6 && Triple.isARM());

peter.smith wrote:
> nickdesaulniers wrote:
> > peter.smith wrote:
> > > nickdesaulniers wrote:
> > > > ardb wrote:
> > > > > peter.smith wrote:
> > > > > > Are we restricting based on whether the threadid register is 
> > > > > > present or whether the instructions are available to access the 
> > > > > > cp15 registers?
> > > > > > 
> > > > > > If we're going by whether the CPU has the register present then it 
> > > > > > will be
> > > > > > * A and R profile (not M profile, even the ones that have Thumb2)
> > > > > > * V6K (includes ARM1176 but not ARM1156t2-s which has Thumb-2!) and 
> > > > > > V7+ (A and R profile)
> > > > > > 
> > > > > > If we're going by the instructions to write to CP15 then it is:
> > > > > > * Arm state (everything)
> > > > > > * Thumb2 (v7 + v6t2)
> > > > > > 
> > > > > > The above seems to be a blend of the two. Is it worth choosing one 
> > > > > > form or the other? GCC seems to use the latter. I guess using this 
> > > > > > option falls into the I know what I'm doing area that accessing 
> > > > > > named system registers comes into. If the kernel supports it the 
> > > > > > stricter version may help catch more mistakes though.
> > > > > > 
> > > > > > The v7 A/R Arm ARM 
> > > > > > https://developer.arm.com/documentation/ddi0403/ed
> > > > > > has in `D12.7.21 CP15 c13, Context ID support`
> > > > > > ``` An ARMv6K implementation requires the Software Thread ID 
> > > > > > registers described in VMSA CP15 c13
> > > > > > register summary, Process, context and thread ID registers on page 
> > > > > > B3-1474. ```
> > > > > > 
> > > > > > The Arm 1156-s (the only v6t2 processor) TRM 
> > > > > > https://developer.arm.com/documentation/ddi0338/g/system-control-coprocessor/system-control-processor-registers/c13--process-id-register?lang=en
> > > > > >  which shows only one process ID register under opcode 1 accessed 
> > > > > > via:
> > > > > > ```
> > > > > > MRC p15, 0, , c13, c0, 1 ;Read Process ID Register
> > > > > > ```
> > > > > > Whereas the ThreadID register is opcode 3 on CPUs that are v6k and 
> > > > > > v7.
> > > > > The primary reason for tightening these checks was to avoid an assert 
> > > > > in the backend when using -mtp=cp15 with a Thumb1 target, so I'd say 
> > > > > this is more about whether the ISA has the opcode to begin with, 
> > > > > rather than whether CPU x implements it or not.
> > > > > Arm state (everything)
> > > > 
> > > > 
> > > > Does that mean that `-march=armv5 -marm` has access/support for "CP15 
> > > > C13 ThreadID" access?
> > > The co-processor instructions are the same, but the effect of the 
> > > instructions will depend on the CPU. For example on armv5 we can write 
> > > the instruction to read/write to CP15 C13 with the ThreadID opcode. 
> > > However on no armv5 implementation will the CP15 C13 have a Thread ID 
> > > register. 
> > > 
> > > The more complex stricter check will make sure that the implementations 
> > > have the ThreadID register. As Ard mentions, the GCC intent seems to be 
> > > whether the instruction is encodable rather than check what the CPU 
> > > supports. 
> > I think this thread is resolved, so marking as done. Please reopen it 
> > though if I misunderstood.
> I think the discussion about whether the MRC/MCR instructions are available 
> is closed. I think the expression isn't quite right as > 7 will permit 
> v8-m.baseline, which is the evolution of cortex-m0.
> 
> To match GCC I think you want the equivalent of ARM || 
> hasThumb2Instructions() 
> 
> I think you can do that with the equivalent of 
> ArmTargetInfo::supportsThumb2() from lib/Basic/Targets/ARM.cpp
> ```
> bool ARMTargetInfo::supportsThumb2() const {
>   return CPUAttr.equals("6T2") ||
>  (ArchVersion >= 7 && !CPUAttr.equals("8M_BASE"));
> }
> ```
> Where CPUAttr.equals("8M_BASE") is equivalent to 
> `llvm::ARM::ArchKind::ARMV8MBaseline` so I think you can use
> ```
> return Triple.isARM() || (Ver >= 7 && AK != 
> llvm::ARM::ArchKind::ARMV8MBaseline);
> ```
> 
> ARMV6K and AR

[PATCH] D114116: [clang][ARM] relax -mtp=cp15 for non-thumb cases

[Peter Smith via Phabricator via cfe-commits]

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

LGTM, thanks for the update.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114116

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

[PATCH] D114124: [clang][ARM] only check -mtp=cp15 for non-asm sources

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

LGTM too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114124

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

[PATCH] D136385: Add support for MC/DC in LLVM Source-Based Code Coverage

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

I've got about as far as the clang changes. As I mentioned in Discourse I'm not 
familiar enough in this area to give good feedback on the implementation, most 
if not all my comments fall under the bike shedding category. Will need to 
leave the high-level feedback to people that are more experienced in this area.

Thanks very much for uploading the design doc pdf, that was very useful. 
Although likely not for this patch as it is large enough already, will be worth 
updating https://llvm.org/docs/CoverageMappingFormat.html and 
https://clang.llvm.org/docs/SourceBasedCodeCoverage.html

I suspect that this patch will need to get split up into smaller parts. While 
it is very useful to see everything at once, it may be better to use this as a 
kind of support for the RFC, and then split off the implementation into smaller 
independently reviewable parts, even if the full functionality isn't useable 
until the last patch lands. For example the LLVM changes could be looked at and 
accepted prior to clang.

Will try and look and some more parts next week, my apologies I can't go 
through it all at once.




Comment at: clang/lib/CodeGen/CodeGenFunction.h:1522
+  /// Used by MC/DC to track the execution state of a boolean expression.
+  Address MCDCTempAddr = Address::invalid();
+

Would MCDCTrackingStateAddr be more appropriate? It could save people some time 
tracking back up to the comment to work out what Temp is being used for.



Comment at: clang/lib/CodeGen/CodeGenFunction.h:1542
 
+  bool isMCDCCoverageEnabled() {
+return (CGM.getCodeGenOpts().hasProfileClangInstr() &&

Can this be `bool isMCDCCoverageEnabled() const`



Comment at: clang/lib/CodeGen/CodeGenFunction.h:1554
+  MCDCTempAddr = CreateIRTemp(getContext().UnsignedIntTy, "mcdc.addr");
+  return;
+}

Is the return necessary here?



Comment at: clang/lib/CodeGen/CodeGenFunction.h:1558
+
+  bool isBinaryLogicalOp(const Expr *S) {
+const BinaryOperator *BOp = dyn_cast(S->IgnoreParens());

This looks like it doesn't need to be a member function? I can appreciate that 
here may be the most convenient place to  put it though.



Comment at: clang/lib/CodeGen/CodeGenPGO.cpp:164
+  /// The next bitmask byte index to assign.
+  unsigned NextBitmask;
+  /// The map of statements to bitmask coverage objects.

perhaps worth including index in the name, for example `NextBitmaskIdx` . Will 
help prevent confusion as to whether this is an index or an actual bitmask 
(comment does say, but harder to see from reading the code).



Comment at: clang/lib/CodeGen/CodeGenPGO.cpp:979
+  // set it to zero.
+  unsigned MCDCMaxConditions = 0;
+  if (CGM.getCodeGenOpts().MCDCCoverage)

`unsigned MCDCMaxConditions = (CGM.getCodeGenOpts().MCDCCoverage) ? 
CGM.getCodeGenOpts().MCDCMaxConditionCount : 0;`

Purely subjective opinion on my part though.



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:102
 
+  unsigned Mask = 0;
+  unsigned Conditions = 0;

Worth a comment that these are associated with MCDC?



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:662
+  /// Is this a logical-AND operation?
+  bool isLAnd(const BinaryOperator *E) { return E->getOpcode() == BO_LAnd; }
+

Could this be a free function? If not then could it be `const`? 



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:718
+  void pushAndAssignIDs(const BinaryOperator *E) {
+if (CGM.getCodeGenOpts().MCDCCoverage) {
+  // If binary expression is disqualified, don't do mapping.

Would it be better to invert the condition and early exit? This would save a 
level of indentation for the rest of the function.



Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:759
+unsigned TotalConds = 0;
+if (CGM.getCodeGenOpts().MCDCCoverage) {
+  // Pop Stmt from 'NestLevel' stack.

Would it be better to invert the condition and early exit? This would save a 
level of indentation for the rest of the function.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:924
+   false)) {
+if (!CoverageMapping)
+  D.Diag(clang::diag::err_drv_argument_only_allowed_with)

Maybe able to simplify this to `if 
(Args.hasFlag(options::OPT_fcoverage_mapping)` and remove `CoverageMapping`. I 
think that in the case of `!ProfileGenerateArg` there are several options so it 
needed to be a variable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136385

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

[PATCH] D142893: [NFC] Class for building MultilibSet

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Looks good so far. Some stylistic suggestions.




Comment at: clang/include/clang/Driver/Multilib.h:25
 namespace driver {
 
 /// This corresponds to a single GCC Multilib, or a segment of one controlled

It took a bit of reading to work out what the relationship between Multilib, 
MultilibVariant, MultilibSet and MultilibVariantBuilder are and how they are 
expected to be used.

IIUC MultilibVariant and MultilibVariantBuilder are used to construct Multilib 
and MultilibSet, which are expected to be immutable post construction.

Will be worth either a comment describing the relation ship or potentially 
write up the design in the clang docs and reference it from here. 



Comment at: clang/include/clang/Driver/Multilib.h:40
 public:
   Multilib(StringRef GCCSuffix = {}, StringRef OSSuffix = {},
+   StringRef IncludeSuffix = {}, int Priority = 0,

Could be worth documenting the requirement for the strings to start with a `/` 
or be empty as the constructor implementation in a different file will assert 
if they aren't?



Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:32
 
-static Multilib makeMultilib(StringRef commonSuffix) {
-  return Multilib(commonSuffix, commonSuffix, commonSuffix);
+static MultilibBuilderVariant makeMultilib(StringRef commonSuffix) {
+  return MultilibBuilderVariant(commonSuffix, commonSuffix, commonSuffix);

Worth making this a lambda in findRISCVMultilibs? Purely subjective opinion 
here as there could be an attempt to limit changes.



Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:44
   if (TargetTriple.isRISCV64()) {
-Multilib Imac = 
makeMultilib("").flag("+march=rv64imac").flag("+mabi=lp64");
-Multilib Imafdc = makeMultilib("/rv64imafdc/lp64d")
-  .flag("+march=rv64imafdc")
-  .flag("+mabi=lp64d");
+auto Imac = makeMultilib("").flag("+march=rv64imac").flag("+mabi=lp64");
+auto Imafdc = makeMultilib("/rv64imafdc/lp64d")

Wondering if we've lost information here by going to auto. We're really making 
a MultilibVariant here. Perhaps worth renaming makeMultilib to 
makeMultilibVariant?



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:1049
 
-static Multilib makeMultilib(StringRef commonSuffix) {
-  return Multilib(commonSuffix, commonSuffix, commonSuffix);
+static MultilibBuilderVariant makeMultilib(StringRef commonSuffix) {
+  return MultilibBuilderVariant(commonSuffix, commonSuffix, commonSuffix);

Similar comment to BareMetal, is it worth renaming to makeMultilibVariant



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:1737
   });
 
   Multilib::flags_list Flags;

Although I'm not too bothered myself, some people prefer to keep changes in 
whitespace to a separate NFC patch for the benefit of git annotate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142893

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

[PATCH] D142905: Change multilib selection algorithm

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

A couple of small stylisitic suggestions.




Comment at: clang/include/clang/Driver/Multilib.h:31
 public:
-  using flags_list = std::vector;
+  using flags_list = std::set;
+  using arg_list = std::vector;

would flags_set be a better name. I'm guessing we're using set as we want 
uniqueness and ordering? 

If we don't need ordering then we may be able to use 
https://llvm.org/docs/ProgrammersManual.html#llvm-adt-stringset-h



Comment at: clang/include/clang/Driver/Multilib.h:106
+  /// Select compatible variants
+  std::vector select(const Multilib::flags_list &Flags) const;
+

Worth using multilib_list for consistency with the rest of the file?

Is this meant to be an overload for bool select below? I mention it as it has a 
different return type. Perhaps use selectCompatible? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142905

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

[PATCH] D142933: Add -print-multi-selection-flags argument

[Peter Smith via Phabricator via cfe-commits]

peter.smith added inline comments.



Comment at: clang/include/clang/Driver/Options.td:4162
 def print_multi_lib : Flag<["-", "--"], "print-multi-lib">;
+def print_multi_selection_flags : Flag<["-", "--"], 
"print-multi-selection-flags-experimental">,
+  HelpText<"Print the flags used for selecting multilibs (experimental)">;

any particular reason for using multi rather than multilib? Is there any intent 
to use this anywhere else?



Comment at: clang/include/clang/Driver/ToolChain.h:289
+  /// Get flags suitable for multilib selection, based on the provided clang
+  /// command line arguments. The arguments aren't suitable to be used directly
+  /// for multilib selection because they are not normalized and normalization

Could be worth using "The command line arguments ..." when I first read I 
didn't pick up on the distinction between flags and arguments. IIUC this 
function translates command line arguments into normalised multilib selection 
flags.



Comment at: clang/include/clang/Driver/ToolChain.h:303
+  ///   returned separately. There may not be valid syntax to express this as a
+  ///   clang argument so an pseudo argument syntax must be used instead.
+  /// To allow users to find out what flags are returned, clang accepts a

typo "a pseudo"



Comment at: clang/lib/Driver/ToolChain.cpp:173
 }
 
+std::vector

IIUC this needs to contain the union of all command-line options used by all 
clients of the YAML but not any of the existing hard-coded Multilibs?

There is a possibility that this function could get large over time, with many 
Toolchain and Architecture specific flags? It is it worth delegating some of 
the work to the Toolchain, for example at the moment the BareMetal driver won't 
support the sanitizers so this could be processed by a Toolchain specific 
function (Fuchsia in this case?). There's likely to be a lot of options used by 
many Multilib implementations so there is still scope for putting stuff here. 
We would need to know what driver and architecture we are using here though.

If we had already constructed the MultilibSet it could be possible to filter 
the flags against what the MultilibSet actually accepts?

Is it worth making that clear, or have some way for the other non YAML users to 
call print_multi_selection_flags? Or is the option purely for 
developing/debugging multilib as I'd not expect the output of 
print_multi_section flags to be used by a user of clang. In that case a 
translation of what command-line flags influence multilib selection (ideally 
filtered) through the processed YAML file would be great.



Comment at: clang/lib/Driver/ToolChain.cpp:220
+
+  switch (Triple.getArch()) {
+  case llvm::Triple::arm:

Even though it might still live in this file, might be worth having all options 
for a particular arch in a separate function. For example:
```
case llvm::Triple::arm:
case llvm::Triple::armeb:
case llvm::Triple::thumb:
case llvm::Triple::thumbeb:
  addArmArchFlags(); // not put any thought into parameters etc. 
```



Comment at: clang/lib/Driver/ToolChain.cpp:225
+  case llvm::Triple::thumbeb: {
+std::vector Features;
+unsigned FPUKind = llvm::ARM::FK_INVALID;

Can we handle the A profile situation where effectively we have v8 + a list of 
features, with each v8.x and v9.y enabling a set of features 
https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html 

Ideally we would want to be able to do things like map v8.5-a+sve+sve2 and v9 
to the same libraries. 



Comment at: clang/lib/Driver/ToolChain.cpp:244
+#include "llvm/TargetParser/ARMTargetParser.def"
+default:
+  llvm_unreachable("Invalid FPUKind");

I'm wondering if instead of using the FPU name we use a string representation 
of the properties for the flags. For example FK_VFPV3_D16 this would be akin to 
expanding march=armv8.6 to a list of target features. We'd need to make a 
stable string representation but I think that should be possible.



Comment at: clang/test/Driver/print-multi-selection-flags.c:40
+// CHECK-MVE: target=thumbv8.1m.main-none-unknown-eabihf
+
+// RUN: %clang -print-multi-selection-flags-experimental 
--target=arm-none-eabi -march=armv8.1m.main+mve+nofp | FileCheck 
--check-prefix=CHECK-MVENOFP %s

Would be useful to have some Armv7-A and Armv8-A tests as well. In particular 
I'm thinking of Arm-v8 and Arm-v8.1 as LSE atomics appear in v8.1. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142933

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

[PATCH] D155808: [clang][driver] Missing the condition in IsARMBigEndain function.

[Peter Smith via Phabricator via cfe-commits]

peter.smith added inline comments.



Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:39
 // normalized triple so we must handle the flag here.
 bool arm::isARMBigEndian(const llvm::Triple &Triple, const ArgList &Args) {
+  if ((Triple.getArch() == llvm::Triple::arm ||

Is this the right place to fix?

I would expect it to be a precondition that the Triple was Arm or Thumb before 
calling isARMBigEndian?

For example
```
if (Triple.isARM() || Triple.isThumb() || Triple.isAArch64()) {
bool IsBigEndian = arm::isARMBigEndian(Triple, Args);
if (IsBigEndian)
  arm::appendBE8LinkFlag(Args, CmdArgs, Triple);
IsBigEndian = IsBigEndian || Arch == llvm::Triple::aarch64_be;
CmdArgs.push_back(IsBigEndian ? "-EB" : "-EL");
  }
```
Shouldn't this be refactored to only call isARMBigEndian for isARM and isThumb? 
Something like:
```
if ((Triple.isARM() || Triple.isThumb()) {
  bool BigEndian = arm::isARMBigEndian(Triple, Args);
  if (BigEndian)
arm::appendBE8LinkFlag(Args, CmdArgs, Triple);
  CmdArgs.push_back(IsBigEndian ? "-EB" : "-EL");
} else if (Triple.isAArch64)) {
  CmdArgs.push_back(Arch == llvm::Triple::aarch64_be ? "-EB" : "-EL");
}
```
This is a bit longer but it is easier to read and keeps ARM and AArch64 
separate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155808

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

[PATCH] D73865: [CodeGenModule] Assume dso_local for -fpic -fno-semantic-interposition

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

If I've understood correctly this would make LLVM more aggressive for PIC 
relocation models, but perhaps more honest in an inter procedural optimisation 
context? The code changes look fine to me, I'm wondering if we've discussed 
this widely enough with the community to work out how to proceed here. For 
example do we have plan to test -fno-semantic-interposition well enough for it 
to be relied on. The consensus may already exist, and I don't know enough about 
it though.




Comment at: clang/lib/CodeGen/CodeGenModule.cpp:845
 
   // If this is not an executable, don't assume anything is local.
   const auto &CGOpts = CGM.getCodeGenOpts();

I think that this comment needs updating to explain the effect of 
SemanticInterposition, and maybe the clang default.



Comment at: clang/test/CodeGen/aapcs-align.cpp:21
 }
-// CHECK: define void @g0
+// CHECK: define dso_local void @g0
 // CHECK: call void @f0(i32 1, [2 x i32] [i32 6, i32 7]

I initially thought a triple of arm-none-none-eabi (bare-metal for embedded 
systems) would have a relocation model of static and hence should have already 
been dso_local. Thinking about it in more detail the clang-driver will usually 
pass the relocation model to cc1 so by default the driver is assuming PIC. May 
be worth pointing that out in your description.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73865



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

[PATCH] D73904: [clang] stop baremetal driver to append .a to lib

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

I think this is the right thing to do. According to 
https://sourceware.org/binutils/docs/ld/Options.html#Options

  Add the archive or object file specified by namespec to the list of files to 
link. This option may be used any number of times. If namespec is of the form 
:filename, ld will search the library path for a file called filename, 
otherwise it will search the library path for a file called libnamespec.a.

An argument could be made that LLD could be a bit cleverer and only add the .a 
when it doesn't exist, although that would fail in the contrived case that 
someone had explicitly named their library lib name.a.a however it would be 
good to fix this in clang so that older versions of LLD will work.

Please could you add a test to clang/test/Driver/arm-compiler-rt.c for the 
arm-none-eabi target.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73904



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

[PATCH] D73904: [clang] stop baremetal driver to append .a to lib

[Peter Smith via Phabricator via cfe-commits]

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

Thanks for the update, looks good to me. The BareMetal driver tests are better 
than the location I suggested.


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

https://reviews.llvm.org/D73904



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

[PATCH] D72897: List implicit operator== after implicit destructors in a vtable.

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

It is also failing on the other 32-bit arm bots, example 
http://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/13052/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Avirtual-compare.cpp
Looking at the failure

  // CHECK-SAME: @_ZThn8_N1AD1Ev 

and the output on an Arm machine

  $_ZThn4_N1AD1Ev = comdat any
  
  $_ZThn4_N1AD0Ev = comdat any
  
  $_ZThn4_N1AaSERKS_ = comdat any

It might be a 32-bit/64-bit specific expectation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72897



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

[PATCH] D72959: Relative VTables ABI on Fuchsia

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

In D72959#1832011 , @pcc wrote:

> > On Aarch64, right now only CALL26 and JUMP26 instructions generate PLT 
> > relocations, so we manifest them with stubs that are just jumps to the 
> > original function.
>
> I think it would be worth considering defining a new relocation type for 
> this. I think we should make the new reloc type represent the relative 
> address shifted right by 2, which would allow it to be used freely in the 
> aarch64 small code model (which permits programs up to 4GB), but would 
> require the target to be 4-byte aligned, which seems like a reasonable 
> restriction. On aarch64, decoding this would not require any additional 
> instructions either (we can fold the lsl into the add). We could use the same 
> technique for a new GOTPCREL relocation to be used for the RTTI component. 
> @peter.smith what do you think?


The idea of a new relocation type has come up before, as I recall it was 
something like the equivalent of R_X86_PLT32

  | R_X86_64_PLT32 | 4 | word32 | L + A - P |
  Where L is defined as: L Represents the place (section offset or address) of 
the Procedure Linkage Table entry for a symbol.

For Fuchsia there are two options:
1.) Ask for an ABI relocation type to be defined. I've raised an ABI issue with 
Arm, for it to progress I think it needs a "yes we really would like one, here 
is the definition we want to use, this is the use case and it could be used 
outside of Fuchsia at some point." For example I can see position independent 
C++ being useful in bare-metal embedded C++. The external facing email for 
feedback on the abi is arm.e...@arm.com (address is on the first page of the 
doc below)
2.) There are two ranges of relocation types reserved for private and platform 
relocations: 
https://developer.arm.com/docs/ihi0056/f/elf-for-the-arm-64-bit-architecture-aarch64-abi-2019q2-documentation

  Private and platform-specific relocations
  Private relocations for vendor experiments:
  
  0xE000 to 0xEFFF for ELF64
  0xE0 to 0xEF for ELF32
  Platform ABI defined relocations:
  
  0xF000 to 0x for ELF64
  0xF0 to 0xFF for ELF32
  Platform ABI relocations can only be interpreted when the EI_OSABI field is 
set to indicate the Platform ABI governing the definition.
  
  All of the above codes will not be assigned by any future version of this 
standard.

If this could be generally useful outside of Fuchsia, then an official 
relocation would be preferable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72959



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

[PATCH] D72959: Relative VTables ABI on Fuchsia

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

In D72959#1835368 , @rjmccall wrote:

> There's two independently-useful things here for the relocation, right?  
> First, it's useful to be able to express a relocation to a symbol that has 
> the semantics of a particular function but doesn't necessarily have its 
> address, and that concept could be used across many "physical" relocations; 
> and second, it's potentially useful to have a shifted-by-two relative address 
> relocation, at least on AArch64 where instructions (and v-table entries under 
> this ABI) are always four-byte-aligned anyway.  Is it possible to separate 
> these concerns in ELF, e.g. by having a "symbol" that can be the target of 
> any other relocation but which actually represents a function of unspecified 
> address with the semantics of another function?


It would be possible to design relocations like that, but I think it would be 
difficult to fit into existing multi-target designs, which assume that the 
relocation code alone encodes all the actions a linker needs to take 
(smart-format, dumb-linker). My understanding of the reasons behind this are:

- The linker can have millions of relocations to resolve and  having all the 
actions explicit in the code simplifies its job.
- There is a large space of available relocation codes, partitioned per target, 
but a much more limited availability of target specific symbol types and flags.
- The concerns can be separated at implementation time, for example the symbol 
lookup, encoding and calculation stages are independent and shared between the 
codes.

It is possible that there is a better trade-off in complexity, but anything 
radically different might need quite a bit of work to fit into an existing 
linker. Hope I've understood you correctly and apologies if the above isn't 
relevant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72959



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

[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,0]

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Although this particular commit will not be at fault, it is the option that 
enables the feature which is the earliest I can bisect the fault to. There are 
3 files in linux that assert fail on the Implement the 'patchable-function 
attribute'. The files in question are kasan/quarantine.c, mm/slab_common.c and 
mm/slub.c .

I reproduced with

  make CC=/path/to/clang/install/clang ARCH=arm64 
CROSS_COMPILE=aarch64-linux-gnu- HOSTGCC=gcc allmodconfig

You can get the log files for the build, which is from clang-10
https://git.linaro.org/toolchain/ci/base-artifacts.git/log/?h=linaro-local/ci/tcwg_kernel/llvm-release-aarch64-mainline-allmodconfig
 (4: update: llvm-linux: 19676)

If the patchable functions is intended for clang-10 we'll need to make sure any 
fix is merged to clang-10.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D7



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

[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,0]

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

>> If the patchable functions is intended for clang-10 we'll need to make sure 
>> any fix is merged to clang-10.
> 
> This commit was made before release/10.x branch. Maybe the easiest thing is 
> to revert the driver change in release/10.x (CC @hans), before we had a 
> better understanding of the problem.
>  (Eventually I think the Linux kernel should have a better configure time 
> test than a simple `whether the compiler accepts 
> -fpatchable-function-entry=2,0?`)
> 
> @peter.smith @nickdesaulniers What do you think?

Revert on the 10.0 release sounds reasonable to me. That would prevent the 
kernel from enabling the option and would prevent the build failure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D7



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

[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,0]

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

>> @peter.smith @nickdesaulniers What do you think?
> 
> Revert on the 10.0 release sounds reasonable to me. That would prevent the 
> kernel from enabling the option and would prevent the build failure.

I should have been clearer, apologies; we're not blocked by the assertion 
failures, the CI won't halt and should pick up other build failures.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D7



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

[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,0]

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

In D7#1836703 , @MaskRay wrote:

> In D7#1836669 , @hans wrote:
>
> > In D7#1836643 , @peter.smith 
> > wrote:
> >
> > > >> If the patchable functions is intended for clang-10 we'll need to make 
> > > >> sure any fix is merged to clang-10.
> > > > 
> > > > This commit was made before release/10.x branch. Maybe the easiest 
> > > > thing is to revert the driver change in release/10.x (CC @hans), before 
> > > > we had a better understanding of the problem.
> > > >  (Eventually I think the Linux kernel should have a better configure 
> > > > time test than a simple `whether the compiler accepts 
> > > > -fpatchable-function-entry=2,0?`)
> > > > 
> > > > @peter.smith @nickdesaulniers What do you think?
> > >
> > > Revert on the 10.0 release sounds reasonable to me. That would prevent 
> > > the kernel from enabling the option and would prevent the build failure.
> >
> >
> > But if trunk is broken, shouldn't it be reverted there first, and then we 
> > can merge the revert to 10.x (and then trunk can be fixed eventually)?
>
>
> I don't think the commit is to be blamed. The availability of the driver 
> option -fpatchable-function-entry= enables `CONFIG_DYNAMIC_FTRACE_WITH_REGS` 
> and `CONFIG_LIVEPATCH`, which were not tested before. There could be other 
> issues in the code path.
>
>   % rg fpatchable-function-entry
>   arch/arm64/Kconfig
>   147:if $(cc-option,-fpatchable-function-entry=2)
>  
>   arch/arm64/Makefile
>   100:  CC_FLAGS_FTRACE := -fpatchable-function-entry=2
>
>
> If we can explicitly disable the options in the CI, that will be very nice.


It definitely won't be this commit, the assert fail looks like it is from the 
patchable-function attribute pass. The CI just runs allyesconfig and 
allmodconfig and tells us if there are any new regressions, it won't report 
this one again so it is unlikely to be worth masking it out.

Assert failure:

  clang-10: /work/llvm-project/llvm/include/llvm/ADT/ilist_iterator.h:139: 
llvm::ilist_iterator::reference 
llvm::ilist_iterator, false, false>::operator*() const [OptionsT = 
llvm::ilist_detail::node_options, 
IsReverse = false, IsConst = false]: Assertion `!NodePtr->isKnownSentinel()' 
failed.
  Stack dump:
  0.Program arguments: /work/llvm-project/build/buildclang/bin/clang-10 
-cc1 -triple aarch64-unknown-linux-gnu -S -disable-free -main-file-name 
slab_common.c -mrelocation-model static -mthread-model posix 
-fno-delete-null-pointer-checks -mllvm -warn-stack-size=2048 
-mframe-pointer=non-leaf -relaxed-aliasing -mdisable-tail-calls -fmath-errno 
-fno-rounding-math -masm-verbose -no-integrated-as -mconstructor-aliases 
-target-cpu generic -target-feature -fp-armv8 -target-feature -crypto 
-target-feature -neon -target-feature -sha2 -target-feature -aes -target-abi 
aapcs -mllvm -aarch64-enable-global-merge=false 
-fallow-half-arguments-and-returns -dwarf-column-info -fno-split-dwarf-inlining 
-debugger-tuning=gdb -nostdsysteminc -nobuiltininc -resource-dir 
/work/llvm-project/build/buildclang/lib/clang/10.0.0 -dependency-file 
mm/.slab_common.o.d -MT mm/slab_common.o -sys-header-deps -isystem 
/work/clangmaster/lib/clang/10.0.0/include -include ./include/linux/kconfig.h 
-include ./include/linux/compiler_types.h -I ./arch/arm64/include -I 
./arch/arm64/include/generated -I ./include -I ./arch/arm64/include/uapi -I 
./arch/arm64/include/generated/uapi -I ./include/uapi -I 
./include/generated/uapi -D __KERNEL__ -D CC_USING_PATCHABLE_FUNCTION_ENTRY -D 
KASAN_SHADOW_SCALE_SHIFT=3 -D CONFIG_AS_LSE=1 -D CONFIG_CC_HAS_K_CONSTRAINT=1 
-D KASAN_SHADOW_SCALE_SHIFT=3 -D KBUILD_BASENAME="slab_common" -D 
KBUILD_MODNAME="slab_common" -fmacro-prefix-map=./= -O2 -Wall -Wundef 
-Werror=strict-prototypes -Wno-trigraphs -Werror=implicit-function-declaration 
-Werror=implicit-int -Wno-format-security -Werror=unknown-warning-option 
-Wno-address-of-packed-member -Wno-format-invalid-specifier -Wno-gnu 
-Wno-tautological-compare -Wno-unused-const-variable 
-Wdeclaration-after-statement -Wvla -Wno-pointer-sign -Werror=date-time 
-Werror=incompatible-pointer-types -Wno-initializer-overrides -Wno-format 
-Wno-sign-compare -Wno-format-zero-length -std=gnu89 -fno-dwarf-directory-asm 
-fdebug-compilation-dir /work/linux/linux -ferror-limit 19 -fmessage-length 0 
-fpatchable-function-entry=2 -fwrapv -stack-protector 2 
-ftrivial-auto-var-init=pattern -fno-builtin -fno-signed-char 
-fwchar-type=short -fno-signed-wchar -fgnuc-version=4.2.1 -fobjc-runtime=gcc 
-fno-common -fdiagnostics-show-option -fcolor-diagnostics -vectorize-loops 
-vectorize-slp -o /tmp/slab_common-7e60cc.s -x c mm/slab_common.c 
  1. parser at end of file
  2.Code generation
  3.Running pass 'Function Pass Manager' on module 'mm/slab_common.c'.
  4.Running pass 'Implement the 'patchable-function' attribute' on funct

[PATCH] D44815: [AArch64]: Add support for parsing rN registers.

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

I think that we may have a bit of a conceptual difference with GCC here. As far 
as I can tell in GCC it doesn't matter if "rn", "wn", "xn" or even "bn" is 
used, this refers to register 0. The Operand constraint such as %w0 is used to 
control the substitution so the r registers aren't strictly aliases like the 
other registers. The comment

> // The S/D/Q and W/X registers overlap, but aren't really aliases; we 
>  //don't want to substitute one of these for a different-sized one.

suggests that this may have been the intent that Clang behave differently to 
GCC in this respect. For example in Clang there appears to be a bit more 
significance placed on the "wn" or "xn", for example clang will warn in the 
following example, whereas gcc will not:

  long f2(int i) {
  register int x asm("w6");
  register int y asm("w7");
  register int z asm("w8");
  asm("add %0, %1, %2\n": "=r"(x) : "r"(y), "r"(z));
  return y;
  }
  
  t2.c:5:34: warning: value size does not match register size specified by the
constraint and modifier [-Wasm-operand-widths]
  asm("add %0, %1, %2\n": "=r"(x) : "r"(y), "r"(z));
   ^~
   %w0
  // and so on for %1 and %2.   

Having said all that, clang will not warn when the operand modifier is 
explicit, for example:

  long f2(int i) {
  register long x asm("w6");
  register long y asm("w7");
  register long z asm("w8");
  asm("add %w0, %w1, %w2\n": "=r"(x) : "r"(y), "r"(z));
  return y;
  }

At the moment I'm not too sure what to make of this. Strictly speaking I think 
"rn" is an unsized register that Clang shouldn't warn about size mismatch in 
the operand modifier. With the implementation above it won't because we default 
to xn and clang doesn't seem to warn when there is an explicit modifier for wn, 
although this could change in the future.

I'd be very interested to hear other opinions on this? If register rn is to be 
supported perhaps a test that clang doesn't warn when asm("rn") is used with 
the operand modifier %wn is used.


Repository:
  rC Clang

https://reviews.llvm.org/D44815



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

[PATCH] D44815: [AArch64]: Add support for parsing rN registers.

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

Thanks for the clarification. With that in mind I'm much less concerned that 
adding "r" as an alias will make extra warnings more difficult.  I agree that 
there should be at least a warning for register long x asm ("wn") although that 
is separate from this patch. Has anyone else got any objections?


Repository:
  rC Clang

https://reviews.llvm.org/D44815



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

[PATCH] D44815: [AArch64]: Add support for parsing rN registers.

[Peter Smith via Phabricator via cfe-commits]

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

Given that this is important for compiling the Linux kernel with clang I think 
that it is worth improving compatibility with GCC. LGTM.


Repository:
  rC Clang

https://reviews.llvm.org/D44815



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

[PATCH] D40127: [Driver][ARM] For assembler files recognize -Xassembler or -Wa, -mthumb

[Peter Smith via Phabricator via cfe-commits]

peter.smith created this revision.
Herald added subscribers: kristof.beyls, javed.absar, aemerson.

The Unified Arm Assembler Language is designed so that the majority of 
assembler files can be assembled for both Arm and Thumb with the choice made as 
a compilation option. The way this is done in gcc is to pass -mthumb to the 
assembler with either -Wa,-mthumb or -Xassembler -mthumb. This change adds 
support for these options to clang. There is no assembler equivalent of 
-mno-thumb, -marm or -mno-arm so we don't need to recognize these.

  

Ideally we would do all of the processing in 
CollectArgsForIntegratedAssembler(). Unfortunately we need to change the triple 
and at that point it is too late. Instead we look for the option earlier in 
ComputeLLVMTriple().

  

Fixes PR34519


https://reviews.llvm.org/D40127

Files:
  lib/Driver/ToolChain.cpp
  lib/Driver/ToolChains/Clang.cpp
  test/Driver/arm-target-as-mthumb.s


Index: test/Driver/arm-target-as-mthumb.s
===
--- /dev/null
+++ test/Driver/arm-target-as-mthumb.s
@@ -0,0 +1,17 @@
+// Make sure -mthumb does not affect assembler triple, but -Wa,-mthumb or
+// -Xassembler -mthumb does. Also check that -Wa,-mthumb or -Xassembler -mthumb
+// does not affect non assembler files.
+
+// RUN: %clang -target armv7a-linux-gnueabi -### -c -mthumb %s 2>&1 | \
+// RUN: FileCheck -check-prefix=TRIPLE-ARM %s
+// RUN: %clang -target armv7a-linux-gnueabi -### -c -Wa,-mthumb \
+// RUN: %S/Inputs/wildcard1.c  2>&1 | FileCheck -check-prefix=TRIPLE-ARM %s
+
+// TRIPLE-ARM: "-triple" "armv7--linux-gnueabi"
+
+// RUN: %clang -target armv7a-linux-gnueabi -### -c -Wa,-mthumb %s 2>&1 | \
+// RUN: FileCheck -check-prefix=TRIPLE-THUMB %s
+// RUN: %clang -target armv7a-linux-gnueabi -### -c -Xassembler -mthumb %s \
+// RUN: 2>&1 | FileCheck -check-prefix=TRIPLE-THUMB %s
+
+// TRIPLE-THUMB: "-triple" "thumbv7--linux-gnueabi"
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -1889,6 +1889,15 @@
   switch (C.getDefaultToolChain().getArch()) {
   default:
 break;
+  case llvm::Triple::thumb:
+  case llvm::Triple::thumbeb:
+  case llvm::Triple::arm:
+  case llvm::Triple::armeb:
+if (Value == "-mthumb")
+  // -mthumb has already been processed in ComputeLLVMTriple()
+  // recognize but skip over here.
+  continue;
+
   case llvm::Triple::mips:
   case llvm::Triple::mipsel:
   case llvm::Triple::mips64:
Index: lib/Driver/ToolChain.cpp
===
--- lib/Driver/ToolChain.cpp
+++ lib/Driver/ToolChain.cpp
@@ -541,11 +541,27 @@
   << tools::arm::getARMArch(MArch, getTriple()) << "ARM";
 }
 
-// Assembly files should start in ARM mode, unless arch is M-profile.
-// Windows is always thumb.
-if ((InputType != types::TY_PP_Asm && Args.hasFlag(options::OPT_mthumb,
- options::OPT_mno_thumb, ThumbDefault)) || IsMProfile ||
- getTriple().isOSWindows()) {
+// Check for -mthumb passed explicitly to the assembler via -Wa or
+// -Xassembler. Ideally we would do this in
+// CollectArgsForIntegratedAssembler but we can't change the ArchName at
+// that point. There is no assembler equivalent of -mno-thumb, -marm, or
+// -mno-arm.
+bool IsIntegratedAssemblerThumb = false;
+for (const Arg *A :
+ Args.filtered(options::OPT_Wa_COMMA, options::OPT_Xassembler)) {
+  for (StringRef Value : A->getValues()) {
+if (Value == "-mthumb")
+  IsIntegratedAssemblerThumb = true;
+  }
+}
+// Assembly files should start in ARM mode, unless arch is M-profile, or
+// -mthumb has been passed explicitly to the assembler. Windows is always
+// thumb.
+if ((InputType != types::TY_PP_Asm &&
+ Args.hasFlag(options::OPT_mthumb, options::OPT_mno_thumb,
+  ThumbDefault)) ||
+(InputType == types::TY_PP_Asm && IsIntegratedAssemblerThumb) ||
+IsMProfile || getTriple().isOSWindows()) {
   if (IsBigEndian)
 ArchName = "thumbeb";
   else


Index: test/Driver/arm-target-as-mthumb.s
===
--- /dev/null
+++ test/Driver/arm-target-as-mthumb.s
@@ -0,0 +1,17 @@
+// Make sure -mthumb does not affect assembler triple, but -Wa,-mthumb or
+// -Xassembler -mthumb does. Also check that -Wa,-mthumb or -Xassembler -mthumb
+// does not affect non assembler files.
+
+// RUN: %clang -target armv7a-linux-gnueabi -### -c -mthumb %s 2>&1 | \
+// RUN: FileCheck -check-prefix=TRIPLE-ARM %s
+// RUN: %clang -target armv7a-linux-gnueabi -### -c -Wa,-mthumb \
+// RUN: %S/Inputs/wildcard1.c  2>&1 | FileCheck -check-prefix=TRIPLE-ARM %s
+
+// TRIPLE-ARM: "-triple" "armv7--linux-gnueabi"
+
+// RUN:

[PATCH] D40127: [Driver][ARM] For assembler files recognize -Xassembler or -Wa, -mthumb

[Peter Smith via Phabricator via cfe-commits]

peter.smith added inline comments.



Comment at: lib/Driver/ToolChain.cpp:549-556
+bool IsIntegratedAssemblerThumb = false;
+for (const Arg *A :
+ Args.filtered(options::OPT_Wa_COMMA, options::OPT_Xassembler)) {
+  for (StringRef Value : A->getValues()) {
+if (Value == "-mthumb")
+  IsIntegratedAssemblerThumb = true;
+  }

compnerd wrote:
> Why not write this as:
> 
> const auto &Values = Args.filtered(options::OPT_Wa_COMMA, 
> options::OPT_Xassembler);
> bool IsIntegratedAssemblerThumb =
> std::any_of(std::begin(A->getValues()),
> std::end(A->getValues()),
> [](StringRef Arg) { return Arg == "-mthumb"});
I gave it a try and I don't think it can be done simply as there are two loops 
and not one. There could be two Args, one for Wa_COMMA and one for X_Assembler. 
Unless there is a way of combining the iterator ranges into one I don't think 
this can be done without nested loops. I checked the other uses of 
Args.filtered and I couldn't find any of use outside of the range for. Please 
do let me know if I'm missing something though as I'm happy to change it if 
there is something better. 



Comment at: lib/Driver/ToolChain.cpp:560-564
+if ((InputType != types::TY_PP_Asm &&
+ Args.hasFlag(options::OPT_mthumb, options::OPT_mno_thumb,
+  ThumbDefault)) ||
+(InputType == types::TY_PP_Asm && IsIntegratedAssemblerThumb) ||
+IsMProfile || getTriple().isOSWindows()) {

compnerd wrote:
> This is horribly complicated to read.  Can we just split this out of the 
> condition and create a variable?
Agreed. I'll post an alternative that attempts to evaluate -mthumb before the 
boolean expression. 


https://reviews.llvm.org/D40127



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

[PATCH] D40127: [Driver][ARM] For assembler files recognize -Xassembler or -Wa, -mthumb

[Peter Smith via Phabricator via cfe-commits]

peter.smith updated this revision to Diff 123310.
peter.smith added a comment.

Updated diff with an attempt to simplify the check for filetype and mthumb. 
I've left the existing Args.filtered in expression for now as I couldn't make a 
better alternative with std::for_any.


https://reviews.llvm.org/D40127

Files:
  lib/Driver/ToolChain.cpp
  lib/Driver/ToolChains/Clang.cpp
  test/Driver/arm-target-as-mthumb.s


Index: test/Driver/arm-target-as-mthumb.s
===
--- /dev/null
+++ test/Driver/arm-target-as-mthumb.s
@@ -0,0 +1,17 @@
+// Make sure -mthumb does not affect assembler triple, but -Wa,-mthumb or
+// -Xassembler -mthumb does. Also check that -Wa,-mthumb or -Xassembler -mthumb
+// does not affect non assembler files.
+
+// RUN: %clang -target armv7a-linux-gnueabi -### -c -mthumb %s 2>&1 | \
+// RUN: FileCheck -check-prefix=TRIPLE-ARM %s
+// RUN: %clang -target armv7a-linux-gnueabi -### -c -Wa,-mthumb \
+// RUN: %S/Inputs/wildcard1.c  2>&1 | FileCheck -check-prefix=TRIPLE-ARM %s
+
+// TRIPLE-ARM: "-triple" "armv7--linux-gnueabi"
+
+// RUN: %clang -target armv7a-linux-gnueabi -### -c -Wa,-mthumb %s 2>&1 | \
+// RUN: FileCheck -check-prefix=TRIPLE-THUMB %s
+// RUN: %clang -target armv7a-linux-gnueabi -### -c -Xassembler -mthumb %s \
+// RUN: 2>&1 | FileCheck -check-prefix=TRIPLE-THUMB %s
+
+// TRIPLE-THUMB: "-triple" "thumbv7--linux-gnueabi"
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -1889,6 +1889,15 @@
   switch (C.getDefaultToolChain().getArch()) {
   default:
 break;
+  case llvm::Triple::thumb:
+  case llvm::Triple::thumbeb:
+  case llvm::Triple::arm:
+  case llvm::Triple::armeb:
+if (Value == "-mthumb")
+  // -mthumb has already been processed in ComputeLLVMTriple()
+  // recognize but skip over here.
+  continue;
+
   case llvm::Triple::mips:
   case llvm::Triple::mipsel:
   case llvm::Triple::mips64:
Index: lib/Driver/ToolChain.cpp
===
--- lib/Driver/ToolChain.cpp
+++ lib/Driver/ToolChain.cpp
@@ -541,11 +541,30 @@
   << tools::arm::getARMArch(MArch, getTriple()) << "ARM";
 }
 
-// Assembly files should start in ARM mode, unless arch is M-profile.
-// Windows is always thumb.
-if ((InputType != types::TY_PP_Asm && Args.hasFlag(options::OPT_mthumb,
- options::OPT_mno_thumb, ThumbDefault)) || IsMProfile ||
- getTriple().isOSWindows()) {
+// Check to see if an explicit choice to use thumb has been made via
+// -mthumb. For assembler files we must check for -mthumb in the options
+// passed to the assember via -Wa or -Xassembler.
+bool IsMThumb = false;
+if (InputType != types::TY_PP_Asm)
+  IsMThumb = Args.hasFlag(options::OPT_mthumb, options::OPT_mno_thumb,
+  ThumbDefault);
+else {
+  // Ideally we would check for these flags in
+  // CollectArgsForIntegratedAssembler but we can't change the ArchName at
+  // that point. There is no assembler equivalent of -mno-thumb, -marm, or
+  // -mno-arm.
+  for (const Arg *A :
+   Args.filtered(options::OPT_Wa_COMMA, options::OPT_Xassembler)) {
+for (StringRef Value : A->getValues()) {
+  if (Value == "-mthumb")
+IsMThumb = true;
+}
+  }
+}
+// Assembly files should start in ARM mode, unless arch is M-profile, or
+// -mthumb has been passed explicitly to the assembler. Windows is always
+// thumb.
+if (IsMThumb || IsMProfile || getTriple().isOSWindows()) {
   if (IsBigEndian)
 ArchName = "thumbeb";
   else


Index: test/Driver/arm-target-as-mthumb.s
===
--- /dev/null
+++ test/Driver/arm-target-as-mthumb.s
@@ -0,0 +1,17 @@
+// Make sure -mthumb does not affect assembler triple, but -Wa,-mthumb or
+// -Xassembler -mthumb does. Also check that -Wa,-mthumb or -Xassembler -mthumb
+// does not affect non assembler files.
+
+// RUN: %clang -target armv7a-linux-gnueabi -### -c -mthumb %s 2>&1 | \
+// RUN: FileCheck -check-prefix=TRIPLE-ARM %s
+// RUN: %clang -target armv7a-linux-gnueabi -### -c -Wa,-mthumb \
+// RUN: %S/Inputs/wildcard1.c  2>&1 | FileCheck -check-prefix=TRIPLE-ARM %s
+
+// TRIPLE-ARM: "-triple" "armv7--linux-gnueabi"
+
+// RUN: %clang -target armv7a-linux-gnueabi -### -c -Wa,-mthumb %s 2>&1 | \
+// RUN: FileCheck -check-prefix=TRIPLE-THUMB %s
+// RUN: %clang -target armv7a-linux-gnueabi -### -c -Xassembler -mthumb %s \
+// RUN: 2>&1 | FileCheck -check-prefix=TRIPLE-THUMB %s
+
+// TRIPLE-THUMB: "-triple" "thumbv7--linux-gnueabi"
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChain

[PATCH] D40127: [Driver][ARM] For assembler files recognize -Xassembler or -Wa, -mthumb

[Peter Smith via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL318647: [ARM] For assembler files recognize -Xassembler or 
-Wa, -mthumb (authored by psmith).

Changed prior to commit:
  https://reviews.llvm.org/D40127?vs=123310&id=123571#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D40127

Files:
  cfe/trunk/lib/Driver/ToolChain.cpp
  cfe/trunk/lib/Driver/ToolChains/Clang.cpp
  cfe/trunk/test/Driver/arm-target-as-mthumb.s


Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp
@@ -1889,6 +1889,15 @@
   switch (C.getDefaultToolChain().getArch()) {
   default:
 break;
+  case llvm::Triple::thumb:
+  case llvm::Triple::thumbeb:
+  case llvm::Triple::arm:
+  case llvm::Triple::armeb:
+if (Value == "-mthumb")
+  // -mthumb has already been processed in ComputeLLVMTriple()
+  // recognize but skip over here.
+  continue;
+
   case llvm::Triple::mips:
   case llvm::Triple::mipsel:
   case llvm::Triple::mips64:
Index: cfe/trunk/lib/Driver/ToolChain.cpp
===
--- cfe/trunk/lib/Driver/ToolChain.cpp
+++ cfe/trunk/lib/Driver/ToolChain.cpp
@@ -541,11 +541,30 @@
   << tools::arm::getARMArch(MArch, getTriple()) << "ARM";
 }
 
-// Assembly files should start in ARM mode, unless arch is M-profile.
-// Windows is always thumb.
-if ((InputType != types::TY_PP_Asm && Args.hasFlag(options::OPT_mthumb,
- options::OPT_mno_thumb, ThumbDefault)) || IsMProfile ||
- getTriple().isOSWindows()) {
+// Check to see if an explicit choice to use thumb has been made via
+// -mthumb. For assembler files we must check for -mthumb in the options
+// passed to the assember via -Wa or -Xassembler.
+bool IsThumb = false;
+if (InputType != types::TY_PP_Asm)
+  IsThumb = Args.hasFlag(options::OPT_mthumb, options::OPT_mno_thumb,
+  ThumbDefault);
+else {
+  // Ideally we would check for these flags in
+  // CollectArgsForIntegratedAssembler but we can't change the ArchName at
+  // that point. There is no assembler equivalent of -mno-thumb, -marm, or
+  // -mno-arm.
+  for (const Arg *A :
+   Args.filtered(options::OPT_Wa_COMMA, options::OPT_Xassembler)) {
+for (StringRef Value : A->getValues()) {
+  if (Value == "-mthumb")
+IsThumb = true;
+}
+  }
+}
+// Assembly files should start in ARM mode, unless arch is M-profile, or
+// -mthumb has been passed explicitly to the assembler. Windows is always
+// thumb.
+if (IsThumb || IsMProfile || getTriple().isOSWindows()) {
   if (IsBigEndian)
 ArchName = "thumbeb";
   else
Index: cfe/trunk/test/Driver/arm-target-as-mthumb.s
===
--- cfe/trunk/test/Driver/arm-target-as-mthumb.s
+++ cfe/trunk/test/Driver/arm-target-as-mthumb.s
@@ -0,0 +1,17 @@
+// Make sure -mthumb does not affect assembler triple, but -Wa,-mthumb or
+// -Xassembler -mthumb does. Also check that -Wa,-mthumb or -Xassembler -mthumb
+// does not affect non assembler files.
+
+// RUN: %clang -target armv7a-linux-gnueabi -### -c -mthumb %s 2>&1 | \
+// RUN: FileCheck -check-prefix=TRIPLE-ARM %s
+// RUN: %clang -target armv7a-linux-gnueabi -### -c -Wa,-mthumb \
+// RUN: %S/Inputs/wildcard1.c  2>&1 | FileCheck -check-prefix=TRIPLE-ARM %s
+
+// TRIPLE-ARM: "-triple" "armv7--linux-gnueabi"
+
+// RUN: %clang -target armv7a-linux-gnueabi -### -c -Wa,-mthumb %s 2>&1 | \
+// RUN: FileCheck -check-prefix=TRIPLE-THUMB %s
+// RUN: %clang -target armv7a-linux-gnueabi -### -c -Xassembler -mthumb %s \
+// RUN: 2>&1 | FileCheck -check-prefix=TRIPLE-THUMB %s
+
+// TRIPLE-THUMB: "-triple" "thumbv7--linux-gnueabi"


Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp
@@ -1889,6 +1889,15 @@
   switch (C.getDefaultToolChain().getArch()) {
   default:
 break;
+  case llvm::Triple::thumb:
+  case llvm::Triple::thumbeb:
+  case llvm::Triple::arm:
+  case llvm::Triple::armeb:
+if (Value == "-mthumb")
+  // -mthumb has already been processed in ComputeLLVMTriple()
+  // recognize but skip over here.
+  continue;
+
   case llvm::Triple::mips:
   case llvm::Triple::mipsel:
   case llvm::Triple::mips64:
Index: cfe/trunk/lib/Driver/ToolChain.cpp
===
--- cfe/trunk/lib/Driver/ToolChain.cpp
+++ cfe/trunk/lib/Driver/ToolChain.cpp
@@ -541,11 +541,30 @@
   << tools::arm::getARMArch(MArch, getTriple

[PATCH] D40127: [Driver][ARM] For assembler files recognize -Xassembler or -Wa, -mthumb

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

In https://reviews.llvm.org/D40127#929578, @compnerd wrote:

> Would be nice to rename the variable prior to commit.


Thanks for the review, I've renamed the variable as suggested.


https://reviews.llvm.org/D40127



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

[PATCH] D142905: [Driver] Change multilib selection algorithm

[Peter Smith via Phabricator via cfe-commits]

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

I've set approved from the Arm side. Please leave some time for people in the 
US time zone to leave any final comments or ask for extensions. I've left a 
suggestion for a comment, but this can be applied prior to commit if you want 
to take it up.




Comment at: clang/include/clang/Driver/Multilib.h:67
+  /// a subset of the flags derived from the Clang command line options.
+  const flag_set &flags() const { return Flags; }
 

Would be good to reference the Multilib Design document's definition and 
examples of (what will be renamed in D145567 to tags).

This may require moving the design doc down the stack, probably not necessary 
if everything is committed at once.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142905

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

[PATCH] D145567: [Driver] Rename multilib flags to tags

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

I've set approved from the Arm side. Please leave some time for people in the 
US time zone to leave any final comments or ask for extensions. 
From looking at the source code alone - assuming that most people would not 
track down the commit message/review for extra context - I found it difficult 
to work out the convention for when Flags is used and when Tags is used. I've 
made a suggestion for some comments. This can be applied prior to commit if you 
want to take it up.




Comment at: clang/lib/Driver/ToolChains/CommonArgs.h:202
+ Multilib::tag_set &Flags);
 
 void addX86AlignBranchArgs(const Driver &D, const llvm::opt::ArgList &Args,

I can see the reason to keep the name `addMultilibFlag`. At this point is the 
tag_set expected to be simplified tags or full command line flags. If it is the 
former I think it would be good to change Flags to Tags here.

May also be useul to add a \p for Flags (or Tags) if there are any 
requirements, or just useful information on what it is expected to be.

Parameter name also applies to CommonArgs.cpp below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145567

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

[PATCH] D145567: [Driver] Rename multilib flags to tags

[Peter Smith via Phabricator via cfe-commits]

peter.smith accepted this revision.
peter.smith added a comment.

Actually click the button this time to set approval, see previous comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145567

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

[PATCH] D142932: Multilib YAML parsing

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

I've set approved from the Arm side. Please leave some time for people in the 
US time zone to leave any final comments or ask for extensions. I've left some 
comments that can be applied prior to commit if you want to take them up.




Comment at: clang/include/clang/Driver/Multilib.h:66
+  /// options and look similar to them, and others can be defined by a
+  /// particular multilib.yaml. A multilib is considered compatible if its tags
+  /// are a subset of the tags derived from the Clang command line options.

With the context of https://discourse.llvm.org/t/rfc-multilib/67494 and 
potential other formats. I think it will be worth making this comment not 
specific to multilib.yaml. For example if there is another non-YAML DSL that 
has tags.

For example `multilib DSL` rather than `multilib.yaml`  



Comment at: clang/include/clang/Driver/Multilib.h:140
+
+  bool parse(llvm::MemoryBufferRef, llvm::SourceMgr::DiagHandlerTy = nullptr,
+ void *DiagHandlerCtxt = nullptr);

With reference to https://discourse.llvm.org/t/rfc-multilib/67494 perhaps 
rename this to `parseMultilibYaml` as parse is generic, yet there is no 
indication it is working on a yaml file here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142932

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

[PATCH] D142933: Add -print-multi-selection-tags-experimental option

[Peter Smith via Phabricator via cfe-commits]

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

I've set approved from the Arm side. Please leave some time for people in the 
US time zone to leave any final comments or ask for extensions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142933

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

[PATCH] D142986: Enable multilib.yaml in the BareMetal ToolChain

[Peter Smith via Phabricator via cfe-commits]

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

I've set approved from the Arm side. Please leave some time for people in the 
US time zone to leave any final comments or ask for extensions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142986

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

[PATCH] D143059: [Driver] Enable selecting multiple multilibs

[Peter Smith via Phabricator via cfe-commits]

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

I've set approved from the Arm side. Please leave some time for people in the 
US time zone to leave any final comments or ask for extensions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143059

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

[PATCH] D143075: BareMetal ToolChain multilib layering

[Peter Smith via Phabricator via cfe-commits]

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

I've set approved from the Arm side. Please leave some time for people in the 
US time zone to leave any final comments or ask for extensions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143075

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

[PATCH] D143587: [Docs] Multilib design

[Peter Smith via Phabricator via cfe-commits]

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

I've set approved from the Arm side. Please leave some time for people in the 
US time zone to leave any final comments or ask for extensions. I've left a 
suggestion that can be applied prior to commit if you want to take it up.




Comment at: clang/docs/Multilib.rst:56
+them, and the criteria by which they are selected.
+
+Multilib processing

Can we add a forward reference to the Stable Interface design principle, or 
perhaps move it here so that it is more obvious?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143587

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