<div dir="auto">I'm agreed with James on all points fwiw. </div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Feb 27, 2020, 2:00 PM James Y Knight via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">jyknight requested changes to this revision.<br>
jyknight added a comment.<br>
This revision now requires changes to proceed.<br>
<br>
In D74918#1885869 <<a href="https://reviews.llvm.org/D74918#1885869" rel="noreferrer noreferrer" target="_blank">https://reviews.llvm.org/D74918#1885869</a>>, @zoecarver wrote:<br>
<br>
> @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.<br>
<br>
<br>
It would be extremely unfortunate to go to all the trouble of attempting to return accurate results from the P0154 interfaces, and then to return an answer insufficient to actually achieve the performance benefit it's intended for, and then be unable to fix it due to ABI concerns. So, yes, I do believe that this issue must be decided.<br>
<br>
Additionally, my opinion here has really not changed from a couple of years ago. I continue to believe it was a mistake to standardize these constexpr values, and that absolutely the best course of action would be to continue to decline to implement this part of the standard, forever. (And that GCC should similarly also continue to decline to implement it).<br>
<br>
That said, the list of cacheline sizes collected in this review is still useful in any case, since it needs to be copied into LLVM's X86Subtarget to implement the backend getCacheLineSize function.<br>
<br>
<br>
Repository:<br>
  rG LLVM Github Monorepo<br>
<br>
CHANGES SINCE LAST ACTION<br>
  <a href="https://reviews.llvm.org/D74918/new/" rel="noreferrer noreferrer" target="_blank">https://reviews.llvm.org/D74918/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D74918" rel="noreferrer noreferrer" target="_blank">https://reviews.llvm.org/D74918</a><br>
<br>
<br>
<br>
</blockquote></div>