[ELF][PPC] Simplify {read,write}FromHalf16

[MaskRay]

Summary

I've change the variable name used in PPC64.cpp from "Instr" to "Insn"
because "Insn" seems a more common abbreviation for "instruction".

While changing PPC64.cpp relocateOne(), make R_PPC64_ADDR16_LO{_DS}
slightly more efficient by saving a read and a write for the TocOptimize
case.

[ruiu]

LGTM

[sfertile]

@MaskRay You broke the PowerpC64 build bot, and have been asked by both the buildbot maintainer (@anil9) and me to fix it. As of this morning the bot is still failing dues to the same problem (http://lab.llvm.org:8011/builders/ppc64le-lld-multistage-test/builds/4263/steps/build-stage2-unified-tree/logs/stdio) Can you please address that before committing PPC64 changes like this. I don't understand why you would consider committing more PowerPC64 changes when the bot is in this state, rather then fixing it first.

[ruiu]

Let me revert this change.

Sean, feel free to revert a change if it broke buildbots like this. You don't have to go through a pre-commit code review for a revert change that fixes buildbots.

[MaskRay]

In D63043#1537919, @ruiu wrote:

Let me revert this change.

Sean, feel free to revert a change if it broke buildbots like this. You don't have to go through a pre-commit code review for a revert change that fixes buildbots.

This revision is probably not the broken change. I'll check in a minute.

[ruiu]

Oh sorry I already reverted this. Once you fix the bot, feel free to resubmit (but please watch the bot after that)

[sfertile]

In D63043#1537919, @ruiu wrote:

Let me revert this change.

Sean, feel free to revert a change if it broke buildbots like this. You don't have to go through a pre-commit code review for a revert change that fixes buildbots.

Thanks. I tried reverting the breaking commit and a commit that built on top of it last week and was able to fix the PowerPC buildbot but broke all the X86 bots in the process. Unfortunately I don't have access to an X86 machine that can build llvm/lld right now so I backed out that change and ask @MaskRay to fix the bot.

In D63043#1537942, @MaskRay wrote:

In D63043#1537919, @ruiu wrote:

Let me revert this change.

Sean, feel free to revert a change if it broke buildbots like this. You don't have to go through a pre-commit code review for a revert change that fixes buildbots.

This revision is probably not the broken change. I'll check in a minute.

Your right, I believe it was commits 729111cf1824159bb4dd331cab8a829eab30313f and f49f58527a6d8147524d8d6f2eb1feb70f856292 that need to be pulled out.

[MaskRay]

Can you please address that before committing PPC64 changes like this. I don't understand why you would consider committing more PowerPC64 changes when the bot is in this state, rather then fixing it first.

@sfertile @anil9 I haven't received any email from ppc64le-lld-multistage-test since May 23. I am sorry I missed Anil's earlier email on rL361830.

The old links on rL361830 are no longer available. From http://lab.llvm.org:8011/builders/ppc64le-lld-multistage-test/builds/4263/steps/build-stage2-unified-tree/logs/stdio

ld.lld: error: relocation refers to a discarded section: .rodata._ZNK4llvm3MVT13getSizeInBitsEv
>>> defined in utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenRegisters.cpp.o
>>> referenced by CodeGenRegisters.cpp
>>>               utils/TableGen/CMakeFiles/llvm-tblgen.dir/CodeGenRegisters.cpp.o:(.toc+0x0)

I don't think this revision is responsible for the breakage.

@sfertile I find you reverted rL361830 and D62840 then restored them. I can't find the explanation why you did that. Sorry if I missed them.

AFAICT, D61583 (relanded by rL361830) caused some unhelpful errors that has been suppressed by D62840.

I'm still puzzled by the error on ppc64le because I think if my commit were bad, it should have caused other build bots to fail. May I ask you to run LLD_REPRODUCE=/tmp/reproduce.tar clang++ ... to get a reproduce tarball and upload it somewhere so that I can analyze it? Can you also check if ld.bfd or gold gives similar errors? (replace -fuse-ld=lld with -fuse-ld=bfd or -fuse-ld=gold)

@ruiu I don't think this commit is responsible for the build failure...

[MaskRay]

In D63043#1537967, @sfertile wrote:

In D63043#1537919, @ruiu wrote:

Let me revert this change.

Sean, feel free to revert a change if it broke buildbots like this. You don't have to go through a pre-commit code review for a revert change that fixes buildbots.

Thanks. I tried reverting the breaking commit and a commit that built on top of it last week and was able to fix the PowerPC buildbot but broke all the X86 bots in the process. Unfortunately I don't have access to an X86 machine that can build llvm/lld right now so I backed out that change and ask @MaskRay to fix the bot.

In D63043#1537942, @MaskRay wrote:

In D63043#1537919, @ruiu wrote:

Let me revert this change.

Sean, feel free to revert a change if it broke buildbots like this. You don't have to go through a pre-commit code review for a revert change that fixes buildbots.

This revision is probably not the broken change. I'll check in a minute.

Your right, I believe it was commits 729111cf1824159bb4dd331cab8a829eab30313f and f49f58527a6d8147524d8d6f2eb1feb70f856292 that need to be pulled out.

@sfertile Sorry but I can't find commit 729111cf1824159bb4dd331cab8a829eab30313f or f49f58527a6d8147524d8d6f2eb1feb70f856292 in either llvm/llvm-project (git monorepo) or llvm-mirror/lld (git multirepo). Do you have the revision numbers, e.g. r362867 (or rL362867 rLLD362867; the latters give a clickable link on Phabricator)

[sfertile]

In D63043#1538015, @MaskRay wrote:

@sfertile Sorry but I can't find commit 729111cf1824159bb4dd331cab8a829eab30313f or f49f58527a6d8147524d8d6f2eb1feb70f856292 in either llvm/llvm-project (git monorepo) or llvm-mirror/lld (git multirepo). Do you have the revision numbers, e.g. r362867 (or rL362867 rLLD362867; the latters give a clickable link on Phabricator)

Sorry I'm not sure where I grabbed those hashes from. The commits I pulled were https://reviews.llvm.org/rLLD362497 and https://reviews.llvm.org/rLLD361830. I verified the link failure on a local machine, and then pulling those 2 commits and rebuilding fixed the problem. I'm not sure there is a problem with your commits per se, rather it seemed we are garbage collecting a section that is referenced by toc relocations.

In D63043#1537973, @MaskRay wrote:

Can you please address that before committing PPC64 changes like this. I don't understand why you would consider committing more PowerPC64 changes when the bot is in this state, rather then fixing it first.

@sfertile @anil9 I haven't received any email from ppc64le-lld-multistage-test since May 23. I am sorry I missed Anil's earlier email on rL361830.
@sfertile I find you reverted rL361830 and D62840 then restored them. I can't find the explanation why you did that. Sorry if I missed them.

There seems to be some confusion all around, and I am sorry for that. I can see that Anils first message was before you pulled and updated your patch and we never followed up with you after your reland failed to fix the PowerPC build bot. Either way reverting those 2 patches is needed to get the bot back to green. Once they are pulled we can investigate what the underlying issue is. I'm reaching out to the current build bot monitor right now to get you the reproducer and so they can check if gold/bfd have a similar issue. (FWIW I checked the day I reverted the patches and there were no issues building with llvm and linking with gold).

[sfertile]

@kamaub ^

[MaskRay]

In D63043#1538267, @sfertile wrote:

@kamaub ^

@sfertile Someone gave me access to a unicamp ppc64le machine. I think I have pinpointed the root cause. D63182 has a workaround.

ELF/Arch/PPC.cpp

public:
   void relaxTlsIeToLe(uint8_t *Loc, RelType Type, uint64_t Val) const override;
 };
 } // namespace
 
 static uint16_t lo(uint32_t V) { return V; }
 static uint16_t ha(uint32_t V) { return (V + 0x8000) >> 16; }
 
 static uint32_t readFromHalf16(const uint8_t *Loc) {
-  return read32(Loc - (Config->EKind == ELF32BEKind ? 2 : 0));
+  return read32(Config->IsLE ? Loc : Loc - 2);
 }
 
 static void writeFromHalf16(uint8_t *Loc, uint32_t Insn) {
-  write32(Loc - (Config->EKind == ELF32BEKind ? 2 : 0), Insn);
+  write32(Config->IsLE ? Loc : Loc - 2, Insn);
 }
 
 void elf::writePPC32GlinkSection(uint8_t *Buf, size_t NumEntries) {
   // On PPC Secure PLT ABI, bl foo@plt jumps to a call stub, which loads an
   // absolute address from a specific .plt slot (usually called .got.plt on
   // other targets) and jumps there.
   //
   // a) With immediate binding (BIND_NOW), the .plt entry is resolved at load

ELF/Arch/PPC64.cpp

static bool isInstructionUpdateForm(uint32_t Encoding) {
   }
 }
 
 // There are a number of places when we either want to read or write an
 // instruction when handling a half16 relocation type. On big-endian the buffer
 // pointer is pointing into the middle of the word we want to extract, and on
 // little-endian it is pointing to the start of the word. These 2 helpers are to
 // simplify reading and writing in that context.
-static void writeInstrFromHalf16(uint8_t *Loc, uint32_t Instr) {
-  write32(Loc - (Config->EKind == ELF64BEKind ? 2 : 0), Instr);
+static void writeFromHalf16(uint8_t *Loc, uint32_t Insn) {
+  write32(Config->IsLE ? Loc : Loc - 2, Insn);
 }
 
-static uint32_t readInstrFromHalf16(const uint8_t *Loc) {
-  return read32(Loc - (Config->EKind == ELF64BEKind ? 2 : 0));
+static uint32_t readFromHalf16(const uint8_t *Loc) {
+  return read32(Config->IsLE ? Loc : Loc - 2);
 }
 
 PPC64::PPC64() {
   GotRel = R_PPC64_GLOB_DAT;
   NoneRel = R_PPC64_NONE;
   PltRel = R_PPC64_JMP_SLOT;
   RelativeRel = R_PPC64_RELATIVE;
   IRelativeRel = R_PPC64_IRELATIVE;
void PPC64::relaxGot(uint8_t *Loc, RelType Type, uint64_t Val) const {
   switch (Type) {
   case R_PPC64_TOC16_HA:
     // Convert "addis reg, 2, .LC0@toc@h" to "addis reg, 2, var@toc@h" or "nop".
     relocateOne(Loc, Type, Val);
     break;
   case R_PPC64_TOC16_LO_DS: {
     // Convert "ld reg, .LC0@toc@l(reg)" to "addi reg, reg, var@toc@l" or
     // "addi reg, 2, var@toc".
-    uint32_t Instr = readInstrFromHalf16(Loc);
-    if (getPrimaryOpCode(Instr) != LD)
+    uint32_t Insn = readFromHalf16(Loc);
+    if (getPrimaryOpCode(Insn) != LD)
       error("expected a 'ld' for got-indirect to toc-relative relaxing");
-    writeInstrFromHalf16(Loc, (Instr & 0x03FFFFFF) | 0x38000000);
+    writeFromHalf16(Loc, (Insn & 0x03FFFFFF) | 0x38000000);
     relocateOne(Loc, R_PPC64_TOC16_LO, Val);
     break;
   }
   default:
     llvm_unreachable("unexpected relocation type");
   }
 }
 
void PPC64::relaxTlsGdToLe(uint8_t *Loc, RelType Type, uint64_t Val) const {
   // Relaxing to local exec entails converting:
   // addis r3, r2, x@got@tlsgd@ha    into      nop
   // addi  r3, r3, x@got@tlsgd@l     into      addis r3, r13, x@tprel@ha
   // bl __tls_get_addr(x@tlsgd)      into      nop
   // nop                             into      addi r3, r3, x@tprel@l
 
   switch (Type) {
   case R_PPC64_GOT_TLSGD16_HA:
-    writeInstrFromHalf16(Loc, 0x60000000); // nop
+    writeFromHalf16(Loc, 0x60000000); // nop
     break;
   case R_PPC64_GOT_TLSGD16:
   case R_PPC64_GOT_TLSGD16_LO:
-    writeInstrFromHalf16(Loc, 0x3c6d0000); // addis r3, r13
+    writeFromHalf16(Loc, 0x3c6d0000); // addis r3, r13
     relocateOne(Loc, R_PPC64_TPREL16_HA, Val);
     break;
   case R_PPC64_TLSGD:
     write32(Loc, 0x60000000);     // nop
     write32(Loc + 4, 0x38630000); // addi r3, r3
     // Since we are relocating a half16 type relocation and Loc + 4 points to
     // the start of an instruction we need to advance the buffer by an extra
     // 2 bytes on BE.
void PPC64::relaxTlsLdToLe(uint8_t *Loc, RelType Type, uint64_t Val) const {
   // Relaxing to local exec entails converting:
   // addis r3, r2, x@got@tlsld@ha   into      nop
   // addi  r3, r3, x@got@tlsld@l    into      addis r3, r13, 0
   // bl __tls_get_addr(x@tlsgd)     into      nop
   // nop                            into      addi r3, r3, 4096
 
   switch (Type) {
   case R_PPC64_GOT_TLSLD16_HA:
-    writeInstrFromHalf16(Loc, 0x60000000); // nop
+    writeFromHalf16(Loc, 0x60000000); // nop
     break;
   case R_PPC64_GOT_TLSLD16_LO:
-    writeInstrFromHalf16(Loc, 0x3c6d0000); // addis r3, r13, 0
+    writeFromHalf16(Loc, 0x3c6d0000); // addis r3, r13, 0
     break;
   case R_PPC64_TLSLD:
     write32(Loc, 0x60000000);     // nop
     write32(Loc + 4, 0x38631000); // addi r3, r3, 4096
     break;
   case R_PPC64_DTPREL16:
   case R_PPC64_DTPREL16_HA:
   case R_PPC64_DTPREL16_HI:
case R_PPC64_TPREL16:
     checkInt(Loc, Val, 16, OriginalType);
     write16(Loc, Val);
     break;
   case R_PPC64_ADDR16_DS:
   case R_PPC64_TPREL16_DS: {
     checkInt(Loc, Val, 16, OriginalType);
     // DQ-form instructions use bits 28-31 as part of the instruction encoding
     // DS-form instructions only use bits 30-31.
-    uint16_t Mask = isDQFormInstruction(readInstrFromHalf16(Loc)) ? 0xF : 0x3;
+    uint16_t Mask = isDQFormInstruction(readFromHalf16(Loc)) ? 0xF : 0x3;
     checkAlignment(Loc, lo(Val), Mask + 1, OriginalType);
     write16(Loc, (read16(Loc) & Mask) | lo(Val));
   } break;
   case R_PPC64_ADDR16_HA:
   case R_PPC64_REL16_HA:
   case R_PPC64_TPREL16_HA:
     if (Config->TocOptimize && ShouldTocOptimize && ha(Val) == 0)
-      writeInstrFromHalf16(Loc, 0x60000000);
+      writeFromHalf16(Loc, 0x60000000);
     else
       write16(Loc, ha(Val));
     break;
   case R_PPC64_ADDR16_HI:
   case R_PPC64_REL16_HI:
   case R_PPC64_TPREL16_HI:
     write16(Loc, hi(Val));
     break;
case R_PPC64_TPREL16_HIGHESTA:
     break;
   case R_PPC64_ADDR16_LO:
   case R_PPC64_REL16_LO:
   case R_PPC64_TPREL16_LO:
     // When the high-adjusted part of a toc relocation evalutes to 0, it is
     // changed into a nop. The lo part then needs to be updated to use the
     // toc-pointer register r2, as the base register.
     if (Config->TocOptimize && ShouldTocOptimize && ha(Val) == 0) {
-      uint32_t Instr = readInstrFromHalf16(Loc);
-      if (isInstructionUpdateForm(Instr))
+      uint32_t Insn = readFromHalf16(Loc);
+      if (isInstructionUpdateForm(Insn))
         error(getErrorLocation(Loc) +
               "can't toc-optimize an update instruction: 0x" +
-              utohexstr(Instr));
-      Instr = (Instr & 0xFFE00000) | 0x00020000;
-      writeInstrFromHalf16(Loc, Instr);
-    }
+              utohexstr(Insn));
+      writeFromHalf16(Loc, (Insn & 0xFFE00000) | 0x00020000 | lo(Val));
+    } else {
       write16(Loc, lo(Val));
+    }
     break;
   case R_PPC64_ADDR16_LO_DS:
   case R_PPC64_TPREL16_LO_DS: {
     // DQ-form instructions use bits 28-31 as part of the instruction encoding
     // DS-form instructions only use bits 30-31.
-    uint32_t Inst = readInstrFromHalf16(Loc);
-    uint16_t Mask = isDQFormInstruction(Inst) ? 0xF : 0x3;
+    uint32_t Insn = readFromHalf16(Loc);
+    uint16_t Mask = isDQFormInstruction(Insn) ? 0xF : 0x3;
     checkAlignment(Loc, lo(Val), Mask + 1, OriginalType);
     if (Config->TocOptimize && ShouldTocOptimize && ha(Val) == 0) {
       // When the high-adjusted part of a toc relocation evalutes to 0, it is
       // changed into a nop. The lo part then needs to be updated to use the toc
       // pointer register r2, as the base register.
-      if (isInstructionUpdateForm(Inst))
+      if (isInstructionUpdateForm(Insn))
         error(getErrorLocation(Loc) +
               "Can't toc-optimize an update instruction: 0x" +
-              Twine::utohexstr(Inst));
-      Inst = (Inst & 0xFFE0000F) | 0x00020000;
-      writeInstrFromHalf16(Loc, Inst);
-    }
+              Twine::utohexstr(Insn));
+      Insn &= 0xFFE0000F | Mask;
+      writeFromHalf16(Loc, Insn | 0x00020000 | lo(Val));
+    } else {
       write16(Loc, (read16(Loc) & Mask) | lo(Val));
+    }
   } break;
   case R_PPC64_ADDR32:
   case R_PPC64_REL32:
     checkInt(Loc, Val, 32, Type);
     write32(Loc, Val);
     break;
   case R_PPC64_ADDR64:
   case R_PPC64_REL64:
void PPC64::relaxTlsGdToIe(uint8_t *Loc, RelType Type, uint64_t Val) const {
   case R_PPC64_GOT_TLSGD16_HA:
     // This is relaxed from addis rT, r2, sym@got@tlsgd@ha to
     //                      addis rT, r2, sym@got@tprel@ha.
     relocateOne(Loc, R_PPC64_GOT_TPREL16_HA, Val);
     return;
   case R_PPC64_GOT_TLSGD16_LO: {
     // Relax from addi  r3, rA, sym@got@tlsgd@l to
     //            ld r3, sym@got@tprel@l(rA)
-    uint32_t InputRegister = (readInstrFromHalf16(Loc) & (0x1f << 16));
-    writeInstrFromHalf16(Loc, 0xE8600000 | InputRegister);
+    uint32_t RA = (readFromHalf16(Loc) & (0x1f << 16));
+    writeFromHalf16(Loc, 0xE8600000 | RA);
     relocateOne(Loc, R_PPC64_GOT_TPREL16_LO_DS, Val);
     return;
   }
   case R_PPC64_TLSGD:
     write32(Loc, 0x60000000);     // bl __tls_get_addr(sym@tlsgd) --> nop
     write32(Loc + 4, 0x7c636A14); // nop --> add r3, r3, r13
     return;
   default: