[PATCH] D72225: Align branches within 32-Byte boundary(Prefix padding)

Thu Jan 16 00:35:25 PST 2020

annita.zhang added a comment.

In D72225#1820813 <https://reviews.llvm.org/D72225#1820813>, @reames wrote:

> I have serious reservations about the rush to land this patch.  I have expressed some of this privately, and other bits have been in previous review threads, but I want to put everything in one public place.
>
> I don't see a strong value in having this in the current LLVM release.  We have support for nop padding, and the delta between nop padding and prefix padding is minimal.  My personal take is that wrapping up all of the wiring to provide the clang driver option to enable padding (via whatever mechanism is available, for now nops) should take preference over making the padding code marginally better.

Regarding prefix padding vs. nop padding, we observed 0.3% improvement in SPECINT geomean, and 0.5% in SPECFP geomean. It's not that much in geomean. But we saw 1~2% improvement in specific SPEC benchmarks. We also observed cases in which nop padding doesn't mitigate the effect very well, but prefix padding does. That's the reason we intend to land the prefix padding into LLVM10. So users have alternative approaches to mitigate the JCC microcode update. They can choose whatever provides better performance. 
Anyway, I understand your concern and agree to hold the patch until it's pretty mature.

> I am also deeply uncomfortable that there does not appear to have been any meaningful progress on defining an assembler syntax for this feature.  I understand - and in fact pushed for - the urgency in getting a mitigation out, but that urgency is now gone.  We have a mitigation which closes most of the gap, we can now focus on ensuring that we're well positioned for the long term.

I saw you proposed an assembler syntax in https://reviews.llvm.org/D71315. So I suppose you will continue to drive it. We can co-work on it to finalize the syntax and implement it.

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

https://reviews.llvm.org/D72225