[PATCH] D71238: Align non-fused branches within 32-Byte boundary (basic case)

Fri Dec 13 17:25:34 PST 2019

andrew.w.kaylor added a comment.

I'm late jumping into the discussion of these patches and may not have seen all comments, so go easy on me if I say something that has already been covered.

> In D71238#1776751 <https://reviews.llvm.org/D71238#1776751>, @chandlerc wrote:
> 
>> I just want to mention here, that my comment on the original patch still stands and really needs to be addressed:
>>
>> We need to have detailed performance and binary size numbers for this kind of change. We should be looking at the test suite and SPEC for performance. I'd like to see before/after performance numbers on a system that is *not* using the microcode feature that addresses jCC (either a newer CPU, an older CPu, or an unpatched CPU). I'd also like to see before/after performance numbers on a system that *is* using the microcode feature to address the jCC issue. I'd suggest Clang itself, Firefox, and Chromium as binaries to show the binary size impact of this patch. Happy for others to suggest binaries to use to evaluate the binary size impact.
> 
> 
> In D71238#1777747 <https://reviews.llvm.org/D71238#1777747>, @reames wrote:
>  I strongly agree that Intel should share these numbers in detail.  So far, we've seen some summary shared on llvm-dev, but nothing in detail.

We ran into a very slow required internal process here. Annita tells me that we'll have data to share very soon, hopefully on Monday. I haven't seen the data myself, but I have a suspicion that it isn't going to be what you're looking for. I'm talking to people to see what I can do to establish a faster process for turning around data for incremental updates to these patches. More detailed numbers mean more scrutiny before we're allowed to share. I like the suggestion of using clang. A self build would give us code size and an execution time that I hope would be good enough for evaluating relative differences introduced by the patch. All interested reviewers should easily be able to repeat this test locally on whatever machine they like, and everyone could share their own data from whatever variations they were interested in trying.

>>> In D71238#1776751 <https://reviews.llvm.org/D71238#1776751>, @chandlerc wrote:
>> 
>> I continue to think that any patch we plan to land should have these numbers, and each incremental update to that patch should have the updated numbers. Otherwise, there is no way to do a good job reviewing the *impact* of these changes even after folks are satisfied with the *code* in these changes.
> 
> In D71238#1777747 <https://reviews.llvm.org/D71238#1777747>, @reames wrote:
>  I understand your concern, but I want to caution drawing too firm a line here.  We have a situation where review on a very important patch is stalled, and we need to unblock it.  This subseting was my attempt to do so, but I'm not in a position to be able to share data at the scale your arguing for.  If we can't work incrementally here, I suspect this effort is going to stall.

I agree with Philip here. Obviously, having data is critical to making decisions and knowing when we've reached an acceptable state. We just need to agree on where the threshold is for enough data to make a reasonable decision.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71238