[PATCH] D54313: [compiler-rt][builtins][PowerPC] Add floattitf builtin compiler-rt method support for PowerPC

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai accepted this revision.
nemanjai added a comment.

The remaining comments from me are minor. I am OK with this going in if Robert is.



================
Comment at: compiler-rt/lib/builtins/ppc/floattitf.c:35
+  int64_t argHiPart = (int64_t)(arg >> 64);
+  int64_t argLoPart = (int64_t)arg;
+
----------------
`uint64_t` vs `int64_t`?
`ArgLoPart`?


================
Comment at: compiler-rt/lib/builtins/ppc/floattitf.c:41
+  long double ldArgHiPart = __floatditf(argHiPart);
+  long double ldArgLoPart = __floatunditf(argLoPart);
+
----------------
I am not sure if any compiler would emit warnings in pedantic mode for a signed argument being passed to a function that takes an unsigned parameter. But I also don't see a compelling need to have both of these signed. The algorithm described states that the low part is treated as unsigned. Why not just declare it as `uint64_t` as well?

Also, I know that there isn't as much adherence to general LLVM naming conventions in compiler-rt currently, but I don't see why we wouldn't adhere to them on new code.
Seems that these should be along the lines of `ConvertedHiPart`, etc.


================
Comment at: compiler-rt/lib/builtins/ppc/floattitf.c:47
+   * final result. */
+  static const double two64 = 0x1.0p64; /* 2^64 */
+  return ((ldArgHiPart * two64) + ldArgLoPart);
----------------
I don't think we should add this temporary.


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

https://reviews.llvm.org/D54313