[PATCH] D27479: ELF/AArch64: Refactor R_AARCH64_LDST{8, 15, 32, 64, 128}_ABS_LO12_NC Relocations

[Adhemerval Zanella via Phabricator via llvm-commits]

zatrazz added inline comments.


================
Comment at: ELF/Target.cpp:1329-1335
+// Return the bits [Start, End] from Val shifted Start bits.  For instance,
+// getBits<4,8>(0xF0) returns 0xF.
+template <uint8_t Start, uint8_t End>
+static uint64_t getBits(uint64_t Val) {
+  uint64_t Mask = ((uint64_t)1 << ((End+1) - Start))-1;
+  return (Val >> Start) & Mask;
+}
----------------
ruiu wrote:
> Maybe we should add this to llvm/Support/MathExtras.h? We have a bunch of bit manipulation functions there.
> 
> I don't see a reason to make it a template. Start and End can be passed as arguments.
> 
> Please run clang-format-diff to format in the LLVM style.
> 
> You don't need a parentheses around `(End + 1)` because `End - Start + 1` is the same as `(End + 1) - Start`.
I do not have a strong preference where to add it, although I see this implementation kinda specialized as it extracts some bits and also shift to a value (not only apply a mask).

And I made it a template so compiler have a better time optimizing it depending of the mask. For instance on x86_64, getBits<0, 11> could optimized to just:

        movq    %rdi, %rax
        andl    $4095, %eax

While the argument passing counterpart would have much more instructions.

I will rerun clang-format-diff and remove the parentheses.


================
Comment at: ELF/Target.cpp:1361
   case R_AARCH64_ADD_ABS_LO12_NC:
-    updateAArch64Add(Loc, Val);
+    updateAArch64LdStrAdd(Loc, Val);
     break;
----------------
ruiu wrote:
> This code is not the same as before. Was the previous code wrong?
Just the function name changed for this snippet, from updateAArch64Add to updateAArch64LdStrAdd (since I am now using it to update the immediate field to load/store instructions as well).


Repository:
  rL LLVM

https://reviews.llvm.org/D27479