[SPARC][MC] Fix encoding of backwards BPr branches

[koakuma]

Summary

Make sure that the upper bits of the offset is placed in bits 20-21 of the instruction word.
This fixes the encoding of backwards (negative offset) BPr branches.

(Previously, the upper two bits of the offset would overwrite parts of the rs1 field, causing it to branch on the wrong register, with the wrong offset)

[koakuma]

Ping?

[jrtc27]

llvm/lib/Target/Sparc/MCTargetDesc/SparcAsmBackend.cpp

45

This doesn't make sense to me. Shouldn't it be (Value >> 16) & 0x3? The fixup tables are correctly specifying the 2 bits for BE and LE.

llvm/lib/Target/Sparc/MCTargetDesc/SparcELFObjectWriter.cpp

67

Isn't this going to create two relocations rather than one? Really there should be a single fixup for the whole thing.

[koakuma]

llvm/lib/Target/Sparc/MCTargetDesc/SparcELFObjectWriter.cpp

67

Yeah, I tried to merge the _14 and the _2 parts, but the MCFixupKindInfo table does not support split bits.
One thing that I can think of is to just emit the real ELF relocation with the _14 internal type and emit a R_SPARC_NONE instead with the _2 internal type but I'm not really sure if this is okay?

[koakuma]

• Fix calculation for fixup_sparc_br16_2.
• Make sure to properly shift fixup values as described in the fixup tables.
• Only emit one R_SPARC_WDISP16 relocation per one fixup_sparc_br16_14/2 pair.

[koakuma]

Remove stray include statement.

[koakuma]

Ping?

[jrtc27]

llvm/lib/Target/Sparc/MCTargetDesc/SparcELFObjectWriter.cpp

72

This is pretty gross, and wastes space in the result. Please just have a single fixup.

[koakuma]

Merge fixup_sparc_br16_2/fixup_sparc_br16_14 into a single fixup.

[brad]

@jrtc27 Ping?

[jrtc27]

llvm/lib/Target/Sparc/MCTargetDesc/SparcAsmBackend.cpp

54

This looks good now

207

Could you please commit this reformatting separately? Combining significant reformatting with functional changes isn't great.

351

This seems unrelated?

llvm/lib/Target/Sparc/MCTargetDesc/SparcFixupKinds.h

15 ↗

(On Diff #504445)

If you're going to reformat this, deindent the namespace too to match every other target, and commit it all separately from the functional change.

llvm/test/MC/Sparc/sparc64-bpr-offset.s

4

for should be from or to

[koakuma]

llvm/lib/Target/Sparc/MCTargetDesc/SparcAsmBackend.cpp

207

Okay, will remove the formatting changes from this patch.

351

I put this because I noticed that the offset/size values from the table is not used at all, so the upper part of the fixup bits aren't placed properly.
(This happens on the old version of the patch with split fixup_sparc_br16_2/fixup_sparc_br16_14, at least)

It's basically just adding what other architectures already do (but for some reason hasn't been done here in the SPARC backend)... should I submit this separately?

[koakuma]

Undo formatting changes and fix typo in test comment.

[brad]

@jrtc27 Ping?

[koakuma]

Ping?

[brad]

@jrtc27 Ping?

[brad]

@jrtc27 Ping?

[brad]

@arsenm Ping.

[arsenm]

Don't know anything about sparc but code seems fine

[vitalybuka]

https://lab.llvm.org/buildbot/#/builders/85/builds/15963

FAIL: LLVM :: DebugInfo/Sparc/gnu-window-save.ll (44906 of 74125)
******************** TEST 'LLVM :: DebugInfo/Sparc/gnu-window-save.ll' FAILED ********************
Script:
--
: 'RUN: at line 1';   /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/llc -filetype=obj -O0 < /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/test/DebugInfo/Sparc/gnu-window-save.ll -mtriple sparc64-unknown-linux-gnu | /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/llvm-dwarfdump -v - | /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/FileCheck /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/test/DebugInfo/Sparc/gnu-window-save.ll --check-prefix=SPARC64
: 'RUN: at line 2';   /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/llc -filetype=obj -O0 < /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/test/DebugInfo/Sparc/gnu-window-save.ll -mtriple sparc-unknown-linux-gnu   | /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/llvm-dwarfdump -v - | /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/FileCheck /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/test/DebugInfo/Sparc/gnu-window-save.ll --check-prefix=SPARC32
--
Exit Code: 2
Command Output (stderr):
--
/b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/Target/Sparc/MCTargetDesc/SparcAsmBackend.cpp:357:15: runtime error: shift exponent 4294967264 is too large for 64-bit type 'uint64_t' (aka 'unsigned long')
    #0 0x562868ff62ff in (anonymous namespace)::ELFSparcAsmBackend::applyFixup(llvm::MCAssembler const&, llvm::MCFixup const&, llvm::MCValue const&, llvm::MutableArrayRef<char>, unsigned long, bool, llvm::MCSubtargetInfo const*) const /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/Target/Sparc/MCTargetDesc/SparcAsmBackend.cpp
    #1 0x562869ff521f in llvm::MCAssembler::layout(llvm::MCAsmLayout&) /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/MC/MCAssembler.cpp:928:22
    #2 0x562869ff5f2d in llvm::MCAssembler::Finish() /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/MC/MCAssembler.cpp:938:3
    #3 0x56286a02453e in llvm::MCELFStreamer::finishImpl() /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/MC/MCELFStreamer.cpp:718:27
    #4 0x5628694200a5 in llvm::AsmPrinter::doFinalization(llvm::Module&) /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2397:16
    #5 0x562869efba12 in llvm::FPPassManager::doFinalization(llvm::Module&) /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1499:41
    #6 0x562869ef1c8a in runOnModule /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1586:41
    #7 0x562869ef1c8a in llvm::legacy::PassManagerImpl::run(llvm::Module&) /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:535:44
    #8 0x5628672cf06c in compileModule(char**, llvm::LLVMContext&) /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/tools/llc/llc.cpp:756:8
    #9 0x5628672cc89b in main /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/tools/llc/llc.cpp:420:22
    #10 0x7fafb5c2350f  (/lib/x86_64-linux-gnu/libc.so.6+0x2350f) (BuildId: d1704d25fbbb72fa95d517b883131828c0883fe9)
    #11 0x7fafb5c235c8 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x235c8) (BuildId: d1704d25fbbb72fa95d517b883131828c0883fe9)
    #12 0x5628672a02e4 in _start (/b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/llc+0x5d892e4)

[koakuma]

Remove the offset shift calculation. We already took care of proper placement of BPr offset value through other means, so this is not needed anymore.

This should also takes care of the sanitizer errors.

llvm/lib/Target/Sparc/MCTargetDesc/SparcAsmBackend.cpp

static unsigned adjustFixupValue(unsigned Kind, uint64_t Value) {
 
   case Sparc::fixup_sparc_br22:
     return (Value >> 2) & 0x3fffff;
 
   case Sparc::fixup_sparc_br19:
     return (Value >> 2) & 0x7ffff;
 
   case Sparc::fixup_sparc_br16_2:
-    return (Value >> 2) & 0xc000;
+    return ((Value >> 2) & 0xc000) << 6;
 
   case Sparc::fixup_sparc_br16_14:
     return (Value >> 2) & 0x3fff;
 
   case Sparc::fixup_sparc_hix22:
     return (~Value >> 10) & 0x3fffff;
 
   case Sparc::fixup_sparc_pc22:
   case Sparc::fixup_sparc_got22:
   case Sparc::fixup_sparc_tls_gd_hi22:
   case Sparc::fixup_sparc_tls_ldm_hi22:
   case Sparc::fixup_sparc_tls_ie_hi22:
   case Sparc::fixup_sparc_hi22:
   case Sparc::fixup_sparc_lm:
     return (Value >> 10) & 0x3fffff;
 
   case Sparc::fixup_sparc_got13:
const MCFixupKindInfo &getFixupKindInfo(MCFixupKind Kind) const override {
         { "fixup_sparc_tls_ie_add",     0,  0,  0 },
         { "fixup_sparc_tls_le_hix22",   0,  0,  0 },
         { "fixup_sparc_tls_le_lox10",   0,  0,  0 },
         { "fixup_sparc_hix22",         10, 22,  0 },
         { "fixup_sparc_lox10",         19, 13,  0 },
         { "fixup_sparc_gotdata_hix22",  0,  0,  0 },
         { "fixup_sparc_gotdata_lox10",  0,  0,  0 },
         { "fixup_sparc_gotdata_op",     0,  0,  0 },
       };
 
       const static MCFixupKindInfo InfosLE[Sparc::NumTargetFixupKinds] = {
         // name                    offset bits  flags
         { "fixup_sparc_call30",     0,     30,  MCFixupKindInfo::FKF_IsPCRel },
         { "fixup_sparc_br22",       0,     22,  MCFixupKindInfo::FKF_IsPCRel },
         { "fixup_sparc_br19",       0,     19,  MCFixupKindInfo::FKF_IsPCRel },
         { "fixup_sparc_br16_2",    20,      2,  MCFixupKindInfo::FKF_IsPCRel },
         { "fixup_sparc_br16_14",    0,     14,  MCFixupKindInfo::FKF_IsPCRel },
void applyFixup(const MCAssembler &Asm, const MCFixup &Fixup,
                     uint64_t Value, bool IsResolved,
                     const MCSubtargetInfo *STI) const override {
 
       if (Fixup.getKind() >= FirstLiteralRelocationKind)
         return;
       Value = adjustFixupValue(Fixup.getKind(), Value);
       if (!Value) return;           // Doesn't change encoding.
 
       unsigned NumBytes = getFixupKindNumBytes(Fixup.getKind());
       unsigned Offset = Fixup.getOffset();
       // For each byte of the fragment that the fixup touches, mask in the bits
       // from the fixup value. The Value has been "split up" into the
       // appropriate bitfields above.
       for (unsigned i = 0; i != NumBytes; ++i) {
         unsigned Idx = Endian == support::little ? i : (NumBytes - 1) - i;
         Data[Offset + Idx] |= uint8_t((Value >> (i * 8)) & 0xff);
       }

llvm/lib/Target/Sparc/MCTargetDesc/SparcELFObjectWriter.cpp

default:
       llvm_unreachable("Unimplemented fixup -> relocation");
     case FK_Data_1:                  return ELF::R_SPARC_DISP8;
     case FK_Data_2:                  return ELF::R_SPARC_DISP16;
     case FK_Data_4:                  return ELF::R_SPARC_DISP32;
     case FK_Data_8:                  return ELF::R_SPARC_DISP64;
     case Sparc::fixup_sparc_call30:  return ELF::R_SPARC_WDISP30;
     case Sparc::fixup_sparc_br22:    return ELF::R_SPARC_WDISP22;
     case Sparc::fixup_sparc_br19:    return ELF::R_SPARC_WDISP19;
+    case Sparc::fixup_sparc_br16_14:
+    case Sparc::fixup_sparc_br16_2:
+      return ELF::R_SPARC_WDISP16;
     case Sparc::fixup_sparc_pc22:    return ELF::R_SPARC_PC22;
     case Sparc::fixup_sparc_pc10:    return ELF::R_SPARC_PC10;
     case Sparc::fixup_sparc_wplt30:  return ELF::R_SPARC_WPLT30;
     }
   }
 
   switch(Fixup.getTargetKind()) {
   default:
     llvm_unreachable("Unimplemented fixup -> relocation");
   case FK_NONE:                  return ELF::R_SPARC_NONE;
   case FK_Data_1:                return ELF::R_SPARC_8;
   case FK_Data_2:                return ((Fixup.getOffset() % 2)
                                          ? ELF::R_SPARC_UA16

llvm/test/MC/Sparc/sparc64-bpr-offset.s

+! RUN: llvm-mc -arch=sparcv9 -filetype=obj %s | llvm-objdump -d - | FileCheck %s --check-prefix=BIN
+
+        !! SPARCv9/SPARC64 BPr branches have different offset encoding than the others,
+        !! so make sure that our offset bits don't trample on other fields.
+        !! This is particularly important with backwards branches.
+
+        ! BIN:  0: 02 c8 40 01    brz %g1, 1
+        ! BIN:  4: 04 c8 40 01    brlez %g1, 1
+        ! BIN:  8: 06 c8 40 01    brlz %g1, 1
+        ! BIN:  c: 0a c8 40 01    brnz %g1, 1
+        ! BIN: 10: 0c c8 40 01    brgz %g1, 1
+        ! BIN: 14: 0e c8 40 01    brgez %g1, 1
+        brz   %g1, .+4
+        brlez %g1, .+4
+        brlz  %g1, .+4
+        brnz  %g1, .+4
+        brgz  %g1, .+4
+        brgez %g1, .+4
+
+        ! BIN: 18: 02 f8 7f ff    brz %g1, 65535
+        ! BIN: 1c: 04 f8 7f ff    brlez %g1, 65535
+        ! BIN: 20: 06 f8 7f ff    brlz %g1, 65535
+        ! BIN: 24: 0a f8 7f ff    brnz %g1, 65535
+        ! BIN: 28: 0c f8 7f ff    brgz %g1, 65535
+        ! BIN: 2c: 0e f8 7f ff    brgez %g1, 65535
+        brz   %g1, .-4
+        brlez %g1, .-4
+        brlz  %g1, .-4
+        brnz  %g1, .-4
+        brgz  %g1, .-4
+        brgez %g1, .-4