[PATCH] D74918: Add method to TargetInfo to get CPU cache line size
Zoe Carver via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Feb 20 22:08:40 PST 2020
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
More information about the cfe-commits
mailing list