[RISCV] Override fixupNeedsRelaxationAdvanced to avoid MC relaxation always promote to 32-bit form when -mrelax enabled

[shiva0217]

Summary

Override fixupNeedsRelaxationAdvanced to avoid RISCV MC Branch Relaxation always promote 16-bit branches to 32-bit form when RISCV linker relaxation enabled.

[sabuasal]

test/MC/RISCV/relocations.s

93

Hi Shiva,

I noticed that gcc-as will relax jumps to unresolved symbols. only generates rvc-jump for resolved ones.

echo "c.jal foo" | riscv32-unknown-linux-gnu-as -march=rv32imc -o ./a.out && riscv32-unknown-linux-gnu-objdump -M no-aliases -dr ./a.out
00000000 <.text>:

0:   000000ef                jal     ra,0x0
                    0: R_RISCV_JAL  foo

echo "foo: nop; jal foo" | riscv32-unknown-linux-gnu-as -march rv32imc -o ./a.out && riscv32-unknown-linux-gnu-> objdump -d -r -M no-aliases ./a.out

0: 0001 c.addi zero,0

2:   3ffd                    c.jal   0 <foo>
                     2: R_RISCV_RVC_JUMP     foo

We can still maintain this behavior by checking the "Fixup" kind and "resolved" in fixupNeedsRelaxationAdvanced. However, this is a better code size optimization I think if the c.jal target appears to be within reach during linking.
@asb
What do you think?

[asb]

test/MC/RISCV/relocations.s

93

It would be great to expand the tests to cover the case of an unresolved jump target. Matching the gcc behaviour seems a sensible starting point.

[shiva0217]

test/MC/RISCV/relocations.s

93

Hi @sabuasal. Resolved will always false when -mrelax enabled due to shouldForceRelocation. If we relax c.jal by checking "Fixup" and "Resolved", c.jal will always relax to jal when -mrelax enabled even if the symbol actually could be resolved.
To match the gcc behaviour as @asb's comments, we have to identify the symbol is actually could be resolved to avoid always relax to expansion form. Perhaps we still need https://reviews.llvm.org/D44887? Any thoughts?

[sabuasal]

test/MC/RISCV/relocations.s

93

Hi Shiva,

if (STI.getFeatureBits()[RISCV::FeatureRelax]) is false we know that Resolved is false because the symbol is actually unresolved, not that it was set to false due to shouldForceRelection() behaviour. If that applies and the FixUp kind you get belongs to a compression fixup then we should just relax the instruction. This way we can match gcc for compressed jumps to unresolved symbols.

What do you think?

93

If after that you think the code will be too ugly I an fine going back to your original patch.

[shiva0217]

test/MC/RISCV/relocations.s

93

Hi @sabuasal,

bool fixupNeedsRelaxationAdvanced(const MCFixup &Fixup, bool Resolved,
                                  uint64_t Value,
                                  const MCRelaxableFragment *DF,
                                  const MCAsmLayout &Layout) const override {
  if (!STI.getFeatureBits()[RISCV::FeatureRelax] && !Resolved)
    return true;
  return fixupNeedsRelaxation(Fixup, Value, DF, Layout);
}

Yes, we could match gcc behaviour for unresolved symbols when relaxation disabled, but c.jal still not relax to jal for unresolved symbols when relaxation enabled.
I think gcc always relax c.jal to jal for unresolved symbols because if the symbols are outside of the compile unit, it's very likely that the symbol offset couldn't fit into c.jal operand field. I'll try to push https://reviews.llvm.org/D44887 if you think it's reasonable, too.

[sabuasal]

test/MC/RISCV/relocations.s

93

bool fixupNeedsRelaxationAdvanced(const MCFixup &Fixup, bool Resolved,
                                  uint64_t Value,
                                  const MCRelaxableFragment *DF,
                                  const MCAsmLayout &Layout) const override {
  if (!STI.getFeatureBits()[RISCV::FeatureRelax] && !Resolved)
    return true;

  if (STI.getFeatureBits()[RISCV::FeatureRelax])
     return true;

  return fixupNeedsRelaxation(Fixup, Value, DF, Layout);
}

[sabuasal]

test/MC/RISCV/relocations.s

93

Oops, please ignore the above comment I hit submit without realizing it.

[sabuasal]

test/MC/RISCV/relocations.s

93

I think gcc always relax c.jal to jal for unresolved symbols because if the symbols are outside of the compile unit, it's very likely that the symbol offset couldn't fit into c.jal operand field.

Yea I believe that is true and it make sense to me.

Yes, we could match gcc behavior for unresolved symbols when relaxation disabled, but c.jal still not relax to jal for unresolved symbols when relaxation enabled.

I just took a look at the assembler framework again. The problem is that the relaxation logic, which happens during the layoutOnce() shares some logic with the Relocation recording by calling evaluateFixup which checks shouldForceRelocation in the backend.

The only way I can see this working is if the fixupNeedsRelocationAdvnced() duplicates the work in evaluateFixup() to get the "Value" and "IsResolved". This way we can separate the relocation and relaxation logic but that does sound too invasive.

I think you are right, I might heave lead you in the wrong way, lets o back to the old patch.

[shiva0217]

test/MC/RISCV/relocations.s

93

Hi Sabuasal. Utilizing existing target hook always better than introducing a new one. Your comment's lead us to explore the possibility and make the new target hook more reasonable. Thanks for your comments and let's push the old one.

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

bool shouldForceRelocation(const MCAssembler &Asm, const MCFixup &Fixup,
     // 2. Find symbol(foo) in auipc's relocation type
     // 3. Resolve offset by symbol(foo)
     if (FixupKind == RISCV::fixup_riscv_pcrel_lo12_i ||
         FixupKind == RISCV::fixup_riscv_pcrel_hi20)
       return true;
     return STI.getFeatureBits()[RISCV::FeatureRelax];
   }
 
+  // Override fixupNeedsRelaxationAdvanced to avoid RISCV MC Branch Relaxation
+  // always promote 16-bit branches to 32-bit form when RISCV linker relaxation
+  // enabled.
+  bool fixupNeedsRelaxationAdvanced(const MCFixup &Fixup, bool Resolved,
+                                    uint64_t Value,
+                                    const MCRelaxableFragment *DF,
+                                    const MCAsmLayout &Layout) const override {
+    return fixupNeedsRelaxation(Fixup, Value, DF, Layout);
+  }
+
   bool fixupNeedsRelaxation(const MCFixup &Fixup, uint64_t Value,
                             const MCRelaxableFragment *DF,
                             const MCAsmLayout &Layout) const override;
 
   unsigned getNumFixupKinds() const override {
     return RISCV::NumTargetFixupKinds;
   }
 

test/MC/RISCV/relocations.s

 # INSTR: jal zero, foo
 # FIXUP: fixup A - offset: 0, value: foo, kind: fixup_riscv_jal
 
 bgeu a0, a1, foo
 # RELOC: R_RISCV_BRANCH
 # INSTR: bgeu a0, a1, foo
 # FIXUP: fixup A - offset: 0, value: foo, kind: fixup_riscv_branch
 
 c.jal foo
-# A compressed jump (c.j) to an unresolved symbol will be relaxed to a (jal).
-# RELOC: R_RISCV_JAL
+# RELOC: R_RISCV_RVC_JUMP
 # INSTR: c.jal foo
 # FIXUP: fixup A - offset: 0, value: foo, kind: fixup_riscv_rvc_jump
 
 c.bnez a0, foo
-# A compressed branch (c.bnez) to an unresolved symbol will be relaxed to a (bnez).
-# RELOC: R_RISCV_BRANCH
+# RELOC: R_RISCV_RVC_BRANCH
 # INSTR: c.bnez a0, foo
 # FIXUP: fixup A - offset: 0, value: foo, kind: fixup_riscv_rvc_branch

test/MC/RISCV/rv32-relaxation.s

 # RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=+c < %s \
 # RUN:     | llvm-objdump -d - | FileCheck -check-prefix=INSTR %s
+# RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=+c,+relax < %s \
+# RUN:     | llvm-objdump -d - | FileCheck -check-prefix=RELAX-INSTR %s
+# RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=+c,+relax < %s \
+# RUN:     | llvm-readobj -r | FileCheck -check-prefix=RELAX-RELOC %s
 
 FAR_JUMP_NEGATIVE:
   c.nop
 .space 2000
 
 FAR_BRANCH_NEGATIVE:
   c.nop
 .space 256
 
 NEAR_NEGATIVE:
   c.nop
 
 start:
   c.bnez a0, NEAR
 #INSTR: c.bnez a0, 72
+#RELAX-INSTR: c.bnez a0, 0
+#RELAX-RELOC: R_RISCV_RVC_BRANCH
   c.bnez a0, NEAR_NEGATIVE
 #INSTR: c.bnez a0, -4
+#RELAX-INSTR: c.bnez a0, 0
+#RELAX-RELOC: R_RISCV_RVC_BRANCH
   c.bnez a0, FAR_BRANCH
 #INSTR-NEXT: bnez a0, 326
+#RELAX-INSTR-NEXT: bnez a0, 0
+#RELAX-RELOC: R_RISCV_BRANCH
   c.bnez a0, FAR_BRANCH_NEGATIVE
 #INSTR-NEXT: bnez a0, -268
+#RELAX-INSTR-NEXT: bnez a0, 0
+#RELAX-RELOC: R_RISCV_BRANCH
   c.bnez a0, FAR_JUMP
 #INSTR-NEXT: bnez a0, 2320
+#RELAX-INSTR-NEXT: bnez a0, 0
+#RELAX-RELOC: R_RISCV_BRANCH
   c.bnez a0, FAR_JUMP_NEGATIVE
 #INSTR-NEXT: bnez a0, -2278
+#RELAX-INSTR-NEXT: bnez a0, 0
+#RELAX-RELOC: R_RISCV_BRANCH
 
   c.beqz a0, NEAR
 #INSTR-NEXT: c.beqz a0, 52
+#RELAX-INSTR-NEXT: c.beqz a0, 0
+#RELAX-RELOC: R_RISCV_RVC_BRANCH
   c.beqz a0, NEAR_NEGATIVE
 #INSTR-NEXT: c.beqz a0, -24
+#RELAX-INSTR-NEXT: c.beqz a0, 0
+#RELAX-RELOC: R_RISCV_RVC_BRANCH
   c.beqz a0, FAR_BRANCH
 #INSTR-NEXT: beqz a0, 306
+#RELAX-INSTR-NEXT: beqz a0, 0
+#RELAX-RELOC: R_RISCV_BRANCH
   c.beqz a0, FAR_BRANCH_NEGATIVE
 #INSTR-NEXT: beqz a0, -288
+#RELAX-INSTR-NEXT: beqz a0, 0
+#RELAX-RELOC: R_RISCV_BRANCH
   c.beqz a0, FAR_JUMP
 #INSTR-NEXT: beqz a0, 2300
+#RELAX-INSTR-NEXT: beqz a0, 0
+#RELAX-RELOC: R_RISCV_BRANCH
   c.beqz a0, FAR_JUMP_NEGATIVE
 #INSTR-NEXT: beqz a0, -2298
+#RELAX-INSTR-NEXT: beqz a0, 0
+#RELAX-RELOC: R_RISCV_BRANCH
 
   c.j NEAR
 #INSTR-NEXT: c.j 32
+#RELAX-INSTR-NEXT: c.j 0
+#RELAX-RELOC: R_RISCV_RVC_JUMP
   c.j NEAR_NEGATIVE
 #INSTR-NEXT: c.j -44
+#RELAX-INSTR-NEXT: c.j 0
+#RELAX-RELOC: R_RISCV_RVC_JUMP
   c.j FAR_BRANCH
 #INSTR-NEXT: c.j 286
+#RELAX-INSTR-NEXT: c.j 0
+#RELAX-RELOC: R_RISCV_RVC_JUMP
   c.j FAR_BRANCH_NEGATIVE
 #INSTR-NEXT: c.j -306
+#RELAX-INSTR-NEXT: c.j 0
+#RELAX-RELOC: R_RISCV_RVC_JUMP
   c.j FAR_JUMP
 #INSTR-NEXT: j 2284
+#RELAX-INSTR-NEXT: j 0
+#RELAX-RELOC: R_RISCV_JAL
   c.j FAR_JUMP_NEGATIVE
 #INSTR-NEXT: j -2314
+#RELAX-INSTR-NEXT: j 0
+#RELAX-RELOC: R_RISCV_JAL
 
   c.jal NEAR
 #INSTR: c.jal 16
+#RELAX-INSTR: c.jal 0
+#RELAX-RELOC: R_RISCV_RVC_JUMP
   c.jal NEAR_NEGATIVE
 #INSTR: c.jal -60
+#RELAX-INSTR: c.jal 0
+#RELAX-RELOC: R_RISCV_RVC_JUMP
   c.jal FAR_BRANCH
 #INSTR-NEXT: c.jal 270
+#RELAX-INSTR-NEXT: c.jal 0
+#RELAX-RELOC: R_RISCV_RVC_JUMP
   c.jal FAR_BRANCH_NEGATIVE
 #INSTR-NEXT: c.jal -322
+#RELAX-INSTR-NEXT: c.jal 0
+#RELAX-RELOC: R_RISCV_RVC_JUMP
   c.jal FAR_JUMP
 #INSTR-NEXT: jal 2268
+#RELAX-INSTR-NEXT: jal 0
+#RELAX-RELOC: R_RISCV_JAL
   c.jal FAR_JUMP_NEGATIVE
 #INSTR-NEXT: jal -2330
+#RELAX-INSTR-NEXT: jal 0
+#RELAX-RELOC: R_RISCV_JAL
 
 NEAR:
   c.nop
 .space 256
 FAR_BRANCH:
   c.nop
 .space 2000
 FAR_JUMP:
   c.nop

test/MC/RISCV/rv64-relaxation.s

 # RUN: llvm-mc -filetype=obj -triple riscv64 -mattr=+c < %s \
 # RUN:     | llvm-objdump -d - | FileCheck -check-prefix=INSTR %s
+# RUN: llvm-mc -filetype=obj -triple riscv64 -mattr=+c,+relax < %s \
+# RUN:     | llvm-objdump -d - | FileCheck -check-prefix=RELAX-INSTR %s
+# RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=+c,+relax < %s \
+# RUN:     | llvm-readobj -r | FileCheck -check-prefix=RELAX-RELOC %s
 
 FAR_JUMP_NEGATIVE:
   c.nop
 .space 2000
 
 FAR_BRANCH_NEGATIVE:
   c.nop
 .space 256
 
 NEAR_NEGATIVE:
   c.nop
 
 start:
   c.bnez a0, NEAR
 #INSTR: c.bnez a0, 56
+#RELAX-INSTR: c.bnez a0, 0
+#RELAX-RELOC: R_RISCV_RVC_BRANCH
   c.bnez a0, NEAR_NEGATIVE
 #INSTR: c.bnez a0, -4
+#RELAX-INSTR: c.bnez a0, 0
+#RELAX-RELOC: R_RISCV_RVC_BRANCH
   c.bnez a0, FAR_BRANCH
 #INSTR-NEXT: bnez a0, 310
+#RELAX-INSTR-NEXT: bnez a0, 0
+#RELAX-RELOC: R_RISCV_BRANCH
   c.bnez a0, FAR_BRANCH_NEGATIVE
 #INSTR-NEXT: bnez a0, -268
+#RELAX-INSTR-NEXT: bnez a0, 0
+#RELAX-RELOC: R_RISCV_BRANCH
   c.bnez a0, FAR_JUMP
 #INSTR-NEXT: bnez a0, 2304
+#RELAX-INSTR-NEXT: bnez a0, 0
+#RELAX-RELOC: R_RISCV_BRANCH
   c.bnez a0, FAR_JUMP_NEGATIVE
 #INSTR-NEXT: bnez a0, -2278
+#RELAX-INSTR-NEXT: bnez a0, 0
+#RELAX-RELOC: R_RISCV_BRANCH
 
   c.beqz a0, NEAR
 #INSTR-NEXT: c.beqz a0, 36
+#RELAX-INSTR-NEXT: c.beqz a0, 0
+#RELAX-RELOC: R_RISCV_RVC_BRANCH
   c.beqz a0, NEAR_NEGATIVE
 #INSTR-NEXT: c.beqz a0, -24
+#RELAX-INSTR-NEXT: c.beqz a0, 0
+#RELAX-RELOC: R_RISCV_RVC_BRANCH
   c.beqz a0, FAR_BRANCH
 #INSTR-NEXT: beqz a0, 290
+#RELAX-INSTR-NEXT: beqz a0, 0
+#RELAX-RELOC: R_RISCV_BRANCH
   c.beqz a0, FAR_BRANCH_NEGATIVE
 #INSTR-NEXT: beqz a0, -288
+#RELAX-INSTR-NEXT: beqz a0, 0
+#RELAX-RELOC: R_RISCV_BRANCH
   c.beqz a0, FAR_JUMP
 #INSTR-NEXT: beqz a0, 2284
+#RELAX-INSTR-NEXT: beqz a0, 0
+#RELAX-RELOC: R_RISCV_BRANCH
   c.beqz a0, FAR_JUMP_NEGATIVE
 #INSTR-NEXT: beqz a0, -2298
+#RELAX-INSTR-NEXT: beqz a0, 0
+#RELAX-RELOC: R_RISCV_BRANCH
 
   c.j NEAR
 #INSTR-NEXT: c.j 16
+#RELAX-INSTR-NEXT: c.j 0
+#RELAX-RELOC: R_RISCV_RVC_JUMP
   c.j NEAR_NEGATIVE
 #INSTR-NEXT: c.j -44
+#RELAX-INSTR-NEXT: c.j 0
+#RELAX-RELOC: R_RISCV_RVC_JUMP
   c.j FAR_BRANCH
 #INSTR-NEXT: c.j 270
+#RELAX-INSTR-NEXT: c.j 0
+#RELAX-RELOC: R_RISCV_RVC_JUMP
   c.j FAR_BRANCH_NEGATIVE
 #INSTR-NEXT: c.j -306
+#RELAX-INSTR-NEXT: c.j 0
+#RELAX-RELOC: R_RISCV_RVC_JUMP
   c.j FAR_JUMP
 #INSTR-NEXT: j 2268
+#RELAX-INSTR-NEXT: j 0
+#RELAX-RELOC: R_RISCV_JAL
   c.j FAR_JUMP_NEGATIVE
 #INSTR-NEXT: j -2314
+#RELAX-INSTR-NEXT: j 0
+#RELAX-RELOC: R_RISCV_JAL
 
 NEAR:
   c.nop
 
 .space 256
 FAR_BRANCH:
   c.nop
 
 .space 2000
 FAR_JUMP:
   c.nop