mgorny added inline comments.
================ Comment at: lib/builtins/floattitf.c:65 + if (a & ((tu_int)1 << LDBL_MANT_DIG)) { + a >>= 1; + ++e; ---------------- scanon wrote: > mgorny wrote: > > scanon wrote: > > > mgorny wrote: > > > > scanon wrote: > > > > > Strictly speaking there's no need to adjust `a` here. If we rounded > > > > > up into a new binade, then `a` is necessarily `0b1000...0`, and the > > > > > leading 1 bit will get killed by the mask when we assemble > > > > > `fb.u.high.all` regardless of its position. Same comment applies to > > > > > floatuntitf. > > > > I'm sorry but I don't feel confident changing that. AFAIU if the > > > > LDBL_MANT_DIG+1 bit is set, this code shifts it lower, so it won't > > > > actually be killed by the mask. > > > In binary128, as in all IEEE 754 binary interchange format encodings, the > > > leading bit of the significand is implicit. The only way to end up in > > > this code path is `0b111...1` rounding up to `0b100...00`, meaning that > > > the significand is 1.0, which is stored as all-zeros (i.e. the leading > > > bit is necessarily masked). > > > > > > To be more explicit, LDBL_MANT_DIG is 113. If this shift happens, after > > > the shift bit 112 is set, and bits 111:0 are zero. The mask `((a >> 64) & > > > 0x0000ffffffffffffLL)` discards bit 112 (= 64 + 48). > > Well, I've tried removing this and it causes one of the tests to fail: > > > > `error in __floatuntitf(0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF) = 0X1P+127, > > expected 0X1P+128` > This sounds like you removed the exponent adjustment (`++e`) as well as (or > instead of) the shift (`a >>= 1`). Without seeing the change, I can't be > certain, of course. Yes, I did that. Now that you state that plainly, I finally understand what you meant ;-). However, if I were to do that, I would rather to do that separately, either in all relevant files or post unifying the code into one logical .inc. This would keep the history of changes cleaner. https://reviews.llvm.org/D27898 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits