In D97982#2657470, @Amir wrote:

@skan:

Is the purpose of this patch to avoid cmp ends and jmp starts exactly at 64b alignment? If it's the only purpose, I think we can reuse the BoundaryAlign fragment by adding a function like "needPadding" in MCAssembler.cpp.

Yes, that's the purpose. We've looked at BoundaryAlign and aligned instruction bundles, but failed to come up with a simple solution. How do you envision BoundaryAlign avoiding the given alignment boundary falling between cmp and jmp? NeverAlign inserts a one-byte nop before cmp if it would end at a given boundary.

Could you also upstream the code that inserts/remove the NeverAlign fragment and some test cases? Usage can help us determine if the design of the fragment is reasonable.

We seek to upstream LLVM parts required by BOLT before pushing BOLT.
The usage is shown here: https://github.com/facebookincubator/BOLT/blob/68abc968b706b55585b1b8be315aef5d3bf90b1c/bolt/src/BinaryEmitter.cpp#L445
We analyze the basic block for macro-op fusion pairs; if such a pair is found, we insert NeverAlign fragment after the first instruction (cmp) with a call MCStreamer::emitNeverAlignCodeAtEnd(). That's the only external use of the interfaces we add, which makes it non-trivial to test. That's why I'm seeking to add a way to trigger the insertion of NeverAlign fragment (see comment above).

In D97982#2652314, @Amir wrote:

We've considered two options to integrate this functionality into LLVM:

1. Similar to JCC errata/BoundaryAlign fragment:

Automatically insert NeverAlign into the section on cmp instruction, check if fuseable instruction follows it, and remove the fragment otherwise.

2. Similar to the current usage in BOLT: https://github.com/facebookincubator/BOLT/blob/68abc968b706b55585b1b8be315aef5d3bf90b1c/bolt/src/BinaryEmitter.cpp#L445

Check basic block instructions in advance, only emit NeverAlign fragment if fuseable sequence is found.

Pros and cons of each approach:

1. Pro is that macro-op fusion alignment will be applied to inline assembly as well as LLVM-produced MCs, the downside is that I've observed a lot of insertions and removals of NeverAlign fragment, which may hurt processing time.
2. Pro is that the check and insertion are happening once per basic block, the downside is that we need to integrate the logic for determining the insertion of this fragment higher up in the MCStreamer stack where we see the basic block.

Thoughts?
+CC @skan

In D92842#2561721, @lenary wrote:

Can you link to the bug you are referencing? With a bug to point to, it'll be easier to push for this to land.

@lenary Do you need to update something? There are three tests fail according to the log.

FYI, this patch also fix the bug that "X86::Int_MemBarrier should be a MetaInstruction".

LGTM. Before this patch, we could only write code like

CS
insl

addr32/addr16 and CS/DS/ES/FS/GS/SS are prefixes, so they should be allowed to be written before the instruction in the same line, and GNU-AS does support this syntax.
One behavior different from GNU-AS is that as rejects es and ss in 64-bit mode, but this has beyond the purpose of this patch.

LGTM

In D94542#2494838, @MaskRay wrote:

In D94542#2494774, @reames wrote:

Both the align and branch handling are "optimizations". I object to one being enabled and the other disabled. If you want them both on by default, fine. If you want them both off by default, fine. Having one off and one on is confusing.

I can disable the other one, too. Due to if (!X86PadForAlign && !X86PadForBranchAlign), disabling one suffices.

In D75203#2491618, @MaskRay wrote:

I am thinking of whether we can use some properties of MCSymbol to filter out some MCSymbol's in LabeledFragments but cannot find one which works (for example, IsUsedInReloc is only used in ObjectWriter, which is after the assembler optimization).

@condy: -mllvm -x86-pad-for-align=false is a workaround.

In D85782#2236493, @MaskRay wrote:

Longer answer:

binutils 2.26 introduced a regression: R_X86_64_PC32 can no longer be used against a protected symbol https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=ca3fe95e469b9daec153caa2c90665f5daaec2b5

The original issue is "Copy relocation against protected symbol doesn't work".
I agree with Rich Felker (https://gcc.gnu.org/ml/gcc/2016-04/msg00168.html) and
Cary Coutant (https://sourceware.org/ml/binutils/2016-03/msg00407.html https://gcc.gnu.org/ml/gcc/2016-04/msg00158.html https://gcc.gnu.org/ml/gcc/2016-04/msg00169.html) that we should
keep using direct access against protected symbols and disallow copy relocations against protected symbols.

I appreciate that Cary Coutant and Rafael Ávila de Espíndola added diagnostics to gold and lld, respectively:

• gold (https://sourceware.org/bugzilla/show_bug.cgi?id=19823)
• lld (https://bugs.llvm.org/show_bug.cgi?id=31476)

@skan Perhaps you are in a good position to change the resolutions to the following issues:)

GCC 5 x86-64 introduced a regression (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65248) I suggested a fix in May, 2019: https://sourceware.org/pipermail/gcc/2019-May/229309.html
i386 was flagged as a reproduce (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55012)

__attribute__((visibility("protected"))) int a;
int foo() { return a; } // GCC>=5 uses R_X86_64_GOTPCREL/R_X86_64_REX_GOTPCRELX instead of R_X86_64_PC32

binutils 2.26 introduced a regression R_X86_64_PC32 can no longer be used against a protected symbol https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=ca3fe95e469b9daec153caa2c90665f5daaec2b5

I think this introduces an old issue when lowering the instruction.

Address review comments

In D83366#2138210, @MaskRay wrote:

This is: if else if else if -> switch. I think it is just a refactoring and I agree that it is the right thing to do, but I don't find complexity being simplified.

In D83366#2138183, @MaskRay wrote:

Note that the number of lines actually increases... so I am not sure this is "simplification"

Update comments

Related revision: D80822

Ping.

Refactor addX86AlignBranchArgs to be reused in CommonArgs.cpp

Move tests to x86-malign-branch.c

LGTM

LGTM (with one comment)

Rebase

LGTM

Address review comments

Address review comments

Address review comments (only test file changes)

In D77851#1989744, @reames wrote:

Has this been reverted? I see discussion of a fix being worked on, but has the change which triggered problems been reverted? I can't tell easily from the review history.

Add test for RISCV to prevent future regression

Feel free to add yourself as a reviewer if you'd like to.

@MaskRay Is this acceptable?

In D77851#1988272, @Razer6 wrote:

In D77851#1988191, @MaskRay wrote:

Which way do you think is better? Personally, I prefer the second one, since getAssembler().getBackend().relaxInstruction(Relaxed, STI, Relaxed) is called in a loop, the assumption that third argument of relaxInstruction is a fresh uninitialized MCInst is very strange.

Special casing RISC-V looks good to me.

A special case is probably also needed for the Hexagon and AMDGPU backend, which does similar stuff in their relaxInstruction implementation than the RISC-V backend.

Is their any reason why relaxInstruction takes three argurments? Since relaxed is given in two arguments which are aliased to the same variable, I would drop the third argument and let relaxInstruction to either modify the given instruction or assign it to a fresh one (RISC-V, Hexagon, AMDGPU case).

In D77851#1988272, @Razer6 wrote:

In D77851#1988191, @MaskRay wrote:

Which way do you think is better? Personally, I prefer the second one, since getAssembler().getBackend().relaxInstruction(Relaxed, STI, Relaxed) is called in a loop, the assumption that third argument of relaxInstruction is a fresh uninitialized MCInst is very strange.

Special casing RISC-V looks good to me.

A special case is probably also needed for the Hexagon and AMDGPU backend, which does similar stuff in their relaxInstruction implementation than the RISC-V backend.

Is their any reason why relaxInstruction takes three argurments? Since relaxed is given in two arguments which are aliased to the same variable, I would drop the third argument and let relaxInstruction to either modify the given instruction or assign it to a fresh one (RISC-V, Hexagon, AMDGPU case).

In D77851#1986486, @Razer6 wrote:

It seems that this change breaks the RISCV backend. RISCVAsmBackend::relaxInstruction assumes that the Relaxed parameter is a fresh uninitialized MCInst. With this change, invalid instructions with too many operands are generated.

In D77851#1988060, @skan wrote:

In D77851#1986486, @Razer6 wrote:

It seems that this change breaks the RISCV backend. RISCVAsmBackend::relaxInstruction assumes that the Relaxed parameter is a fresh uninitialized MCInst. With this change, invalid instructions with too many operands are generated. A similar problem probably happens for the Hexagon and for the AMDGPU backend.

/cc @asb also FYI that this breaks the RISCV backend.

Could you provide a new fail case that can be reproduced?

Address review comments

In D77851#1986486, @Razer6 wrote:

It seems that this change breaks the RISCV backend. RISCVAsmBackend::relaxInstruction assumes that the Relaxed parameter is a fresh uninitialized MCInst. With this change, invalid instructions with too many operands are generated. A similar problem probably happens for the Hexagon and for the AMDGPU backend.

/cc @asb also FYI that this breaks the RISCV backend.

Address review comments

Address part of review comments
Check the REX is not zero before emitting it

Address review comments

In D77971#1985752, @MaskRay wrote:

In D77971#1985717, @skan wrote:

In D77971#1981345, @MaskRay wrote:

If we consider instructions in non-SHF_EXECINSTR sections an undefined behavior.

I asked some experts about this, and got the answer that putting instructions in non-executable section is legal and their encoding values will be treated as data. Do you have other concern about this patch?

Can you add some information to the description? Ideally there should be an example that instructions in a non-executable section are useful.

Personally I like this resolution because that means we don't need to extend D78138 (bss like sections) to non-executable sections.