[PATCH] D76525: Expose cache line size in __builtin_get_cpu_cache_line_size

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

In D76525#1942825 , @zoecarver wrote:

> @craig.topper I'm not sure what should happen. It should probably just use 
> that CPU (although that's not a great solution). Is there a way to detect if 
> an attribute was used to change the target in which case we could error? What 
> do you think should happen?


I'm not sure what should happen as I'm not familiar with the library feature 
being implemented. As implemented here, the target attribute will be ignored 
and it will just follow the command line. Why do you say that following the 
attribute is not a great solution?




Comment at: clang/test/SemaCXX/builtin-cache-line-size.cpp:1
+// RUN: %clang -mcpu=i368 %s
+// RUN: ./a.out

This say i368 which is not a valid cpu. Did you mean i386? Also x86 uses use 
-march not -mcpu. I don't think -mcpu does anything.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76525



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

[PATCH] D76525: Expose cache line size in __builtin_get_cpu_cache_line_size

[Zoe Carver via Phabricator via cfe-commits]

zoecarver updated this revision to Diff 252742.
zoecarver added a comment.

- Diff from master


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76525

Files:
  clang/include/clang/Basic/Builtins.def
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/builtin-cache-line-size.cpp


Index: clang/test/SemaCXX/builtin-cache-line-size.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/builtin-cache-line-size.cpp
@@ -0,0 +1,32 @@
+// RUN: %clang -mcpu=i368 %s
+// RUN: ./a.out
+
+#include 
+
+size_t a() {
+  return __builtin_get_cpu_cache_line_size();
+}
+
+size_t b(bool x) {
+  if (x) {
+return __builtin_get_cpu_cache_line_size();
+  }
+  return 0;
+}
+
+int main() {
+  assert(a() == 64);
+  assert(b(true) == 64);
+  assert(b(false) == 0);
+}
+
+// This is based on the value for i368.
+static_assert(__builtin_get_cpu_cache_line_size() == 64, "");
+
+struct keep_apart {
+  alignas(__builtin_get_cpu_cache_line_size()) int cat;
+  alignas(__builtin_get_cpu_cache_line_size()) int dog;
+};
+
+static_assert(sizeof(keep_apart) == __builtin_get_cpu_cache_line_size() * 2 ||
+  __builtin_get_cpu_cache_line_size() == 0, "");
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -10915,6 +10915,14 @@
 return Success(Info.InConstantContext, E);
   }
 
+  case Builtin::BI__builtin_get_cpu_cache_line_size: {
+Optional cacheLineSize =
+  Info.Ctx.getTargetInfo().getCPUCacheLineSize();
+if (cacheLineSize.hasValue())
+  return Success(cacheLineSize.getValue(), E);
+return Success(0, E);
+  }
+
   case Builtin::BI__builtin_ctz:
   case Builtin::BI__builtin_ctzl:
   case Builtin::BI__builtin_ctzll:
Index: clang/include/clang/Basic/Builtins.def
===
--- clang/include/clang/Basic/Builtins.def
+++ clang/include/clang/Basic/Builtins.def
@@ -1480,6 +1480,7 @@
 BUILTIN(__builtin_char_memchr, "c*cC*iz", "n")
 BUILTIN(__builtin_dump_struct, "ivC*v*", "tn")
 BUILTIN(__builtin_preserve_access_index, "v.", "t")
+BUILTIN(__builtin_get_cpu_cache_line_size, "zv", "nc")
 
 // Alignment builtins (uses custom parsing to support pointers and integers)
 BUILTIN(__builtin_is_aligned, "bvC*z", "nct")


Index: clang/test/SemaCXX/builtin-cache-line-size.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/builtin-cache-line-size.cpp
@@ -0,0 +1,32 @@
+// RUN: %clang -mcpu=i368 %s
+// RUN: ./a.out
+
+#include 
+
+size_t a() {
+  return __builtin_get_cpu_cache_line_size();
+}
+
+size_t b(bool x) {
+  if (x) {
+return __builtin_get_cpu_cache_line_size();
+  }
+  return 0;
+}
+
+int main() {
+  assert(a() == 64);
+  assert(b(true) == 64);
+  assert(b(false) == 0);
+}
+
+// This is based on the value for i368.
+static_assert(__builtin_get_cpu_cache_line_size() == 64, "");
+
+struct keep_apart {
+  alignas(__builtin_get_cpu_cache_line_size()) int cat;
+  alignas(__builtin_get_cpu_cache_line_size()) int dog;
+};
+
+static_assert(sizeof(keep_apart) == __builtin_get_cpu_cache_line_size() * 2 ||
+  __builtin_get_cpu_cache_line_size() == 0, "");
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -10915,6 +10915,14 @@
 return Success(Info.InConstantContext, E);
   }
 
+  case Builtin::BI__builtin_get_cpu_cache_line_size: {
+Optional cacheLineSize =
+  Info.Ctx.getTargetInfo().getCPUCacheLineSize();
+if (cacheLineSize.hasValue())
+  return Success(cacheLineSize.getValue(), E);
+return Success(0, E);
+  }
+
   case Builtin::BI__builtin_ctz:
   case Builtin::BI__builtin_ctzl:
   case Builtin::BI__builtin_ctzll:
Index: clang/include/clang/Basic/Builtins.def
===
--- clang/include/clang/Basic/Builtins.def
+++ clang/include/clang/Basic/Builtins.def
@@ -1480,6 +1480,7 @@
 BUILTIN(__builtin_char_memchr, "c*cC*iz", "n")
 BUILTIN(__builtin_dump_struct, "ivC*v*", "tn")
 BUILTIN(__builtin_preserve_access_index, "v.", "t")
+BUILTIN(__builtin_get_cpu_cache_line_size, "zv", "nc")
 
 // Alignment builtins (uses custom parsing to support pointers and integers)
 BUILTIN(__builtin_is_aligned, "bvC*z", "nct")
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76525: Expose cache line size in __builtin_get_cpu_cache_line_size

[Zoe Carver via Phabricator via cfe-commits]

zoecarver updated this revision to Diff 252741.
zoecarver added a comment.

- Fix based on review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76525

Files:
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/ExprConstant.cpp
  clang/lib/Basic/Targets/X86.cpp
  clang/lib/Basic/Targets/X86.h
  clang/test/SemaCXX/builtin-cache-line-size.cpp

Index: clang/test/SemaCXX/builtin-cache-line-size.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/builtin-cache-line-size.cpp
@@ -0,0 +1,32 @@
+// RUN: %clang -mcpu=i368 %s
+// RUN: ./a.out
+
+#include 
+
+size_t a() {
+  return __builtin_get_cpu_cache_line_size();
+}
+
+size_t b(bool x) {
+  if (x) {
+return __builtin_get_cpu_cache_line_size();
+  }
+  return 0;
+}
+
+int main() {
+  assert(a() == 64);
+  assert(b(true) == 64);
+  assert(b(false) == 0);
+}
+
+// This is based on the value for i368.
+static_assert(__builtin_get_cpu_cache_line_size() == 64, "");
+
+struct keep_apart {
+  alignas(__builtin_get_cpu_cache_line_size()) int cat;
+  alignas(__builtin_get_cpu_cache_line_size()) int dog;
+};
+
+static_assert(sizeof(keep_apart) == __builtin_get_cpu_cache_line_size() * 2 ||
+  __builtin_get_cpu_cache_line_size() == 0, "");
Index: clang/lib/Basic/Targets/X86.h
===
--- clang/lib/Basic/Targets/X86.h
+++ clang/lib/Basic/Targets/X86.h
@@ -182,6 +182,8 @@
   StringRef Name,
   llvm::SmallVectorImpl &Features) const override;
 
+  Optional getCPUCacheLineSize() const override;
+
   bool validateAsmConstraint(const char *&Name,
  TargetInfo::ConstraintInfo &info) const override;
 
Index: clang/lib/Basic/Targets/X86.cpp
===
--- clang/lib/Basic/Targets/X86.cpp
+++ clang/lib/Basic/Targets/X86.cpp
@@ -1731,6 +1731,118 @@
   }
 }
 
+// Below is based on the following information:
+// ++-+--+
+// |   Processor Name   | Cache Line Size (Bytes) |Source|
+// ++-+--+
+// | i386   |  64 | https://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-optimization-manual.pdf  |
+// | i486   |  16 | "four doublewords" (doubleword = 32 bits, 4 bits * 32 bits = 16 bytes) https://en.wikichip.org/w/images/d/d3/i486_MICROPROCESSOR_HARDWARE_REFERENCE_MANUAL_%281990%29.pdf and http://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.126.4216&rep=rep1&type=pdf (page 29) |
+// | i586/Pentium MMX   |  32 | https://www.7-cpu.com/cpu/P-MMX.html |
+// | i686/Pentium   |  32 | https://www.7-cpu.com/cpu/P6.html|
+// | Netburst/Pentium4  |  64 | https://www.7-cpu.com/cpu/P4-180.html|
+// | Atom   |  64 | https://www.7-cpu.com/cpu/Atom.html  |
+// | Westmere   |  64 | https://en.wikichip.org/wiki/intel/microarchitectures/sandy_bridge_(client) "Cache Architecture" |
+// | Sandy Bridge   |  64 | https://en.wikipedia.org/wiki/Sandy_Bridge and https://www.7-cpu.com/cpu/SandyBridge.html|
+// | Ivy Bridge |  64 | https://blog.stuffedcow.net/2013/01/ivb-cache-replacement/ and https://www.7-cpu.com/cpu/IvyBridge.html  |
+// | Haswell 

[PATCH] D76525: Expose cache line size in __builtin_get_cpu_cache_line_size

[Zoe Carver via Phabricator via cfe-commits]

zoecarver added a comment.

@craig.topper I'm not sure what should happen. It should probably just use that 
CPU (although that's not a great solution). Is there a way to detect if an 
attribute was used to change the target in which case we could error? What do 
you think should happen?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76525



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

[PATCH] D76525: Expose cache line size in __builtin_get_cpu_cache_line_size

[Zoe Carver via Phabricator via cfe-commits]

zoecarver added a comment.

@jyknight yes, the current plan is to use it in libc++'s implementation. I can 
put that implementation (which uses this builtin) up for review before this 
lands. That way we can discuss the implementation before adding a (possibly 
unneeded) builtin. Sound good?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76525



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

[PATCH] D76525: Expose cache line size in __builtin_get_cpu_cache_line_size

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

I don't think there's a particularly good reason to expose this as a builtin, 
unless it's to be used by the standard library implementation. (Which again I 
do not think we should implement.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76525



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

[PATCH] D76525: Expose cache line size in __builtin_get_cpu_cache_line_size

[Zoe Carver via Phabricator via cfe-commits]

zoecarver added a comment.

@craig.topper thanks for the comments! I'm going to hold off on updating this 
until we figure out what's happening with D74918 
, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76525



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

[PATCH] D76525: Expose cache line size in __builtin_get_cpu_cache_line_size

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

What should the behavior of the builtin be if the calling function has its own 
target cpu like __attribute__((target("arch=sandybridge"))) and that CPU has a 
different size than the CPU that was passed on the command line?




Comment at: clang/include/clang/Basic/Builtins.def:1483
 BUILTIN(__builtin_preserve_access_index, "v.", "t")
+BUILTIN(__builtin_get_cpu_cache_line_size, "z", "nctu")
 

Can this be "zv" and drop the "tu" from the second string?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76525



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

[PATCH] D76525: Expose cache line size in __builtin_get_cpu_cache_line_size

[Zoe Carver via Phabricator via cfe-commits]

zoecarver created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
zoecarver added reviewers: jfb, EricWF, efriedma, jyknight, echristo, __simt__.
Herald added a subscriber: dexonsmith.

This patch creates the __builtin_get_cpu_cache_line_size to wrap the cpu cache 
line size API from D74918 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76525

Files:
  clang/include/clang/Basic/Builtins.def
  clang/lib/AST/ExprConstant.cpp
  clang/test/SemaCXX/builtin-cache-line-size.cpp


Index: clang/test/SemaCXX/builtin-cache-line-size.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/builtin-cache-line-size.cpp
@@ -0,0 +1,32 @@
+// RUN: %clang -mcpu=i368 %s
+// RUN: ./a.out
+
+#include 
+
+size_t a() {
+  return __builtin_get_cpu_cache_line_size();
+}
+
+size_t b(bool x) {
+  if (x) {
+return __builtin_get_cpu_cache_line_size();
+  }
+  return 0;
+}
+
+int main() {
+  assert(a() == 64);
+  assert(b(true) == 64);
+  assert(b(false) == 0);
+}
+
+// This is based on the value for i368.
+static_assert(__builtin_get_cpu_cache_line_size() == 64, "");
+
+struct keep_apart {
+  alignas(__builtin_get_cpu_cache_line_size()) int cat;
+  alignas(__builtin_get_cpu_cache_line_size()) int dog;
+};
+
+static_assert(sizeof(keep_apart) == __builtin_get_cpu_cache_line_size() * 2 ||
+  __builtin_get_cpu_cache_line_size() == 0, "");
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -10910,6 +10910,13 @@
 return Success(Info.InConstantContext, E);
   }
 
+  case Builtin::BI__builtin_get_cpu_cache_line_size: {
+Optional cacheLineSize = 
Info.Ctx.getTargetInfo().getCPUCacheLineSize();
+if (cacheLineSize.hasValue())
+  return Success(cacheLineSize.getValue(), E);
+return Success(0, E);
+  }
+
   case Builtin::BI__builtin_ctz:
   case Builtin::BI__builtin_ctzl:
   case Builtin::BI__builtin_ctzll:
Index: clang/include/clang/Basic/Builtins.def
===
--- clang/include/clang/Basic/Builtins.def
+++ clang/include/clang/Basic/Builtins.def
@@ -1480,6 +1480,7 @@
 BUILTIN(__builtin_char_memchr, "c*cC*iz", "n")
 BUILTIN(__builtin_dump_struct, "ivC*v*", "tn")
 BUILTIN(__builtin_preserve_access_index, "v.", "t")
+BUILTIN(__builtin_get_cpu_cache_line_size, "z", "nctu")
 
 // Alignment builtins (uses custom parsing to support pointers and integers)
 BUILTIN(__builtin_is_aligned, "bvC*z", "nct")


Index: clang/test/SemaCXX/builtin-cache-line-size.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/builtin-cache-line-size.cpp
@@ -0,0 +1,32 @@
+// RUN: %clang -mcpu=i368 %s
+// RUN: ./a.out
+
+#include 
+
+size_t a() {
+  return __builtin_get_cpu_cache_line_size();
+}
+
+size_t b(bool x) {
+  if (x) {
+return __builtin_get_cpu_cache_line_size();
+  }
+  return 0;
+}
+
+int main() {
+  assert(a() == 64);
+  assert(b(true) == 64);
+  assert(b(false) == 0);
+}
+
+// This is based on the value for i368.
+static_assert(__builtin_get_cpu_cache_line_size() == 64, "");
+
+struct keep_apart {
+  alignas(__builtin_get_cpu_cache_line_size()) int cat;
+  alignas(__builtin_get_cpu_cache_line_size()) int dog;
+};
+
+static_assert(sizeof(keep_apart) == __builtin_get_cpu_cache_line_size() * 2 ||
+  __builtin_get_cpu_cache_line_size() == 0, "");
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -10910,6 +10910,13 @@
 return Success(Info.InConstantContext, E);
   }
 
+  case Builtin::BI__builtin_get_cpu_cache_line_size: {
+Optional cacheLineSize = Info.Ctx.getTargetInfo().getCPUCacheLineSize();
+if (cacheLineSize.hasValue())
+  return Success(cacheLineSize.getValue(), E);
+return Success(0, E);
+  }
+
   case Builtin::BI__builtin_ctz:
   case Builtin::BI__builtin_ctzl:
   case Builtin::BI__builtin_ctzll:
Index: clang/include/clang/Basic/Builtins.def
===
--- clang/include/clang/Basic/Builtins.def
+++ clang/include/clang/Basic/Builtins.def
@@ -1480,6 +1480,7 @@
 BUILTIN(__builtin_char_memchr, "c*cC*iz", "n")
 BUILTIN(__builtin_dump_struct, "ivC*v*", "tn")
 BUILTIN(__builtin_preserve_access_index, "v.", "t")
+BUILTIN(__builtin_get_cpu_cache_line_size, "z", "nctu")
 
 // Alignment builtins (uses custom parsing to support pointers and integers)
 BUILTIN(__builtin_is_aligned, "bvC*z", "nct")
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits