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

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

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

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

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

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

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

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/

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

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 ___

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

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

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

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

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

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:

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

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

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

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

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

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

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

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:

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

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.

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

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

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

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

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

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.

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

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

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

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. +

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

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

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

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

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

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

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

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

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

__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

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

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

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

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

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

__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

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

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

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

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

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

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

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

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

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

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: >

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

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.

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

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:

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

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/

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

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

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

craig.topper added inline comments. Comment at: clang/lib/Basic/Targets/X86.cpp:1738 +//

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

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

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

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

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

craig.topper added inline comments. Comment at: clang/lib/Basic/Targets/X86.cpp:1738 +//

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

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

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

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

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

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

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

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:

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

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

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

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

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

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