[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 <http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0154r1.html>.

@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&rep=rep1&type=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/GenuineIntel0000590_Lakemont_CPUID2.txt 
I may very well be interpreting this incorrectly but I think that the cache line sizes are at `eax = 0x80000005` and `eax = 0x80000006`. However, all the registers at those codes are zero. If I instead look at `eax = 0x00000001` (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/

https://reviews.llvm.org/D74918