[RISCV][MC] Add CLI option to disable branch relaxation

[jobnoorman]

Summary

D154958 enables branch relaxation for unresolved symbols. This has an
interesting consequence for some LLD tests: branch relocations are
tested by using branches to undefined symbols and defining them, with
different values, on the LLD command line. These tests broke and there
doesn't seem to be an easy workaround: as far as I can tell, there is no
way to convince llvm-mc to emit a branch relocation to an undefined
symbol without branch relaxation kicking in.

This patch proposes to add a flag, --riscv-disable-branch-relaxation,
to do just that. The main purpose for this flag is for testing but it
might be seen as a first step to some kind of "strict" or WYSIWYG mode
(i.e., what you give to the assembler is exactly what comes out). The
need for this has been mentioned in, for example, D108961. However, I
suspect there will be a lot of discussion around what exactly such a
strict mode would look like. Therefore, I gated this feature behind a
CLI flag instead of adding a new target feature.

[asb]

I'm supportive of adding a targeted flag to make it easier to write these tests, with the idea that a more public facing flag might later be introduced that alters a full "strict" mode.

I think it might make sense to pass cl::Hidden to DisableBranchRelaxation so this doesn't become a documented feature.

llvm/test/MC/RISCV/long-jump-disable-relax.s

3

Nit: I think we normally use the -single-hyphen-prefix spelling for llvm-mc opts.

[MaskRay]

llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

32

Drop false.

I think positive option names are more common. Perhaps -riscv-asm-relax-branch=0 since asm and linker relaxation are quite different and the current name can lure users to think the other way.

[MaskRay]

llvm/test/MC/RISCV/long-jump-disable-relax.s

18

Also add tests that the target symbol is defined.

[jobnoorman]

Address reviewer comments:

• Make option cl::Hidden
• Rename option to -riscv-asm-relax-branches
• Add tests for branches to defined symbols

The tests revealed some issues for the case where symbols are out of range for
compressed jumps and branches: no errors were reported because it was impossible
to trigger this (due to relaxation). This update also adds to necessary error
reporting.

[asb]

LGTM, thanks.

[MaskRay]

llvm/test/MC/RISCV/long-jump-disable-relax.s

2

Give a file-level comment?

45

[[@LINE]] is deprecated lit syntax. Use [[#@LINE]] for new tests.

[jobnoorman]

Address @MaskRay's comments

llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

 #include "llvm/MC/MCAssembler.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCDirectives.h"
 #include "llvm/MC/MCELFObjectWriter.h"
 #include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCObjectWriter.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/MC/MCValue.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/EndianStream.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/LEB128.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace llvm;
 
+static cl::opt<bool> RelaxBranches("riscv-asm-relax-branches", cl::init(true),
+                                   cl::Hidden);
+
 std::optional<MCFixupKind> RISCVAsmBackend::getFixupKind(StringRef Name) const {
   if (STI.getTargetTriple().isOSBinFormatELF()) {
     unsigned Type;
     Type = llvm::StringSwitch<unsigned>(Name)
 #define ELF_RELOC(X, Y) .Case(#X, Y)
 #include "llvm/BinaryFormat/ELFRelocs/RISCV.def"
 #undef ELF_RELOC
                .Case("BFD_RELOC_NONE", ELF::R_RISCV_NONE)

 }
 
 bool RISCVAsmBackend::fixupNeedsRelaxationAdvanced(const MCFixup &Fixup,
                                                    bool Resolved,
                                                    uint64_t Value,
                                                    const MCRelaxableFragment *DF,
                                                    const MCAsmLayout &Layout,
                                                    const bool WasForced) const {
+  if (!RelaxBranches)
+    return false;
+
   int64_t Offset = int64_t(Value);
   unsigned Kind = Fixup.getTargetKind();
 
   // We only do conditional branch relaxation when the symbol is resolved.
   // For conditional branch, the immediate must be in the range
   // [-4096, 4094].
   if (Kind == RISCV::fixup_riscv_branch)
     return Resolved && !isInt<13>(Offset);
case RISCV::fixup_riscv_call_plt: {
     // Jalr will add UpperImm with the sign-extended 12-bit LowerImm,
     // we need to add 0x800ULL before extract upper bits to reflect the
     // effect of the sign extension.
     uint64_t UpperImm = (Value + 0x800ULL) & 0xfffff000ULL;
     uint64_t LowerImm = Value & 0xfffULL;
     return UpperImm | ((LowerImm << 20) << 32);
   }
   case RISCV::fixup_riscv_rvc_jump: {
+    if (!isInt<12>(Value))
+      Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
     // Need to produce offset[11|4|9:8|10|6|7|3:1|5] from the 11-bit Value.
     unsigned Bit11  = (Value >> 11) & 0x1;
     unsigned Bit4   = (Value >> 4) & 0x1;
     unsigned Bit9_8 = (Value >> 8) & 0x3;
     unsigned Bit10  = (Value >> 10) & 0x1;
     unsigned Bit6   = (Value >> 6) & 0x1;
     unsigned Bit7   = (Value >> 7) & 0x1;
     unsigned Bit3_1 = (Value >> 1) & 0x7;
     unsigned Bit5   = (Value >> 5) & 0x1;
     Value = (Bit11 << 10) | (Bit4 << 9) | (Bit9_8 << 7) | (Bit10 << 6) |
             (Bit6 << 5) | (Bit7 << 4) | (Bit3_1 << 1) | Bit5;
     return Value;
   }
   case RISCV::fixup_riscv_rvc_branch: {
+    if (!isInt<9>(Value))
+      Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
     // Need to produce offset[8|4:3], [reg 3 bit], offset[7:6|2:1|5]
     unsigned Bit8   = (Value >> 8) & 0x1;
     unsigned Bit7_6 = (Value >> 6) & 0x3;
     unsigned Bit5   = (Value >> 5) & 0x1;
     unsigned Bit4_3 = (Value >> 3) & 0x3;
     unsigned Bit2_1 = (Value >> 1) & 0x3;
     Value = (Bit8 << 12) | (Bit4_3 << 10) | (Bit7_6 << 5) | (Bit2_1 << 3) |
             (Bit5 << 2);

llvm/test/MC/RISCV/long-jump-disable-relax.s

+## Test that long branches are not relaxed with -riscv-asm-relax-branches=0
+# RUN: split-file %s %t
+# RUN: llvm-mc -filetype=obj -triple=riscv64 -mattr=+c \
+# RUN:       -riscv-asm-relax-branches=0 %t/pass.s \
+# RUN:     | llvm-objdump -dr -M no-aliases - \
+# RUN:     | FileCheck %t/pass.s
+# RUN: not llvm-mc -filetype=obj -triple=riscv64 -mattr=+c -o /dev/null \
+# RUN:       -riscv-asm-relax-branches=0 %t/fail.s 2>&1 \
+# RUN:     | FileCheck %t/fail.s
+
+#--- pass.s
+  .text
+test_undefined:
+## Branches to undefined symbols should not be relaxed
+# CHECK:      bne a0, a1, {{.*}}
+# CHECK-NEXT: R_RISCV_BRANCH foo
+   bne a0, a1, foo
+# CHECK:      c.beqz a0, {{.*}}
+# CHECK-NEXT: R_RISCV_RVC_BRANCH foo
+   c.beqz a0, foo
+# CHECK:      c.j {{.*}}
+# CHECK-NEXT: R_RISCV_RVC_JUMP foo
+   c.j foo
+
+## Branches to defined in-range symbols should work normally
+test_defined_in_range:
+# CHECK:      bne a0, a1, {{.*}} <bar>
+  bne a0, a1, bar
+# CHECK:      c.beqz a0, {{.*}} <bar>
+   c.beqz a0, bar
+# CHECK:      c.j {{.*}} <bar>
+   c.j bar
+bar:
+
+#--- fail.s
+  .text
+## Branches to defined out-of-range symbols should report an error
+test_defined_out_of_range:
+  bne a0, a1, 1f # CHECK: :[[#@LINE]]:3: error: fixup value out of range
+  .skip (1 << 12)
+1:
+  c.beqz a0, 1f # CHECK: :[[#@LINE]]:3: error: fixup value out of range
+  .skip (1 << 8)
+1:
+  c.j 1f # CHECK: :[[#@LINE]]:3: error: fixup value out of range
+  .skip (1 << 11)
+1: