[PATCH] D158197: [PowerPC][lld] Account for additional X-Forms -> D-Form/DS-Forms load/stores when relaxing initial-exec to local-exec

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: lld/ELF/Arch/PPC64.cpp:944
+          error("unrecognized instruction for IE to LE R_PPC64_TLS");
+        finalInstr = (dFormOp | (read32(loc) & 0x03FFFFFC));
+        finalReloc = R_PPC64_TPREL16_LO_DS;
----------------
amyk wrote:
> nemanjai wrote:
> > Using `0x03FFFFFC` rather than `0x03FFFFFF` is presumably because the two least significant bits of the DS-Form must not be overwritten. While I think that makes sense, I have a bit of a concern with using a different value there:
> > 1. We always had DS-Forms there (`LD/STD`) and didn't treat them specially
> > 2. Presumably, having either of those bits set in the incoming instruction is an indication that something has gone wrong with whatever produced the code, so masking out the bits just hides the issue
> > 
> > Perhaps it would be appropriate to set the `finalInstr` the same way regardless of how we set the `dFormOp` and just have an assert (or possibly a fatal_error or warning is even better) in the DS-Form section that those bits are unset.
> 
> 
> I'd like to understand this suggestion a bit more.
> 
> Do you mean, we should just do:
> ```
> if (dFormOp == 0) { // Expecting a DS-Form instruction.
>   dFormOp = getPPCDSFormOp(secondaryOp);
>   if (dFormOp == 0)
>     error("unrecognized instruction for IE to LE R_PPC64_TLS");
>         
>     // Check if the last two bits are unset. If set, we `errorOrWarn()`
> 
>     finalReloc = R_PPC64_TPREL16_LO_DS;
> } else
>     finalReloc = R_PPC64_TPREL16_LO;
>   write32(loc, (dFormOp | (read32(loc) & 0x03FFFFFF)));
>   relocateNoSym(loc + offset, finalReloc, val);
> }
> ```
> 
> In any case, yes, if I understand correctly, `0x03FFFFFF` grabs bits 6-31 in the X-Form instruction, while `0x03FFFFFC` grabs bits 6-29, leaving the last two bits. 
> 
> Also, to give a bit more context behind why I am treating the DS-Forms differently:
> 
> I discussed the use of use of `0x03FFFFFC` in the DS-Form case with a few others before putting up the patch. 
> This all came about because I was using a different relocation for DS-Form `R_PPC64_TPREL16_LO_DS`.
> I found that when I used the different relocation with the original masking in the DS-Form case:
> ```
>   write32(loc, (dFormOp | (read32(loc) & 0x03FFFFFF)));
>   relocateNoSym(loc + offset, R_PPC64_TPREL16_LO_DS, val);
> ```
> 
> For the following situation:
> ```
> test8:
>   addis 4, 2, l at got@tprel at ha
>   ld 4, l at got@tprel at l(4)
>   stdx 3, 4, l at tls
> 
> .section .tdata,"awT", at progbits
> .globl l
> l:
> .quad 55
> ```
> I'd get `stq` instead of `std` like we would expect.
> ```
> Disassembly of section .text:
> 
> 0000000010010200 <test8>:
> 10010200:      	nop
> 10010204:      	addis 4, 13, 0
> 10010208:      	stq 3, -28672(4)
> ```
> Presumably this is because `std` and `stq` share the same primary opcode, and the other thing that differs are the last two bits (00 for `std` whereas 10 for `stq`), so we found that `0x03FFFFFC` works with `R_PPC64_TPREL16_LO_DS`.
> 
> I don't quite remember the exact discussion that we also had, but I think we were saying before that `LD/STD` perhaps just happened to have worked when we did `0x03FFFFFF` with `R_PPC64_TPREL16_LO` in the past, since the last two bits in these instructions are both 00.
> 
> 
That `stq` vs. `std` issue is a bit alarming. Where do the low bits come from? The variable `l` is aligned at 8 bytes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158197/new/

https://reviews.llvm.org/D158197