There is a tiny logic error of D75300, making branch is not
correctly aligned with option -x86-pad-max-prefix-size
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Delete the unimplemented check in test file (we haven't supported relaxing
the branches to be aligned to reduce the bytes of nops)
It's not clear what bug you think you're fixing. Please provide a description of the bad behaviour.
llvm/test/MC/X86/align-branch-64-basic.s | ||
---|---|---|
106 | Please restore this test and show any diff. This test is not an unimplemented case, it's testing the jne being relaxed which is happening. | |
llvm/test/MC/X86/align-branch-64-pad-max-prefix.s | ||
11 | This case should already be handled by skipping instructions past the branch align. |
llvm/test/MC/X86/align-branch-64-pad-max-prefix.s | ||
---|---|---|
11 | Do you mean when jmp size is expanded, it cross the align boundary? That seems a problem. The jmp size should be frozen after nop padding. |
llvm/test/MC/X86/align-branch-64-pad-max-prefix.s | ||
---|---|---|
11 | Yes, the jump's size is expanded and leans against the boundary. This patch is trying to fix this bug. |
llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp | ||
---|---|---|
973–977 | The existing code in finishLayout intends to not pad instructions following boundary align by checking whether F is MCBoundaryAlignFragment. But this early continue prevents the control flow from reaching that check. This patch is pretty small (delete 3 lines) and is a bug fix, could reviewers take a close look at it? |
LGTM, thank you for the explanation from Tuesday, sorry for not noticing that sooner.
The existing code in finishLayout intends to not pad instructions following boundary align by checking whether F is MCBoundaryAlignFragment. But this early continue prevents the control flow from reaching that check. This patch is pretty small (delete 3 lines) and is a bug fix, could reviewers take a close look at it?