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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

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.

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.

Returning specifically to this patch, I see the following technical concerns:

1. The use a black list - instead of a white list - for deciding which instructions can be safely padded has already been shown to be problematic correctness wise.  This seems to ignore the possibility that an instruction might *already have* prefixes when deciding how many to insert, and it's not clear to me that all chosen prefixes are valid on *all* instructions.
2. The fact that *every single instruction* ends up in it's own fragment is a *huge* increase in memory usage.  This has been brought up repeatedly (see original thread), but I have seen *no* data provided to make the overhead concrete.  There are several ways to potentially mitigate this, but it doesn't appeared to have been assessed.
3. Stylistic points - such as defining an enum with names for prefixes - previously raised in review comments have not been addressed.
4. I see little discussion of what validation has been done on this patch.  The only comments I can find - https://reviews.llvm.org/D72225#1818149 - seems to indicate that fairly basic testing exposes functional issues.  That does not inspire confidence in the quality of the implementation.

Out of these, (4) and (2) are by far the most worrying.

Putting all this together, I don't think this patch should land at this time.


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

https://reviews.llvm.org/D72225