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

[Olivier Giroux via Phabricator via cfe-commits]

__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 healthy does of conservatism here.

The C++ values can't change as a result of selecting between microarchitecture variations that are supposed to link, it takes an ABI break to change these.

We specified two numbers here so we could conservatively set them (e.g. constructive: smallest; destructive: largest) if we want, but then they are fixed.

I think there's just two plausible answers for x86_64:

1. constructive=64, destructive=64 (the only plausible answer for X86 classic edition)
2. constructive=64, destructive=128 (reserve some room for line-pair designs)

Recapping: precision is nice but we need to fix this in the ABI so ultimate precision isn't required nor desirable; we should choose one of these pairs (or debate if another pair is viable that I'm not thinking of); then we should ship C++17.



================
Comment at: clang/lib/Basic/Targets/X86.cpp:1748
+// | Haswell                            |                      64 | https://www.7-cpu.com/cpu/Haswell.html                                                                                                                       |
+// | Bradwell                           |                      64 | https://www.7-cpu.com/cpu/Broadwell.html                                                                                                                     |
+// | Skylake (including skylake-avx512) |                      64 | https://www.nas.nasa.gov/hecc/support/kb/skylake-processors_550.html "Cache Hierarchy"                                                                       |
----------------
Broadwell.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74918/new/

https://reviews.llvm.org/D74918