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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai 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 |
----------------
Is this the common practice in compiler-rt for detecting NaN's vs something like `isnan()`? 


================
Comment at: compiler-rt/lib/builtins/ppc/fixunstfti.c:27
+  DD ldi; /* The long double representation, with high and low portions. */
+  int hi_expo, lo_expo, shift;
+  unsigned long long tmp_result;
----------------
Do we have a different coding guideline for compiler-rt in terms of naming variables?


================
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;
----------------
s/minusing/subtracting


================
Comment at: compiler-rt/lib/builtins/ppc/fixunstfti.c:60
+  } else {
+    /* Detect edge cases. */
+    if (hi_bits.d > 0) {
----------------
Please state what the edge cases are for both of the conditions. It is not at all clear to the reader why we will saturate the value when the exponent in the high double is above 128 and the sign bit isn't set. Similarly for the opposite case where we seem to produce `1`.


================
Comment at: compiler-rt/lib/builtins/ppc/fixunstfti.c:75
+    /* If the double does not fit within 64 bits, scale it to fit in int128. */
+    shift = lo_expo - 54; /* Calculate the amount to scale the double by. */
+    lo_bits.x &= 0x800fffffffffffffll; /* Clear all of the exponent bits. */
----------------
This code seems like it's just a repeat of what we do for the bits in the high `double` above. Can we common this up into a macro?


================
Comment at: compiler-rt/test/builtins/Unit/ppc/fixunstfti_test.h:14
+};
+
+struct testCase testList[] = {
----------------
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.


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

https://reviews.llvm.org/D54911