[PATCH] D37615: [AVR] implement the relocation of LDI and other Instructions for LLD

[Dylan McKay via Phabricator via llvm-commits]

dylanmckay added inline comments.


================
Comment at: ELF/Arch/AVR.cpp:45
 namespace {
+uint16_t caculateForLDI(uint16_t X, uint16_t SRel) {
+  return (X & 0xf0f0) | (SRel & 0xf) | ((SRel << 4) & 0xf00);
----------------
`s/caculate/calculate`


================
Comment at: ELF/Arch/AVR.cpp:98
+  case R_AVR_7_PCREL:
+    SRel -= 2;
+    X = (X & 0xfc07) | (((SRel >> 1) << 3) & 0x3f8);
----------------
I think we should make this into a method - all PC-relative instructions need to be adjusted by `-2` IIRC to account for the size of the current instruction.

We can use this function in the other cases in this method, and also add a comment to the new method itself describing that all instructions that are pc-relative are 16-bit instructions, which is why it is `2` and not `4`.

Consider the equivalent code in LLVM itself [here](https://github.com/llvm-mirror/llvm/blob/master/lib/Target/AVR/MCTargetDesc/AVRAsmBackend.cpp#L255), which is very readable


================
Comment at: ELF/Arch/AVR.cpp:104
+    SRel -= 2;
+    SRel >>= 1;
+    X = (X & 0xf000) | (SRel & 0xfff);
----------------
Can we move the rightshifting functionality out into a separate method with a comment? 

This is required because all instructions are 2 or 4 bytes, and so every single instruction resides at an even byte offset (implies bit 0 is always 0). Because of this, AVR addresses in these instructions are rightshifted by one to gain an extra addressing bit.


================
Comment at: ELF/Arch/AVR.cpp:110
+  case R_AVR_LDI:
+    write16le(Loc, caculateForLDI(X, SRel));
+    break;
----------------
I believe we need to mask out all the upper bits for `LO8`. 


================
Comment at: ELF/Arch/AVR.cpp:150
+  case R_AVR_LO8_LDI_PM:
+    SRel = (SRel >> 9) & 0xff;;
+    write16le(Loc, caculateForLDI(X, SRel));
----------------
Double semicolon


================
Comment at: ELF/Arch/AVR.cpp:176
+  case R_AVR_16:
+  case R_AVR_16_PM:
+    write16le(Loc, (SRel >> 1) & 0x00ffff);
----------------
I think there's a difference between these two relocations


Repository:
  rL LLVM

https://reviews.llvm.org/D37615