[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<unsigned> 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