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

Thu Jan 3 05:23:03 PST 2019

amyk marked 5 inline comments as done.
amyk added inline comments.

================
Comment at: compiler-rt/lib/builtins/ppc/floattitf.c:41
+  long double ldArgHiPart = __floatditf(argHiPart);
+  long double ldArgLoPart = __floatunditf(argLoPart);
+
----------------
nemanjai wrote:
> 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.
Yeah. that is a good point. I will change the low part to unsigned, and also rename the variables a little to adhere to LLVM naming conventions. 

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

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

https://reviews.llvm.org/D54313