[PATCH] D81457: [LLD][PowerPC] Add support for R_PPC64_PCREL34

Mon Jun 15 09:46:50 PDT 2020

nemanjai added inline comments.

================
Comment at: lld/ELF/Arch/PPC64.cpp:382
+// pieces separately and assemble/disassemble them accordingly.
+static void writePrefixedInstruction(uint8_t *loc, uint64_t insn) {
+  insn = config->isLE ? insn << 32 | insn >> 32 : insn;
----------------
stefanp wrote:
> sfertile wrote:
> > Can I ask the rational on why we want to treat a prefixed instruction as a single 8-byte instruction as opposed to a 4-byte prefix and a 4-byte instruction?  I'm not necessarily disagreeing we should treat them as a single entity, I just want to make sure we have though about what the best representation is considering all the new relocations to be added.
> Good question. I like to treat them as one because conceptually they are one instruction. So, in my mind I tend to think of them as 8 byte instructions. 
> Some reasons I tend to think of them this way:
> 1) We can have the instruction part without the prefix but we can never have a prefix without a following instruction to go with it.
> 2) Immediates are represented across the boundary of the prefix/instruction. For example `paddi` has 18 bits of immediate in the prefix and 16 bits of the same immediate in the instruction. Those two are concatenated together to form the actual immediate. 
> 3) Flags in the prefix change the meaning of the subsequent instruction. Think of the PC Relative flag.
> 4) In llc we treat them as one instruction (I know this is a bit circular...)
> For example in `PPCInstrPrefix.td`we have the base class for all prefixed instructions:
> ```
> // Top-level class for prefixed instructions.
> class PI<bits<6> pref, bits<6> opcode, dag OOL, dag IOL, string asmstr,
>          InstrItinClass itin> : Instruction {
>   field bits<64> Inst;
>   field bits<64> SoftFail = 0;
>   bit PCRel = 0; // Default value, set by isPCRel.
>   let Size = 8;
> ```
> 
> In the end the reason is purely conceptual from my perspective. I don't have a technical reason to treat them as one.
Not that it makes a significant difference, but also one store vs. two.

================
Comment at: lld/ELF/Arch/PPC64.cpp:1011
+    uint64_t si1 = (val & si1Mask);
+    uint64_t instr = readPrefixedInstruction(loc) & ~fullMask;
+
----------------
MaskRay wrote:
> stefanp wrote:
> > MaskRay wrote:
> > > sfertile wrote:
> > > > I've got a patch which removes the masking of the instruction bits on the other relocations types we were doing it for [[ https://reviews.llvm.org/D81106 | (D81106)]].  My understanding is that for REL ELF archs the addend is encoded into the bits to be relocated, and for RELA ELF archs the bits to be relocated are zeros, with the addend in the relocation. The extra masking doesn't hurt anything functionally but it can lead to misunderstandings on what the bits to be relocated should  [[ https://reviews.llvm.org/D81082 | contain]] so I think its important to be precise and omit the extra masking despite it not hurting anything. 
> > > Even if powerpc is a RELA target, if it is easy to drop the requirement, I hope we can make some efforts making that work. See D80496 (-z rel)
> > I do see what you mean. The way I look at this is that we have 3 options:
> > 1) We just ignore it and assume that the zeros are there. This is what you are suggesting. However, my worry is that this way we could be silently producing bad code. Basically if there is a garbage bit in there we end up with an "or" of that bad bit and a valid offset and we sometimes end up with a bad offset.
> > 2) We mask it out. This is what I did. It basically hides the problem. If there are bad bits they get zeroed out and we end up with a valid instruction. We will never know that the initial encoding is wrong.
> > 3) We assert on those bits. This means that if there are non-zero bits in the encoding we don't even link the code. I don't want to do this last one mainly because GCC doesn't do it. I know, it is not necessarily a valid reason to do this.
> > I had originally picked option 2 because that is what GCC did. It produces the correct output despite the garbage I put in there. Again, just because GCC does this it doesn't mean we should.
> > 
> > 
> > With respect to the second comment:
> > At this point I'm not going to try to change this to support REL yet. I'm just looking at RELA. If we want to add that support I would say doing it in a completely different patch.
> I changed my mind on D81106.
> 
> > 2. We mask it out. This is what I did. It basically hides the problem. If there are bad bits they get zeroed out and we end up with a valid instruction. We will never know that the initial encoding is wrong.
> 
> This one looks good to me. Why does it "hide the problem"? powerpc{,64} are RELA targets. For a RELA target, the implicit addend should not matter. FWIW, AArch64.cpp marks out the bits as well.
I too am in favour of masking out the bits. This is what `ld.bfd` does. It doesn't hurt performance. It is conservatively correct with minimal overhead.

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

https://reviews.llvm.org/D81457