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

[Kan Shengchen via Phabricator via llvm-commits]

skan added a comment.

In D72225#1818531 <https://reviews.llvm.org/D72225#1818531>, @MaskRay wrote:

> In D72225#1818510 <https://reviews.llvm.org/D72225#1818510>, @skan wrote:
>
> > In D72225#1818149 <https://reviews.llvm.org/D72225#1818149>, @MaskRay wrote:
> >
> > > @skan @craig.topper This patch introduces an unknown bug. I applied Diff 15 (https://reviews.llvm.org/D72225?id=237569) + D72463 <https://reviews.llvm.org/D72463> (clangDriver) + a local patch which enables `-mbranches-within-32B-boundaries` by default, then randonly picked 1000 tests and 40 failed. I will try providing a reproduce.
> > >
> > > NOP padding alone seems good.
> >
> >
> > Could you reproduce the fail with this patch only? Applying three patches together makes things complicated and can not prove there is something wrong. I am glad to wait for the reproduced fail for half day.
>
>
>
>
> - `-mllvm -x86-align-branch-boundary=32 -mllvm -x86-align-branch=fused,jcc,jmp -mllvm -x86-align-branch-prefix-size=0` => pass (NOP padding only)
> - `-mllvm -x86-align-branch-boundary=32 -mllvm -x86-align-branch=fused,jcc,jmp -mllvm -x86-align-branch-prefix-size=5` => fail (this patch)
>
>   How can I reproduce with this patch only? Without a clangDriver patch, the code path added in this patch is not exercised and surely nothing breaks.
>
>   As I explained earlier, we already have NOP padding (it passes for our internal 1000 tests) With part of https://reviews.llvm.org/D72463 , we have something decent enough to ship for clang 10.0.0 (the difference between NOP padding alone and this patch is tiny). I understand that you eagerly want to ship the full feature for clang 10.0.0, but IMHO it is not very safe.


Passing option `-mllvm -x86-align-branch-boundary=32 -mllvm -x86-align-branch=fused+jcc+jmp -mllvm -x86-align-branch-prefix-size=5` doesn't need any patch for driver.


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

https://reviews.llvm.org/D72225