subject:"\[PATCH\] D86488\: \[X86\] Default to \-mtune=generic unless \-march is passed to the driver. Add TuneCPU to the AST serialization"

[PATCH] D86488: [X86] Default to -mtune=generic unless -march is passed to the driver. Add TuneCPU to the AST serialization

2020-08-27 Thread Craig Topper via Phabricator via cfe-commits

craig.topper added a comment.

In D86488#2241176 , @nikic wrote:

> This change has a 0.3% compile-time regression 
> (https://llvm-compile-time-tracker.com/compare.php?from=0c55889d809027136048a0d144209a2bc282e7fc=71f3169e1baeff262583b35ef88f8fb6df7be85e=instructions)
>  and a 0.3% max-rss regression 
> (https://llvm-compile-time-tracker.com/compare.php?from=0c55889d809027136048a0d144209a2bc282e7fc=71f3169e1baeff262583b35ef88f8fb6df7be85e=max-rss).
>
> Is that expected? The diff looks pretty harmless to me.

No it wasnt expected. There was already a comment on the commit about it. The 
settings it’s selecting should match the default we get with no -march on 
Linux. The only thing I’ve found so far is extra time to read the function 
attribute from getSubtargetImpl which happen every time getTTI is called. And 
extra time to add the function attribute in clang. The strange thing is it 
seems to have regressed O0 even so that rules out a lot of optimization related 
code. Any information you can provide would be helpful.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86488

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

[PATCH] D86488: [X86] Default to -mtune=generic unless -march is passed to the driver. Add TuneCPU to the AST serialization

2020-08-27 Thread Nikita Popov via Phabricator via cfe-commits

nikic added a comment.

This change has a 0.3% compile-time regression 
(https://llvm-compile-time-tracker.com/compare.php?from=0c55889d809027136048a0d144209a2bc282e7fc=71f3169e1baeff262583b35ef88f8fb6df7be85e=instructions)
 and a 0.3% max-rss regression 
(https://llvm-compile-time-tracker.com/compare.php?from=0c55889d809027136048a0d144209a2bc282e7fc=71f3169e1baeff262583b35ef88f8fb6df7be85e=max-rss).

Is that expected? The diff looks pretty harmless to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86488

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

[PATCH] D86488: [X86] Default to -mtune=generic unless -march is passed to the driver. Add TuneCPU to the AST serialization

2020-08-26 Thread Craig Topper via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG71f3169e1bae: [X86] Default to -mtune=generic unless -march 
is passed to the driver. Add… (authored by craig.topper).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86488

Files:
  clang/lib/Basic/Targets/X86.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Driver/x86-mtune.c
  clang/test/Modules/module_file_info.m

Index: clang/test/Modules/module_file_info.m
===
--- clang/test/Modules/module_file_info.m
+++ clang/test/Modules/module_file_info.m
@@ -28,6 +28,7 @@
 // CHECK: Target options:
 // CHECK: Triple:
 // CHECK: CPU:
+// CHECK: TuneCPU:
 // CHECK: ABI:
 
 // CHECK: Header search options:
Index: clang/test/Driver/x86-mtune.c
===
--- clang/test/Driver/x86-mtune.c
+++ clang/test/Driver/x86-mtune.c
@@ -1,5 +1,14 @@
 // Ensure we support the -mtune flag.
-//
+
+// Default mtune should be generic.
+// RUN: %clang -target x86_64-unknown-unknown -c -### %s 2>&1 \
+// RUN:   | FileCheck %s -check-prefix=notune
+// notune: "-tune-cpu" "generic"
+
+// RUN: %clang -target x86_64-unknown-unknown -c -### %s -mtune=generic 2>&1 \
+// RUN:   | FileCheck %s -check-prefix=generic
+// generic: "-tune-cpu" "generic"
+
 // RUN: %clang -target x86_64-unknown-unknown -c -### %s -mtune=nocona 2>&1 \
 // RUN:   | FileCheck %s -check-prefix=nocona
 // nocona: "-tune-cpu" "nocona"
@@ -18,3 +27,16 @@
 // RUN:   | FileCheck %s -check-prefix=athlon
 // athlon: "-tune-cpu" "athlon"
 
+// Check interaction between march and mtune.
+
+// -march should remove default mtune generic.
+// RUN: %clang -target x86_64-unknown-unknown -c -### %s -march=core2 2>&1 \
+// RUN:   | FileCheck %s -check-prefix=marchcore2
+// marchcore2: "-target-cpu" "core2"
+// marchcore2-NOT: "-tune-cpu"
+
+// -march should remove default mtune generic.
+// RUN: %clang -target x86_64-unknown-unknown -c -### %s -march=core2 -mtune=nehalem 2>&1 \
+// RUN:   | FileCheck %s -check-prefix=marchmtune
+// marchmtune: "-target-cpu" "core2"
+// mmarchmtune: "-tune-cpu" "nehalem"
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1272,6 +1272,7 @@
   const TargetOptions  = Target.getTargetOpts();
   AddString(TargetOpts.Triple, Record);
   AddString(TargetOpts.CPU, Record);
+  AddString(TargetOpts.TuneCPU, Record);
   AddString(TargetOpts.ABI, Record);
   Record.push_back(TargetOpts.FeaturesAsWritten.size());
   for (unsigned I = 0, N = TargetOpts.FeaturesAsWritten.size(); I != N; ++I) {
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -390,8 +390,10 @@
   // We can tolerate different CPUs in many cases, notably when one CPU
   // supports a strict superset of another. When allowing compatible
   // differences skip this check.
-  if (!AllowCompatibleDifferences)
+  if (!AllowCompatibleDifferences) {
 CHECK_TARGET_OPT(CPU, "target CPU");
+CHECK_TARGET_OPT(TuneCPU, "tune CPU");
+  }
 
 #undef CHECK_TARGET_OPT
 
@@ -5779,6 +5781,7 @@
   TargetOptions TargetOpts;
   TargetOpts.Triple = ReadString(Record, Idx);
   TargetOpts.CPU = ReadString(Record, Idx);
+  TargetOpts.TuneCPU = ReadString(Record, Idx);
   TargetOpts.ABI = ReadString(Record, Idx);
   for (unsigned N = Record[Idx++]; N; --N) {
 TargetOpts.FeaturesAsWritten.push_back(ReadString(Record, Idx));
Index: clang/lib/Frontend/FrontendActions.cpp
===
--- clang/lib/Frontend/FrontendActions.cpp
+++ clang/lib/Frontend/FrontendActions.cpp
@@ -561,6 +561,7 @@
   Out.indent(2) << "Target options:\n";
   Out.indent(4) << "  Triple: " << TargetOpts.Triple << "\n";
   Out.indent(4) << "  CPU: " << TargetOpts.CPU << "\n";
+  Out.indent(4) << "  TuneCPU: " << TargetOpts.TuneCPU << "\n";
   Out.indent(4) << "  ABI: " << TargetOpts.ABI << "\n";
 
   if (!TargetOpts.FeaturesAsWritten.empty()) {
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -3877,7 +3877,7 @@
 
   // Extend the signature with the target options.
   code = hash_combine(code, TargetOpts->Triple, TargetOpts->CPU,
-  TargetOpts->ABI);
+  TargetOpts->TuneCPU,

[PATCH] D86488: [X86] Default to -mtune=generic unless -march is passed to the driver. Add TuneCPU to the AST serialization

2020-08-26 Thread Craig Topper via Phabricator via cfe-commits

craig.topper added inline comments.



Comment at: clang/lib/Serialization/ASTReader.cpp:395
 CHECK_TARGET_OPT(CPU, "target CPU");
+CHECK_TARGET_OPT(TuneCPU, "tune CPU");
+  }

erichkeane wrote:
> What does this string do?  Does there have to be a test for it?
It seems to be used by a diagnostic. But I couldn't find any tests for the 
diagnostic for "target CPU" that I could copy from.


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

https://reviews.llvm.org/D86488

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

[PATCH] D86488: [X86] Default to -mtune=generic unless -march is passed to the driver. Add TuneCPU to the AST serialization

2020-08-26 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added inline comments.



Comment at: clang/lib/Serialization/ASTReader.cpp:395
 CHECK_TARGET_OPT(CPU, "target CPU");
+CHECK_TARGET_OPT(TuneCPU, "tune CPU");
+  }

What does this string do?  Does there have to be a test for it?


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

https://reviews.llvm.org/D86488

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

[PATCH] D86488: [X86] Default to -mtune=generic unless -march is passed to the driver. Add TuneCPU to the AST serialization

2020-08-24 Thread Craig Topper via Phabricator via cfe-commits

craig.topper created this revision.
craig.topper added reviewers: spatel, erichkeane, RKSimon, echristo, efriedma.
craig.topper requested review of this revision.

This patch defaults to -mtune=generic unless -march is present. If -march is 
present we'll use the empty string unless its overridden by mtune. The back 
should use the target cpu if the tune-cpu isn't present.

It also adds AST serialization support to fix some tests that emit AST and 
parse it back. These tests diff the IR against the output from not going 
through AST. So if we don't serialize the tune CPU we fail the diff.


https://reviews.llvm.org/D86488

Files:
  clang/lib/Basic/Targets/X86.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Frontend/FrontendActions.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Driver/x86-mtune.c
  clang/test/Modules/module_file_info.m

Index: clang/test/Modules/module_file_info.m
===
--- clang/test/Modules/module_file_info.m
+++ clang/test/Modules/module_file_info.m
@@ -28,6 +28,7 @@
 // CHECK: Target options:
 // CHECK: Triple:
 // CHECK: CPU:
+// CHECK: TuneCPU:
 // CHECK: ABI:
 
 // CHECK: Header search options:
Index: clang/test/Driver/x86-mtune.c
===
--- clang/test/Driver/x86-mtune.c
+++ clang/test/Driver/x86-mtune.c
@@ -1,5 +1,14 @@
 // Ensure we support the -mtune flag.
-//
+
+// Default mtune should be generic.
+// RUN: %clang -target x86_64-unknown-unknown -c -### %s 2>&1 \
+// RUN:   | FileCheck %s -check-prefix=notune
+// notune: "-tune-cpu" "generic"
+
+// RUN: %clang -target x86_64-unknown-unknown -c -### %s -mtune=generic 2>&1 \
+// RUN:   | FileCheck %s -check-prefix=generic
+// generic: "-tune-cpu" "generic"
+
 // RUN: %clang -target x86_64-unknown-unknown -c -### %s -mtune=nocona 2>&1 \
 // RUN:   | FileCheck %s -check-prefix=nocona
 // nocona: "-tune-cpu" "nocona"
@@ -18,3 +27,16 @@
 // RUN:   | FileCheck %s -check-prefix=athlon
 // athlon: "-tune-cpu" "athlon"
 
+// Check interaction between march and mtune.
+
+// -march should remove default mtune generic.
+// RUN: %clang -target x86_64-unknown-unknown -c -### %s -march=core2 2>&1 \
+// RUN:   | FileCheck %s -check-prefix=marchcore2
+// marchcore2: "-target-cpu" "core2"
+// marchcore2-NOT: "-tune-cpu"
+
+// -march should remove default mtune generic.
+// RUN: %clang -target x86_64-unknown-unknown -c -### %s -march=core2 -mtune=nehalem 2>&1 \
+// RUN:   | FileCheck %s -check-prefix=marchmtune
+// marchmtune: "-target-cpu" "core2"
+// mmarchmtune: "-tune-cpu" "nehalem"
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1272,6 +1272,7 @@
   const TargetOptions  = Target.getTargetOpts();
   AddString(TargetOpts.Triple, Record);
   AddString(TargetOpts.CPU, Record);
+  AddString(TargetOpts.TuneCPU, Record);
   AddString(TargetOpts.ABI, Record);
   Record.push_back(TargetOpts.FeaturesAsWritten.size());
   for (unsigned I = 0, N = TargetOpts.FeaturesAsWritten.size(); I != N; ++I) {
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -390,8 +390,10 @@
   // We can tolerate different CPUs in many cases, notably when one CPU
   // supports a strict superset of another. When allowing compatible
   // differences skip this check.
-  if (!AllowCompatibleDifferences)
+  if (!AllowCompatibleDifferences) {
 CHECK_TARGET_OPT(CPU, "target CPU");
+CHECK_TARGET_OPT(TuneCPU, "tune CPU");
+  }
 
 #undef CHECK_TARGET_OPT
 
@@ -5779,6 +5781,7 @@
   TargetOptions TargetOpts;
   TargetOpts.Triple = ReadString(Record, Idx);
   TargetOpts.CPU = ReadString(Record, Idx);
+  TargetOpts.TuneCPU = ReadString(Record, Idx);
   TargetOpts.ABI = ReadString(Record, Idx);
   for (unsigned N = Record[Idx++]; N; --N) {
 TargetOpts.FeaturesAsWritten.push_back(ReadString(Record, Idx));
Index: clang/lib/Frontend/FrontendActions.cpp
===
--- clang/lib/Frontend/FrontendActions.cpp
+++ clang/lib/Frontend/FrontendActions.cpp
@@ -561,6 +561,7 @@
   Out.indent(2) << "Target options:\n";
   Out.indent(4) << "  Triple: " << TargetOpts.Triple << "\n";
   Out.indent(4) << "  CPU: " << TargetOpts.CPU << "\n";
+  Out.indent(4) << "  TuneCPU: " << TargetOpts.TuneCPU << "\n";
   Out.indent(4) << "  ABI: " << TargetOpts.ABI << "\n";
 
   if (!TargetOpts.FeaturesAsWritten.empty()) {
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++

[PATCH] D86488: [X86] Default to -mtune=generic unless -march is passed to the driver. Add TuneCPU to the AST serialization

[PATCH] D86488: [X86] Default to -mtune=generic unless -march is passed to the driver. Add TuneCPU to the AST serialization

[PATCH] D86488: [X86] Default to -mtune=generic unless -march is passed to the driver. Add TuneCPU to the AST serialization

[PATCH] D86488: [X86] Default to -mtune=generic unless -march is passed to the driver. Add TuneCPU to the AST serialization

[PATCH] D86488: [X86] Default to -mtune=generic unless -march is passed to the driver. Add TuneCPU to the AST serialization

[PATCH] D86488: [X86] Default to -mtune=generic unless -march is passed to the driver. Add TuneCPU to the AST serialization

6 matches

Site Navigation

Mail list logo

Footer information