[PATCH] D54911: [compiler-rt][builtins][PowerPC] Add ___fixunstfti builtin compiler-rt method support for PowerPC

[Amy Kwan via Phabricator via llvm-commits]

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


================
Comment at: compiler-rt/lib/builtins/ppc/fixunstfti.c:20
+  /* If we are trying to convert a NaN, simply return NaN. */
+  if (ld != ld) {
+    return ((__uint128_t) 0x7FF8000000000000ll) << 64 |
----------------
nemanjai wrote:
> Is this the common practice in compiler-rt for detecting NaN's vs something like `isnan()`? 
That is true, there is a `crt_isnan()` that can be used within compiler-rt, so I will use that. 


================
Comment at: compiler-rt/lib/builtins/ppc/fixunstfti.c:42
+   * of the long double, by getting rid of the sign,
+   * isolating the exponent, and minusing the bias from it. */
+  tmp_result = hi_bits.x & 0x7FFFFFFFFFFFFFFFll;
----------------
nemanjai wrote:
> s/minusing/subtracting
Comment has been adjusted. 


================
Comment at: compiler-rt/test/builtins/Unit/ppc/fixunstfti_test.h:14
+};
+
+struct testCase testList[] = {
----------------
nemanjai wrote:
> It might be more readable to define a macro such as:
> `#define INIT_U128(HI, LO) (((__uint128_t) (HI) << 64) | (LO))`
> 
> And use that to initialize the 128-bit int values.
That is a good point; I agree with your suggestion. I will utilize a macro since it also enhances readability. 


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

https://reviews.llvm.org/D54911