[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Zoe Carver via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rGb915aec6b591: Add method to TargetInfo to get CPU cache line 
size (authored by zoecarver).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918

Files:
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/Targets/X86.cpp
  clang/lib/Basic/Targets/X86.h

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 ) const override;
 
+  Optional getCPUCacheLineSize() const override;
+
   bool validateAsmConstraint(const char *,
  TargetInfo::ConstraintInfo ) const override;
 
Index: clang/lib/Basic/Targets/X86.cpp
===
--- clang/lib/Basic/Targets/X86.cpp
+++ clang/lib/Basic/Targets/X86.cpp
@@ -1731,6 +1731,119 @@
   }
 }
 
+// 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=rep1=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|  64 | https://www.7-cpu.com/cpu/Haswell.html   |
+// | Boadwell   |  64 | https://www.7-cpu.com/cpu/Broadwell.html |
+// | Skylake (including skylake-avx512) |  64 | https://www.nas.nasa.gov/hecc/support/kb/skylake-processors_550.html "Cache Hierarchy"   |
+// | Cascade Lake   |  64 | https://www.nas.nasa.gov/hecc/support/kb/cascade-lake-processors_579.html "Cache Hierarchy"  |
+// | Skylake  

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Zoe Carver via Phabricator via cfe-commits]

zoecarver added a comment.

@lebedev.ri my misunderstanding. I'll commit :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Like i said in previous comments, this can be moved later, that is not a 
blocker.
Ship it first :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Zoe Carver via Phabricator via cfe-commits]

zoecarver added a comment.

I don't know enough about clang and llvm to know what is both possible and 
preferable. @craig.topper @lebedev.ri what is the best course of action here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Zoe Carver via Phabricator via cfe-commits]

zoecarver added a comment.

@craig.topper that would mean that it couldn't be constexpr though, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

In D74918#1937291 , @lebedev.ri wrote:

> In D74918#1937237 , @zoecarver wrote:
>
> > @lebedev.ri Where should I put this? Could you maybe point me to another 
> > LLVM target API that clang uses that I could base this off? Maybe it should 
> > go inside the triple or just be a static function?
>
>
> I would have guessed `TargetLoweringInfo`/`TargetTransformInfo`, but i'm not 
> sure if we can access it here without violating layering.


I don't think can be done unless the proposed builtin for this lowered to an 
llvm intrinsic and was handled completely in llvm CodeGen.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D74918#1937237 , @zoecarver wrote:

> @lebedev.ri Where should I put this? Could you maybe point me to another LLVM 
> target API that clang uses that I could base this off? Maybe it should go 
> inside the triple or just be a static function?


I would have guessed `TargetLoweringInfo`/`TargetTransformInfo`, but i'm not 
sure if we can access it here without violating layering.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Zoe Carver via Phabricator via cfe-commits]

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

- Rebase off master
- For i386, return 16


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918

Files:
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/Targets/X86.cpp
  clang/lib/Basic/Targets/X86.h

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 ) const override;
 
+  Optional getCPUCacheLineSize() const override;
+
   bool validateAsmConstraint(const char *,
  TargetInfo::ConstraintInfo ) const override;
 
Index: clang/lib/Basic/Targets/X86.cpp
===
--- clang/lib/Basic/Targets/X86.cpp
+++ clang/lib/Basic/Targets/X86.cpp
@@ -1731,6 +1731,119 @@
   }
 }
 
+// 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=rep1=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|  64 | https://www.7-cpu.com/cpu/Haswell.html   |
+// | Boadwell   |  64 | https://www.7-cpu.com/cpu/Broadwell.html |
+// | Skylake (including skylake-avx512) |  64 | https://www.nas.nasa.gov/hecc/support/kb/skylake-processors_550.html "Cache Hierarchy"   |
+// | Cascade Lake   |  64 | https://www.nas.nasa.gov/hecc/support/kb/cascade-lake-processors_579.html "Cache Hierarchy"  |
+// | Skylake|  64 | https://en.wikichip.org/wiki/intel/microarchitectures/kaby_lake "Memory Hierarchy" 

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Zoe Carver via Phabricator via cfe-commits]

zoecarver added a comment.

@lebedev.ri Where should I put this? Could you maybe point me to another LLVM 
target API that clang uses that I could base this off? Maybe it should go 
inside the triple or just be a static function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Zoe Carver via Phabricator via cfe-commits]

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



Comment at: clang/lib/Basic/Targets/X86.cpp:1786
+// i386
+case CK_i386:
+// Netburst

craig.topper wrote:
> zoecarver wrote:
> > craig.topper wrote:
> > > I found the documentation for the 82385 cache controller chip for the 
> > > 386. It's a bit weird. The tags for the cache are based on 16 byte lines, 
> > > but there are valid bits for every 2 bytes within that line. So its 
> > > possible for only a portion of a line to be valid.
> > Interesting. Does this mean that we should be returning a different value?
> I think we should make 386 return 16.
Alright. I'll update it. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.



Comment at: clang/lib/Basic/Targets/X86.cpp:1786
+// i386
+case CK_i386:
+// Netburst

zoecarver wrote:
> craig.topper wrote:
> > I found the documentation for the 82385 cache controller chip for the 386. 
> > It's a bit weird. The tags for the cache are based on 16 byte lines, but 
> > there are valid bits for every 2 bytes within that line. So its possible 
> > for only a portion of a line to be valid.
> Interesting. Does this mean that we should be returning a different value?
I think we should make 386 return 16.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Zoe Carver via Phabricator via cfe-commits]

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

Fix based on review:

- update comments
- return 64 for Core2


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918

Files:
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/Targets/X86.cpp


Index: clang/lib/Basic/Targets/X86.cpp
===
--- clang/lib/Basic/Targets/X86.cpp
+++ clang/lib/Basic/Targets/X86.cpp
@@ -1745,7 +1745,7 @@
 // | 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|  64 | 
https://www.7-cpu.com/cpu/Haswell.html  
 |
-// | Bradwell   |  64 | 
https://www.7-cpu.com/cpu/Broadwell.html
 |
+// | Boadwell   |  64 | 
https://www.7-cpu.com/cpu/Broadwell.html
 |
 // | Skylake (including skylake-avx512) |  64 | 
https://www.nas.nasa.gov/hecc/support/kb/skylake-processors_550.html "Cache 
Hierarchy"  
 |
 // | Cascade Lake   |  64 | 
https://www.nas.nasa.gov/hecc/support/kb/cascade-lake-processors_579.html 
"Cache Hierarchy"   
   |
 // | Skylake|  64 | 
https://en.wikichip.org/wiki/intel/microarchitectures/kaby_lake "Memory 
Hierarchy"  
 |
@@ -1833,11 +1833,11 @@
 case CK_x86_64:
 case CK_Yonah:
 case CK_Penryn:
+case CK_Core2:
   return 64;
 
 // The following currently have unknown cache line sizes (but they are 
probably all 64):
 // Core
-case CK_Core2:
 case CK_Generic:
   return None;
   }
Index: clang/include/clang/Basic/TargetInfo.h
===
--- clang/include/clang/Basic/TargetInfo.h
+++ clang/include/clang/Basic/TargetInfo.h
@@ -1207,7 +1207,7 @@
   }
 
   // Get the cache line size of a given cpu. This method switches over
-  // the given cpu and returns `0` if the CPU is not found.
+  // the given cpu and returns "None" if the CPU is not found.
   virtual Optional getCPUCacheLineSize() const { return None; }
 
   // Returns maximal number of args passed in registers.


Index: clang/lib/Basic/Targets/X86.cpp
===
--- clang/lib/Basic/Targets/X86.cpp
+++ clang/lib/Basic/Targets/X86.cpp
@@ -1745,7 +1745,7 @@
 // | 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|  64 | https://www.7-cpu.com/cpu/Haswell.html   |
-// | Bradwell   |  64 | https://www.7-cpu.com/cpu/Broadwell.html |
+// | Boadwell   |  64 | https://www.7-cpu.com/cpu/Broadwell.html |
 // | Skylake (including skylake-avx512) |  64 | https://www.nas.nasa.gov/hecc/support/kb/skylake-processors_550.html "Cache Hierarchy"   |
 // | Cascade Lake   |  64 | https://www.nas.nasa.gov/hecc/support/kb/cascade-lake-processors_579.html "Cache Hierarchy"

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Zoe Carver via Phabricator via cfe-commits]

zoecarver marked 2 inline comments as done.
zoecarver added a comment.

I've made the suggested changes. Is there a consensus that this should be moved 
to LLVM? I think it's fairly clang-specific (given the use case). We can also 
always move it to LLVM if the need arises in the future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Zoe Carver via Phabricator via cfe-commits]

zoecarver added a comment.

In D76525  I've added a builtin that uses this 
API. We can use that builtin in libc++.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D74918#1931488 , @zoecarver wrote:

> @lebedev.ri LLVM may be better, I'm not sure. If you feel strongly I can move 
> it.


I do think that it makes much more sense somewhere closer to the backend

> @jyknight I'm planning on adding a builtin that uses this method. I can put 
> the patch up for that first if you would like.

but that being said, currently nothing needs that and with all those concerns 
about exposing this info,
i'd hold off moving it until it is settled whether it is needed (and not a dead 
code) in the first place :/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Zoe Carver via Phabricator via cfe-commits]

zoecarver marked 3 inline comments as done.
zoecarver added a comment.

@lebedev.ri LLVM may be better, I'm not sure. If you feel strongly I can move 
it.

@jyknight I'm planning on adding a builtin that uses this method. I can put the 
patch up for that first if you would like.




Comment at: clang/include/clang/Basic/TargetInfo.h:1192
+  // Get the cache line size of a given cpu. This method switches over
+  // the given cpu and returns `0` if the CPU is not found.
+  virtual Optional getCPUCacheLineSize() const { return None; }

lebedev.ri wrote:
> Comment is no longer valid - returns `None` instead.
> Also, might it be worth explicitly calling out that there is zero guarantees 
> of stability of the returned values?
Good catch. I think we should try to have as much stability as possible but I 
can add a note that these values may change. 



Comment at: clang/lib/Basic/Targets/X86.cpp:1786
+// i386
+case CK_i386:
+// Netburst

craig.topper wrote:
> I found the documentation for the 82385 cache controller chip for the 386. 
> It's a bit weird. The tags for the cache are based on 16 byte lines, but 
> there are valid bits for every 2 bytes within that line. So its possible for 
> only a portion of a line to be valid.
Interesting. Does this mean that we should be returning a different value?



Comment at: clang/lib/Basic/Targets/X86.cpp:1840
+// Core
+case CK_Core2:
+case CK_Generic:

craig.topper wrote:
> If Yonah and Penryn are 64, then Core2 should be as well. It's the generation 
> between them.
I'll update it. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

This patch as it stands is harmless, since as it only defines an internal 
interface, which is unused. So in that sense, it's perfectly fine to commit 
even with the remaining unresolved questions about the correct values to 
return. However, unless we're going to actually use it, adding this code to 
clang is not useful, for the same reason of it only defining an internal 
interface which is unused.

Ultimately, I don't see a reason to commit this, until/unless we are going to 
commit code in Clang using it (which I continue to believe we should not do). 
So I'd say leaving this in a pending state, until a use is going to be 
committed immediately afterwards, seems best.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

This seems misplaced - why is this in clang and not LLVM?




Comment at: clang/include/clang/Basic/TargetInfo.h:1192
+  // Get the cache line size of a given cpu. This method switches over
+  // the given cpu and returns `0` if the CPU is not found.
+  virtual Optional getCPUCacheLineSize() const { return None; }

Comment is no longer valid - returns `None` instead.
Also, might it be worth explicitly calling out that there is zero guarantees of 
stability of the returned values?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Zoe Carver via Phabricator via cfe-commits]

zoecarver added a comment.

@kristof.beyls I'll try to bring that up in my libc++ patch, thanks.

I'm planning on committing this today unless anyone still has concerns. 
@jyknight is this path OK with you?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added a comment.

In D74918#1923151 , @zoecarver wrote:

> There are a lot of different ways we could implement the feature. We may want 
> to only enable it when `-march=native`, or maybe only in the unstable ABI, 
> and maybe we want to support aligned pairs on some architectures. I think 
> that's an important discussion to have but I'm not sure _this_ patch is the 
> best place to have that discussion.
>
> Even if we don't use this patch in the implementation I think it would still 
> be a good utility to have. Here's what I suggest: I commit this, create 
> another patch to add a builtin that exposes this API, and then open a libc++ 
> patch with a _possible_ implementation. In that patch, we can discuss how we 
> should actually implement the feature and after we have a consensus I can do 
> the work to implement it. Any objections to that plan?


Discussing the implementation strategy for 
std::hardware_{constructive,destructive}_interference_size on a libc++ thread 
rather than here makes sense.
I'm afraid I don't have a good view on all the ways the API and associated 
intrinsic proposed here will or could be used in practice.
My only thought on it is that we cannot guarantee that different versions of 
LLVM will keep on reporting the same number, even for identical targets.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Zoe Carver via Phabricator via cfe-commits]

zoecarver added a comment.
Herald added a subscriber: danielkiss.

There are a lot of different ways we could implement the feature. We may want 
to only enable it when `-march=native`, or maybe only in the unstable ABI, and 
maybe we want to support aligned pairs on some architectures. I think that's an 
important discussion to have but I'm not sure _this_ patch is the best place to 
have that discussion.

Even if we don't use this patch in the implementation I think it would still be 
a good utility to have. Here's what I suggest: I commit this, create another 
patch to add a builtin that exposes this API, and then open a libc++ patch with 
a _possible_ implementation. In that patch, we can discuss how we should 
actually implement the feature and after we have a consensus I can do the work 
to implement it. Any objections to that plan?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added a comment.

In D74918#1898191 , @__simt__ wrote:

> In D74918#1897636 , @kristof.beyls 
> wrote:
>
> > If these values are part of the C++ target platform ABI, it seems to me the 
> > values for std::hardware_{constructive,destructive}_interference_size 
> > should be set by whoever has the authority to decide C++ platform ABI for 
> > specific platforms.
> >  Assuming my thought in the previous sentence is correct; discussions on 
> > which values to chose for 
> > std::hardware_{constructive,destructive}_interference_size should happen in 
> > whichever forums decide C++ platform ABI for the various platforms? (Maybe 
> > for some platforms that forum might be clang-related fora like 
> > reviews.llvm.org, but probably not for all platforms).
> >  With my (probably limited) understanding of the requirements, it seems 
> > like deriving std::hardware_{constructive,destructive}_interference_size 
> > from actual cache line size on a specific micro-architecture doesn't seem 
> > to be the right approach?
>
>
> They will be in the library ABI, meaning the libc++ ABI.
>
> It's valid for libstdc++ and libc++ to have different values there. If we 
> wish, we could try for an alignment (no pun intended) on these values, but 
> even then that's just between these two libraries.
>
> Which is good and encouraging, because I don't know what forum we would have 
> to go to.


I see.
So IIUC, this is library C++ ABI, to be defined by the C++ library. Since no 2 
C++ libraries can co-exist in a single application, there is no need for 
different C++ libraries to agree?

I think there is still an issue then with getting the values of 
std::hardware_{constructive,destructive}_interference_size in the library 
implementation derived from compiler builtins.
There are quite a few systems where clang supports targeting a different C++ 
library than libc++, e.g. libstdc++ on linux or  (IIUC) the MSVC C++ library on 
Windows.
If these C++ libraries implement 
std::hardware_{constructive,destructive}_interference_size based on a value 
returned by a compiler builtin, and the different compilers that are used with 
these libraries return different values for such a builtin, then the library 
C++ ABI here will be dependent on which compiler used?
Doesn't this indicate that either:

- std::hardware_{constructive,destructive}_interference_size should not be 
implemented using a compiler builtin, or
- all compilers must return the same value for the builtin; for all targets 
they support.

Overall, that makes me doubt that using a compiler builtin to implement 
std::hardware_{constructive,destructive}_interference_size is the right 
direction.
If the functionality in this patch does not need to support implementing 
std::hardware_{constructive,destructive}_interference_size, then the design 
constraints for this patch change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Olivier Giroux via Phabricator via cfe-commits]

__simt__ added a comment.

In D74918#1897636 , @kristof.beyls 
wrote:

> If these values are part of the C++ target platform ABI, it seems to me the 
> values for std::hardware_{constructive,destructive}_interference_size should 
> be set by whoever has the authority to decide C++ platform ABI for specific 
> platforms.
>  Assuming my thought in the previous sentence is correct; discussions on 
> which values to chose for 
> std::hardware_{constructive,destructive}_interference_size should happen in 
> whichever forums decide C++ platform ABI for the various platforms? (Maybe 
> for some platforms that forum might be clang-related fora like 
> reviews.llvm.org, but probably not for all platforms).
>  With my (probably limited) understanding of the requirements, it seems like 
> deriving std::hardware_{constructive,destructive}_interference_size from 
> actual cache line size on a specific micro-architecture doesn't seem to be 
> the right approach?


They will be in the library ABI, meaning the libc++ ABI.

It's valid for libstdc++ and libc++ to have different values there. If we wish, 
we could try for an alignment (no pun intended) on these values, but even then 
that's just between these two libraries.

Which is good and encouraging, because I don't know what forum we would have to 
go to.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added a comment.

In D74918#1896930 , @__simt__ wrote:

> (//I assume I'm not seeing a code review being used to veto a C++ Standard 
> feature, but actually the other points are the reason for the red flag.//)
>
> I can see a desire for hyper-precise definitions to achieve the best possible 
> performance, but we really need a healthy does of conservatism here.
>
> The C++ values can't change as a result of selecting between 
> microarchitecture variations that are supposed to link, it takes an ABI break 
> to change these.


If these values are part of the C++ target platform ABI, it seems to me the 
values for std::hardware_{constructive,destructive}_interference_size should be 
set by whoever has the authority to decide C++ platform ABI for specific 
platforms.
Assuming my thought in the previous sentence is correct; discussions on which 
values to chose for std::hardware_{constructive,destructive}_interference_size 
should happen in whichever forums decide C++ platform ABI for the various 
platforms? (Maybe for some platforms that forum might be clang-related fora 
like reviews.llvm.org, but probably not for all platforms).
With my (probably limited) understanding of the requirements, it seems like 
deriving std::hardware_{constructive,destructive}_interference_size from actual 
cache line size on a specific micro-architecture doesn't seem to be the right 
approach?

> We specified two numbers here so we could conservatively set them (e.g. 
> constructive: smallest; destructive: largest) if we want, but then they are 
> fixed.
> 
> I think there's just two plausible answers for x86_64:
> 
> 1. constructive=64, destructive=64 (the only plausible answer for X86 classic 
> edition)
> 2. constructive=64, destructive=128 (reserve some room for line-pair designs)
> 
>   Recapping: precision is nice but we need to fix this in the ABI so ultimate 
> precision isn't required nor desirable; we should choose one of these pairs 
> (or debate if another pair is viable that I'm not thinking of); then we 
> should ship C++17.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Zoe Carver via Phabricator via cfe-commits]

zoecarver added a comment.

Here's what I think we should do: continue to have this method just return the 
cache line size. Then have another method that returns `true` or `false` for 
whether the given architecture supports aligned pairs of cache lines then, 
users of this (either in clang or libc++) can decide what they want to do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Olivier Giroux via Phabricator via cfe-commits]

__simt__ added a comment.

(//I assume I'm not seeing a code review being used to veto a C++ Standard 
feature, but actually the other points are the reason for the red flag.//)

I can see a desire for hyper-precise definitions to achieve the best possible 
performance, but we really need a healthy does of conservatism here.

The C++ values can't change as a result of selecting between microarchitecture 
variations that are supposed to link, it takes an ABI break to change these.

We specified two numbers here so we could conservatively set them (e.g. 
constructive: smallest; destructive: largest) if we want, but then they are 
fixed.

I think there's just two plausible answers for x86_64:

1. constructive=64, destructive=64 (the only plausible answer for X86 classic 
edition)
2. constructive=64, destructive=128 (reserve some room for line-pair designs)

Recapping: precision is nice but we need to fix this in the ABI so ultimate 
precision isn't required nor desirable; we should choose one of these pairs (or 
debate if another pair is viable that I'm not thinking of); then we should ship 
C++17.




Comment at: clang/lib/Basic/Targets/X86.cpp:1748
+// | Haswell|  64 | 
https://www.7-cpu.com/cpu/Haswell.html  
 |
+// | Bradwell   |  64 | 
https://www.7-cpu.com/cpu/Broadwell.html
 |
+// | Skylake (including skylake-avx512) |  64 | 
https://www.nas.nasa.gov/hecc/support/kb/skylake-processors_550.html "Cache 
Hierarchy"  
 |

Broadwell.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

Re: [PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Eric Christopher via cfe-commits]

I'm agreed with James on all points fwiw.

On Thu, Feb 27, 2020, 2:00 PM James Y Knight via Phabricator <
revi...@reviews.llvm.org> wrote:

> jyknight requested changes to this revision.
> jyknight added a comment.
> This revision now requires changes to proceed.
>
> In D74918#1885869 , @zoecarver
> wrote:
>
> > @jyknight It would probably be best if we could account for CPUs who
> like to use aligned pairs when getting a cache line. It's probably also
> important that we don't change the value `getCPUCacheLineSize` returns, so,
> if we are going to account for that, we should probably update
> `getCPUCacheLineSize ` before this patch lands.
>
>
> It would be extremely unfortunate to go to all the trouble of attempting
> to return accurate results from the P0154 interfaces, and then to return an
> answer insufficient to actually achieve the performance benefit it's
> intended for, and then be unable to fix it due to ABI concerns. So, yes, I
> do believe that this issue must be decided.
>
> Additionally, my opinion here has really not changed from a couple of
> years ago. I continue to believe it was a mistake to standardize these
> constexpr values, and that absolutely the best course of action would be to
> continue to decline to implement this part of the standard, forever. (And
> that GCC should similarly also continue to decline to implement it).
>
> That said, the list of cacheline sizes collected in this review is still
> useful in any case, since it needs to be copied into LLVM's X86Subtarget to
> implement the backend getCacheLineSize function.
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D74918/new/
>
> https://reviews.llvm.org/D74918
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[James Y Knight via Phabricator via cfe-commits]

jyknight requested changes to this revision.
jyknight added a comment.
This revision now requires changes to proceed.

In D74918#1885869 , @zoecarver wrote:

> @jyknight It would probably be best if we could account for CPUs who like to 
> use aligned pairs when getting a cache line. It's probably also important 
> that we don't change the value `getCPUCacheLineSize` returns, so, if we are 
> going to account for that, we should probably update `getCPUCacheLineSize ` 
> before this patch lands.


It would be extremely unfortunate to go to all the trouble of attempting to 
return accurate results from the P0154 interfaces, and then to return an answer 
insufficient to actually achieve the performance benefit it's intended for, and 
then be unable to fix it due to ABI concerns. So, yes, I do believe that this 
issue must be decided.

Additionally, my opinion here has really not changed from a couple of years 
ago. I continue to believe it was a mistake to standardize these constexpr 
values, and that absolutely the best course of action would be to continue to 
decline to implement this part of the standard, forever. (And that GCC should 
similarly also continue to decline to implement it).

That said, the list of cacheline sizes collected in this review is still useful 
in any case, since it needs to be copied into LLVM's X86Subtarget to implement 
the backend getCacheLineSize function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.



Comment at: clang/lib/Basic/Targets/X86.cpp:1786
+// i386
+case CK_i386:
+// Netburst

I found the documentation for the 82385 cache controller chip for the 386. It's 
a bit weird. The tags for the cache are based on 16 byte lines, but there are 
valid bits for every 2 bytes within that line. So its possible for only a 
portion of a line to be valid.



Comment at: clang/lib/Basic/Targets/X86.cpp:1840
+// Core
+case CK_Core2:
+case CK_Generic:

If Yonah and Penryn are 64, then Core2 should be as well. It's the generation 
between them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Zoe Carver via Phabricator via cfe-commits]

zoecarver added a comment.

Friendly ping. Any other comments on this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Zoe Carver via Phabricator via cfe-commits]

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



Comment at: clang/lib/Basic/Targets/X86.cpp:1834
+case CK_Tigerlake:
+case CK_Lakemont:
+

craig.topper wrote:
> zoecarver wrote:
> > craig.topper wrote:
> > > zoecarver wrote:
> > > > craig.topper wrote:
> > > > > I think Lakemont is 16 bytes. Assuming I'm interpretting the CLFLUSH 
> > > > > line size from this CPUID dump correctly 
> > > > > https://github.com/InstLatx64/InstLatx64/blob/master/GenuineIntel/GenuineIntel590_Lakemont_CPUID2.txt
> > > > >  
> > > > I may very well be interpreting this incorrectly but I think that the 
> > > > cache line sizes are at `eax = 0x8005` and `eax = 0x8006`. 
> > > > However, all the registers at those codes are zero. If I instead look 
> > > > at `eax = 0x0001` (which I think is incorrect), `ecx = 00010200` 
> > > > which would be 128 bytes (maybe this is where the 16 came from, if they 
> > > > were bits instead?). How were you interpreting it?
> > > Interpreted bits 15:8 of the ecx = 00010200 to be the clflush 
> > > information. The spec for cpuid says to multiply by 8 to get cache line 
> > > size. So 15:8 is 2, multiply by 8 is 16 bytes.
> > > 
> > > I think Lakemont is based on the 486 architecture so that seems possible.
> > I see. That looks correct. I'll update it.
> > 
> > Still, I do find it strange that all registers at `eax = 0x8006` are 
> > zeroed out. Using the `cpuid` instruction on my cpu I get the following:
> > ```
> > CPUID 0: 13 71 110 105
> > CPUID 1: 97 0 191 255
> > CPUID 2: 1 255 0 0
> > CPUID 8000: 8 0 0 0
> > CPUID 8001: 0 0 33 0
> > CPUID 8002: 73 108 32 101
> > CPUID 8003: 41 45 48 67
> > CPUID 8004: 64 50 122 0
> > CPUID 8005: 0 0 0 0
> > CPUID 8006: 0 0 64 0
> > CPUID 8007: 0 0 0 0
> > CPUID 8008: 39 0 0 0
> > ```
> > I know my L2 cache line size is 64 and the L2 cache line size should be 
> > `ecx` at `eax = 0x8006` which would be `64`. But, when I read the dump 
> > you linked to, it doesn't look like there's any data at `0x8006`.
> > 
> > Anyway, as I said above, I'll look past this and update it based on the 
> > clflush information. That seems valid. 
> Lakemont is a modernized version of a 486 which probably had none of the 
> 8000 leafs originally. My speculation is that they really wanted to 
> support leaf 8008 so they just put zeros in the rest rather than add a 
> lot of microcode.
Alrighty, that makes sense. Thanks :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.



Comment at: clang/lib/Basic/Targets/X86.cpp:1834
+case CK_Tigerlake:
+case CK_Lakemont:
+

zoecarver wrote:
> craig.topper wrote:
> > zoecarver wrote:
> > > craig.topper wrote:
> > > > I think Lakemont is 16 bytes. Assuming I'm interpretting the CLFLUSH 
> > > > line size from this CPUID dump correctly 
> > > > https://github.com/InstLatx64/InstLatx64/blob/master/GenuineIntel/GenuineIntel590_Lakemont_CPUID2.txt
> > > >  
> > > I may very well be interpreting this incorrectly but I think that the 
> > > cache line sizes are at `eax = 0x8005` and `eax = 0x8006`. 
> > > However, all the registers at those codes are zero. If I instead look at 
> > > `eax = 0x0001` (which I think is incorrect), `ecx = 00010200` which 
> > > would be 128 bytes (maybe this is where the 16 came from, if they were 
> > > bits instead?). How were you interpreting it?
> > Interpreted bits 15:8 of the ecx = 00010200 to be the clflush information. 
> > The spec for cpuid says to multiply by 8 to get cache line size. So 15:8 is 
> > 2, multiply by 8 is 16 bytes.
> > 
> > I think Lakemont is based on the 486 architecture so that seems possible.
> I see. That looks correct. I'll update it.
> 
> Still, I do find it strange that all registers at `eax = 0x8006` are 
> zeroed out. Using the `cpuid` instruction on my cpu I get the following:
> ```
> CPUID 0: 13 71 110 105
> CPUID 1: 97 0 191 255
> CPUID 2: 1 255 0 0
> CPUID 8000: 8 0 0 0
> CPUID 8001: 0 0 33 0
> CPUID 8002: 73 108 32 101
> CPUID 8003: 41 45 48 67
> CPUID 8004: 64 50 122 0
> CPUID 8005: 0 0 0 0
> CPUID 8006: 0 0 64 0
> CPUID 8007: 0 0 0 0
> CPUID 8008: 39 0 0 0
> ```
> I know my L2 cache line size is 64 and the L2 cache line size should be `ecx` 
> at `eax = 0x8006` which would be `64`. But, when I read the dump you 
> linked to, it doesn't look like there's any data at `0x8006`.
> 
> Anyway, as I said above, I'll look past this and update it based on the 
> clflush information. That seems valid. 
Lakemont is a modernized version of a 486 which probably had none of the 
8000 leafs originally. My speculation is that they really wanted to support 
leaf 8008 so they just put zeros in the rest rather than add a lot of 
microcode.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Zoe Carver via Phabricator via cfe-commits]

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

- Update getCPUCacheLineSize to return optional


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918

Files:
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/Targets/X86.cpp
  clang/lib/Basic/Targets/X86.h

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 ) const override;
 
+  Optional getCPUCacheLineSize() const override;
+
   bool validateAsmConstraint(const char *,
  TargetInfo::ConstraintInfo ) 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=rep1=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|  64 | https://www.7-cpu.com/cpu/Haswell.html   |
+// | Bradwell   |  64 | https://www.7-cpu.com/cpu/Broadwell.html |
+// | Skylake (including skylake-avx512) |  64 | https://www.nas.nasa.gov/hecc/support/kb/skylake-processors_550.html "Cache Hierarchy"   |
+// | Cascade Lake   |  64 | https://www.nas.nasa.gov/hecc/support/kb/cascade-lake-processors_579.html "Cache Hierarchy"  |
+// | Skylake|  64 | https://en.wikichip.org/wiki/intel/microarchitectures/kaby_lake "Memory Hierarchy"   

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Zoe Carver via Phabricator via cfe-commits]

zoecarver updated this revision to Diff 245896.
zoecarver marked 6 inline comments as done.
zoecarver added a comment.

- Update values returned by getCPUCacheLineSize based on review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918

Files:
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/Targets/X86.cpp
  clang/lib/Basic/Targets/X86.h

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 ) const override;
 
+  unsigned getCPUCacheLineSize() const override;
+
   bool validateAsmConstraint(const char *,
  TargetInfo::ConstraintInfo ) 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=rep1=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|  64 | https://www.7-cpu.com/cpu/Haswell.html   |
+// | Bradwell   |  64 | https://www.7-cpu.com/cpu/Broadwell.html |
+// | Skylake (including skylake-avx512) |  64 | https://www.nas.nasa.gov/hecc/support/kb/skylake-processors_550.html "Cache Hierarchy"   |
+// | Cascade Lake   |  64 | https://www.nas.nasa.gov/hecc/support/kb/cascade-lake-processors_579.html "Cache Hierarchy"  |
+// | Skylake|  64 | 

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Zoe Carver via Phabricator via cfe-commits]

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



Comment at: clang/lib/Basic/Targets/X86.cpp:1834
+case CK_Tigerlake:
+case CK_Lakemont:
+

craig.topper wrote:
> zoecarver wrote:
> > craig.topper wrote:
> > > I think Lakemont is 16 bytes. Assuming I'm interpretting the CLFLUSH line 
> > > size from this CPUID dump correctly 
> > > https://github.com/InstLatx64/InstLatx64/blob/master/GenuineIntel/GenuineIntel590_Lakemont_CPUID2.txt
> > >  
> > I may very well be interpreting this incorrectly but I think that the cache 
> > line sizes are at `eax = 0x8005` and `eax = 0x8006`. However, all 
> > the registers at those codes are zero. If I instead look at `eax = 
> > 0x0001` (which I think is incorrect), `ecx = 00010200` which would be 
> > 128 bytes (maybe this is where the 16 came from, if they were bits 
> > instead?). How were you interpreting it?
> Interpreted bits 15:8 of the ecx = 00010200 to be the clflush information. 
> The spec for cpuid says to multiply by 8 to get cache line size. So 15:8 is 
> 2, multiply by 8 is 16 bytes.
> 
> I think Lakemont is based on the 486 architecture so that seems possible.
I see. That looks correct. I'll update it.

Still, I do find it strange that all registers at `eax = 0x8006` are zeroed 
out. Using the `cpuid` instruction on my cpu I get the following:
```
CPUID 0: 13 71 110 105
CPUID 1: 97 0 191 255
CPUID 2: 1 255 0 0
CPUID 8000: 8 0 0 0
CPUID 8001: 0 0 33 0
CPUID 8002: 73 108 32 101
CPUID 8003: 41 45 48 67
CPUID 8004: 64 50 122 0
CPUID 8005: 0 0 0 0
CPUID 8006: 0 0 64 0
CPUID 8007: 0 0 0 0
CPUID 8008: 39 0 0 0
```
I know my L2 cache line size is 64 and the L2 cache line size should be `ecx` 
at `eax = 0x8006` which would be `64`. But, when I read the dump you linked 
to, it doesn't look like there's any data at `0x8006`.

Anyway, as I said above, I'll look past this and update it based on the clflush 
information. That seems valid. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.



Comment at: clang/lib/Basic/Targets/X86.cpp:1738
+// 
++-+--+
+// | i386   |  64 | 
https://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-optimization-manual.pdf
  |
+// | i486   |  64 | "four 
doublewords" 
https://en.wikichip.org/w/images/d/d3/i486_MICROPROCESSOR_HARDWARE_REFERENCE_MANUAL_%281990%29.pdf
|

zoecarver wrote:
> craig.topper wrote:
> > I'd be surprised if 386 is larger than 486 or 586.
> Yes, it does seem surprising but this other source corroborates it 
> https://osxbook.com/blog/2009/03/02/retrieving-x86-processor-information/
-march=i386 there seem just seems to be a compiler switch. The 386 CPU didn't 
have CPUID, that was added on the 486. The dump there seems to be a dump from 
Nehalem. They just compiled the code targeting a really old CPU for some reason.

Thinking about it more, the 386 is a bit weird because the cache wasn't part of 
the CPU, it was on the motherboard and not always present. Not sure what that 
means for cache line size. I have an ancient 386 hardware manual in my cube at 
work, maybe I can find something in there.



Comment at: clang/lib/Basic/Targets/X86.cpp:1834
+case CK_Tigerlake:
+case CK_Lakemont:
+

zoecarver wrote:
> craig.topper wrote:
> > I think Lakemont is 16 bytes. Assuming I'm interpretting the CLFLUSH line 
> > size from this CPUID dump correctly 
> > https://github.com/InstLatx64/InstLatx64/blob/master/GenuineIntel/GenuineIntel590_Lakemont_CPUID2.txt
> >  
> I may very well be interpreting this incorrectly but I think that the cache 
> line sizes are at `eax = 0x8005` and `eax = 0x8006`. However, all the 
> registers at those codes are zero. If I instead look at `eax = 0x0001` 
> (which I think is incorrect), `ecx = 00010200` which would be 128 bytes 
> (maybe this is where the 16 came from, if they were bits instead?). How were 
> you interpreting it?
Interpreted bits 15:8 of the ecx = 00010200 to be the clflush information. The 
spec for cpuid says to multiply by 8 to get cache line size. So 15:8 is 2, 
multiply by 8 is 16 bytes.

I think Lakemont is based on the 486 architecture so that seems possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Zoe Carver via Phabricator via cfe-commits]

zoecarver marked 7 inline comments as done.
zoecarver added a comment.

To address the question, what will this be used for: this could be used for 
several things in the library and compiler, but the primary use will be to 
create a builtin that can be used to implement P0154 
.

@jyknight It would probably be best if we could account for CPUs who like to 
use aligned pairs when getting a cache line. It's probably also important that 
we don't change the value `getCPUCacheLineSize` returns, so, if we are going to 
account for that, we should probably update `getCPUCacheLineSize ` before this 
patch lands.




Comment at: clang/include/clang/Basic/TargetInfo.h:1193
+  // the given cpu and returns `0` if the CPU is not found.
+  virtual unsigned getCPUCacheLineSize() const { return 0; }
+

jfb wrote:
> Return an optional instead of using zero to mean "unknown"?
Good idea, will do.



Comment at: clang/lib/Basic/Targets/X86.cpp:1738
+// 
++-+--+
+// | i386   |  64 | 
https://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-optimization-manual.pdf
  |
+// | i486   |  64 | "four 
doublewords" 
https://en.wikichip.org/w/images/d/d3/i486_MICROPROCESSOR_HARDWARE_REFERENCE_MANUAL_%281990%29.pdf
|

craig.topper wrote:
> I'd be surprised if 386 is larger than 486 or 586.
Yes, it does seem surprising but this other source corroborates it 
https://osxbook.com/blog/2009/03/02/retrieving-x86-processor-information/



Comment at: clang/lib/Basic/Targets/X86.cpp:1739
+// | i386   |  64 | 
https://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-optimization-manual.pdf
  |
+// | i486   |  64 | "four 
doublewords" 
https://en.wikichip.org/w/images/d/d3/i486_MICROPROCESSOR_HARDWARE_REFERENCE_MANUAL_%281990%29.pdf
|
+// | i586/Pentium MMX   |  32 | 
https://www.7-cpu.com/cpu/P-MMX.html
 |

craig.topper wrote:
> doubleword is 32-bits on X86. So 4 double words is 16 bytes I think?
Yes, you're right! My bad. [[ 
http://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.126.4216=rep1=pdf
 | This PDF ]] (page 29) also agrees with 16 bytes. I'll update the table.



Comment at: clang/lib/Basic/Targets/X86.cpp:1833
+case CK_Cannonlake:
+case CK_Tigerlake:
+case CK_Lakemont:

craig.topper wrote:
> Nehalem, coooperlake, cannonlake and tigerlake are all 64.
Great, I'll update it. Thanks.



Comment at: clang/lib/Basic/Targets/X86.cpp:1834
+case CK_Tigerlake:
+case CK_Lakemont:
+

craig.topper wrote:
> I think Lakemont is 16 bytes. Assuming I'm interpretting the CLFLUSH line 
> size from this CPUID dump correctly 
> https://github.com/InstLatx64/InstLatx64/blob/master/GenuineIntel/GenuineIntel590_Lakemont_CPUID2.txt
>  
I may very well be interpreting this incorrectly but I think that the cache 
line sizes are at `eax = 0x8005` and `eax = 0x8006`. However, all the 
registers at those codes are zero. If I instead look at `eax = 0x0001` 
(which I think is incorrect), `ecx = 00010200` which would be 128 bytes (maybe 
this is where the 16 came from, if they were bits instead?). How were you 
interpreting it?



Comment at: clang/lib/Basic/Targets/X86.cpp:1840
+case CK_Generic:
+case CK_Penryn:
+  return 0;

jfb wrote:
> All of the above (except generic) should be 64.
Good to know, I'll update it :)



Comment at: clang/lib/Sema/SemaStmt.cpp:2817
+
+  unsigned targetCacheLineSize = Ctx.getTargetInfo().getCPUCacheLineSize();
+  if (!targetCacheLineSize)

efriedma wrote:
> "64" here is arbitrary; it's not actually supposed to be the cache line size 
> for any particular CPU, just something generally expensive.  I don't think 
> this warning should depend on -mcpu.
This will fall back to `64` as a default if the cache line size isn't known. I 
can revert this change if you want, though. It's not central to the patch. 


Repository:
  rG LLVM Github Monorepo

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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.



Comment at: clang/lib/Basic/Targets/X86.cpp:1834
+case CK_Tigerlake:
+case CK_Lakemont:
+

I think Lakemont is 16 bytes. Assuming I'm interpretting the CLFLUSH line size 
from this CPUID dump correctly 
https://github.com/InstLatx64/InstLatx64/blob/master/GenuineIntel/GenuineIntel590_Lakemont_CPUID2.txt
 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.



Comment at: clang/lib/Basic/Targets/X86.cpp:1738
+// 
++-+--+
+// | i386   |  64 | 
https://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-optimization-manual.pdf
  |
+// | i486   |  64 | "four 
doublewords" 
https://en.wikichip.org/w/images/d/d3/i486_MICROPROCESSOR_HARDWARE_REFERENCE_MANUAL_%281990%29.pdf
|

I'd be surprised if 386 is larger than 486 or 586.



Comment at: clang/lib/Basic/Targets/X86.cpp:1739
+// | i386   |  64 | 
https://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-optimization-manual.pdf
  |
+// | i486   |  64 | "four 
doublewords" 
https://en.wikichip.org/w/images/d/d3/i486_MICROPROCESSOR_HARDWARE_REFERENCE_MANUAL_%281990%29.pdf
|
+// | i586/Pentium MMX   |  32 | 
https://www.7-cpu.com/cpu/P-MMX.html
 |

doubleword is 32-bits on X86. So 4 double words is 16 bytes I think?



Comment at: clang/lib/Basic/Targets/X86.cpp:1833
+case CK_Cannonlake:
+case CK_Tigerlake:
+case CK_Lakemont:

Nehalem, coooperlake, cannonlake and tigerlake are all 64.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: clang/include/clang/Basic/TargetInfo.h:1193
+  // the given cpu and returns `0` if the CPU is not found.
+  virtual unsigned getCPUCacheLineSize() const { return 0; }
+

Return an optional instead of using zero to mean "unknown"?



Comment at: clang/lib/Basic/Targets/X86.cpp:1840
+case CK_Generic:
+case CK_Penryn:
+  return 0;

All of the above (except generic) should be 64.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

As I've mentioned before, depending on what this will be used for, "64" is not 
a _useful_ answer if you want to know how your memory accesses will behave on 
modern intel x86 CPUs, despite being the "correct" answer for cache-line size. 
But, modern intel CPUs fetch aligned-pairs of cache-lines together.  Therefore, 
you'll start to see cache interference effects as-if the cache-lines were 128 
bytes, rather than 64 bytes. (This may or may not also apply to AMD cpus, I 
haven't looked into it.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

Where are you planning to use this?




Comment at: clang/lib/Sema/SemaStmt.cpp:2817
+
+  unsigned targetCacheLineSize = Ctx.getTargetInfo().getCPUCacheLineSize();
+  if (!targetCacheLineSize)

"64" here is arbitrary; it's not actually supposed to be the cache line size 
for any particular CPU, just something generally expensive.  I don't think this 
warning should depend on -mcpu.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Zoe Carver via Phabricator via cfe-commits]

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

- Add AMD cache line sizes based on @lebedev.ri's comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918

Files:
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/Targets/X86.cpp
  clang/lib/Basic/Targets/X86.h
  clang/lib/Sema/SemaStmt.cpp

Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -2813,7 +2813,12 @@
   // diagnostic for these instances. 64 bytes is a common size of a cache line.
   // (The function `getTypeSize` returns the size in bits.)
   ASTContext  = SemaRef.Context;
-  if (Ctx.getTypeSize(VariableType) <= 64 * 8 &&
+
+  unsigned targetCacheLineSize = Ctx.getTargetInfo().getCPUCacheLineSize();
+  if (!targetCacheLineSize)
+targetCacheLineSize = 64;
+
+  if (Ctx.getTypeSize(VariableType) <= targetCacheLineSize * 8 &&
   (VariableType.isTriviallyCopyableType(Ctx) ||
hasTrivialABIAttr(VariableType)))
 return;
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 ) const override;
 
+  unsigned getCPUCacheLineSize() const override;
+
   bool validateAsmConstraint(const char *,
  TargetInfo::ConstraintInfo ) const override;
 
Index: clang/lib/Basic/Targets/X86.cpp
===
--- clang/lib/Basic/Targets/X86.cpp
+++ clang/lib/Basic/Targets/X86.cpp
@@ -1731,6 +1731,117 @@
   }
 }
 
+// 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   |  64 | "four doublewords" https://en.wikichip.org/w/images/d/d3/i486_MICROPROCESSOR_HARDWARE_REFERENCE_MANUAL_%281990%29.pdf|
+// | 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|  64 | https://www.7-cpu.com/cpu/Haswell.html   |
+// | Bradwell   |  64 | https://www.7-cpu.com/cpu/Broadwell.html  

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Zoe Carver via Phabricator via cfe-commits]

zoecarver added a comment.

@lebedev.ri thanks! I'll add those.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

No proofs, but for completeness, some amd numbers




Comment at: clang/lib/Basic/Targets/X86.cpp:1811-1814
+// K6
+case CK_K6:
+case CK_K6_2:
+case CK_K6_3:

K6 should have 32B cache line size



Comment at: clang/lib/Basic/Targets/X86.cpp:1815-1832
+// K7
+case CK_Athlon:
+case CK_AthlonXP:
+// K8
+case CK_K8:
+case CK_K8SSE3:
+case CK_AMDFAM10:

K7..ZnVer2 should have 64B



Comment at: clang/lib/Basic/Targets/X86.cpp:1833-1834
+case CK_ZNVER2:
+// Geode
+case CK_Geode:
+

32B for this one


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74918



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

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

[Zoe Carver via Phabricator via cfe-commits]

zoecarver created this revision.
Herald added subscribers: cfe-commits, kristof.beyls.
Herald added a project: clang.

This patch adds a virtual method `getCPUCacheLineSize()` to `TargetInfo`. 
Currently, I've only (partially) implemented the method in `X86TargetInfo`. 
It's extremely important that each CPU's cache line size correct (e.g., we 
can't just define it as `64` across the board) so, it has been a little slow 
getting to this point. There are still quite a few CPUs I haven't been able to 
find the cache line size of yet; for those, I'm returning zero so that the 
caller of this method can propagate an error. See the commented table above 
`X86TargetInfo::getCPUCacheLineSize` to check my sources for each CPU.

I'll work on the ARM CPUs next, but that will probably come later in a 
different patch.

Also, I updated the current uses of cache line sizes in the compiler to use 
this API when possible. The only one (that I could find) that I didn't update 
is in `TargetTransformInfo`. Updating that would require a more significant API 
change, which would be out of scope for this patch. It would be nice if that 
also used this API (to keep everything in one place), so I'll try to update 
that too at some point.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D74918

Files:
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/Targets/X86.cpp
  clang/lib/Basic/Targets/X86.h
  clang/lib/Sema/SemaStmt.cpp

Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -2813,7 +2813,12 @@
   // diagnostic for these instances. 64 bytes is a common size of a cache line.
   // (The function `getTypeSize` returns the size in bits.)
   ASTContext  = SemaRef.Context;
-  if (Ctx.getTypeSize(VariableType) <= 64 * 8 &&
+
+  unsigned targetCacheLineSize = Ctx.getTargetInfo().getCPUCacheLineSize();
+  if (!targetCacheLineSize)
+targetCacheLineSize = 64;
+
+  if (Ctx.getTypeSize(VariableType) <= targetCacheLineSize * 8 &&
   (VariableType.isTriviallyCopyableType(Ctx) ||
hasTrivialABIAttr(VariableType)))
 return;
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 ) const override;
 
+  unsigned getCPUCacheLineSize() const override;
+
   bool validateAsmConstraint(const char *,
  TargetInfo::ConstraintInfo ) const override;
 
Index: clang/lib/Basic/Targets/X86.cpp
===
--- clang/lib/Basic/Targets/X86.cpp
+++ clang/lib/Basic/Targets/X86.cpp
@@ -1731,6 +1731,117 @@
   }
 }
 
+// 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   |  64 | "four doublewords" https://en.wikichip.org/w/images/d/d3/i486_MICROPROCESSOR_HARDWARE_REFERENCE_MANUAL_%281990%29.pdf|
+// | 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   |