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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits