[X86][MC] Make -x86-pad-max-prefix-size compatible with --mc-relax-all

[skan]

Summary

We allow non-relaxable instructions emitted into relaxable Fragment when we prefix padding branch. So we need to check if the instruction need relaxation before relaxing it. Without this patch, it currently triggers a report_fatal_error in llvm::MCAsmBackend::relaxInstruction when we prefix padding branch along with --mc-relax-all.

[skan]

The failed tests are not related. I ran check-all locally and passed.

[LuoYuanke]

LGTM. Better to wait for Philip accept.

[MaskRay]

Best to state that this currently triggers a report_fatal_error in llvm::MCAsmBackend::relaxInstruction.

llvm/test/MC/X86/align-branch-64-relax-all.s

2

You can actually strip -pc-linux-gnu. ELF is the default.

[skan]

llvm/test/MC/X86/align-branch-64-relax-all.s

2

Thanks for the suggestion. I plan to strip all the -pc-linux-gnu and the blank before # RUN in the test files align-branch-* after landing this patch.

[skan]

Nothing changed, just to trigger a new remote build

[Razer6]

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.

[skan]

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?

[skan]

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?

yeah, I got your point. We may need to add a lit test to gurantee this assumption will not be broken.

[skan]

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.

We can fix this in two ways

1. Change the control flow in MCObjectStreamer.cpp
2. or create a fresh uninitialized MCInst at the beginning of RISCVAsmBackend::relaxInstruction, and after relaxation, assign its value to Relaxed at the end of RISCVAsmBackend::relaxInstruction.

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.

[MaskRay]

In D77851#1988163, @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.

We can fix this in two ways

1. Change the control flow in MCObjectStreamer.cpp
2. or create a fresh uninitialized MCInst at the beginning of RISCVAsmBackend::relaxInstruction, and after relaxation, assign its value to Relaxed at the end of RISCVAsmBackend::relaxInstruction.

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.

[Razer6]

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).

[skan]

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).

Droppng the third argument looks good to me.

[skan]

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).

I am working on this fix.

[nickdesaulniers]

In D77851#1988060, @skan wrote:

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

https://bugs.llvm.org/show_bug.cgi?id=45580

this broke our Linux kernel builds of RISCV, too.

[reames]

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.

[LuoYuanke]

llvm/lib/MC/MCObjectStreamer.cpp

404

If target expected "Relaxed" be a fresh MCInst, then this line also has problem. I think for RISC-V, this line isn't been executed, so the problem is not exposed before. I prefer to fix the bug in MCObjectStreamer.cpp, because some other target may also append new operand to Relaxed MCInst. And the relaxInstruction() API seems imply the Relaxed MCInst is fresh.

while (getAssembler().getBackend().mayNeedRelaxation(Inst, STI)) {
  MCInst Relaxed;
  getAssembler().getBackend().relaxInstruction(Inst, STI, Relaxed);
  Inst = Relaxed;
}

[skan]

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.

No, we plan to fix based on this.

[skan]

llvm/lib/MC/MCObjectStreamer.cpp

404

The code you paste doesn't make sense, Inst is a const MCInst &.

llvm/lib/MC/MCObjectStreamer.cpp

void MCObjectStreamer::emitInstructionImpl(const MCInst &Inst,
 
   // Otherwise, relax and emit it as data if either:
   // - The RelaxAll flag was passed
   // - Bundling is enabled and this instruction is inside a bundle-locked
   //   group. We want to emit all such instructions into the same data
   //   fragment.
   if (Assembler.getRelaxAll() ||
       (Assembler.isBundlingEnabled() && Sec->isBundleLocked())) {
-    MCInst Relaxed;
-    getAssembler().getBackend().relaxInstruction(Inst, STI, Relaxed);
+    MCInst Relaxed = Inst;
     while (getAssembler().getBackend().mayNeedRelaxation(Relaxed, STI))
       getAssembler().getBackend().relaxInstruction(Relaxed, STI, Relaxed);
     EmitInstToData(Relaxed, STI);
     return;
   }
 
   // Otherwise emit to a separate fragment.
   EmitInstToFragment(Inst, STI);
 }
 

llvm/test/MC/X86/align-branch-64-relax-all.s

   # RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu --x86-align-branch-boundary=32 --x86-align-branch=fused+jcc --mc-relax-all %s | llvm-objdump -d --no-show-raw-insn - | FileCheck %s
+  # RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu --x86-align-branch-boundary=32 --x86-align-branch=fused+jcc --x86-pad-max-prefix-size=5 --mc-relax-all %s | llvm-objdump -d --no-show-raw-insn - | FileCheck %s
 
   # Check instructions can be aligned correctly along with option --mc-relax-all
 
   .text
   .global foo
 foo:
   .p2align  5
   .rept 25