device-libs: Split nonfinite edge case handling of fract usage#1587
device-libs: Split nonfinite edge case handling of fract usage#1587
Conversation
| double frac = BUILTIN_FRACTION_F64_IMPL(BUILTIN_FLDEXP_F64(f2, -2)); | ||
| double frac_fixup = BUILTIN_FRACTION_F64_FIXUP(frac, x); | ||
|
|
||
| f2 = BUILTIN_FLDEXP_F64(frac_fixup, 2); |
There was a problem hiding this comment.
No need to split this into 3 statements and add 2 new variables. A single line like the original is better.
There was a problem hiding this comment.
Disagree. It's not like there's a statement and variable count budget. Temporary variables help readability.
There was a problem hiding this comment.
In some cases that may be true, but not here. Please write this as a single statement.
arsenm
force-pushed
the
device-libs/split-f64-fract-finite-only-check
branch
from
f22a444 to
10333a0
Compare
| EVALUATE(x, p2, p1, p0, f2, f1, f0); | ||
|
|
||
| f2 = BUILTIN_FLDEXP_F64(BUILTIN_FRACTION_F64(BUILTIN_FLDEXP_F64(f2, -2)), 2); | ||
| f2 = BUILTIN_FLDEXP_F64(BUILTIN_FRACTION_F64_FIXUP(BUILTIN_FRACTION_F64_IMPL(BUILTIN_FLDEXP_F64(f2, -2)), x), 2); |
There was a problem hiding this comment.
Why is it x here and not f2? The original implementation tested f2.
There was a problem hiding this comment.
This is the whole point of doing this. x is the original argument, which is trivially provable to not be infinity or nan. The above computation cannot introduce an infinity or nan. We know that isinf(x) == isinf(f2) at this point, so we can do the special case checks on the original argument
Break the builtin macro usage into the main sequence and the edge case fixup part. Perform the edge case checks on the original argument value, rather than the result of the earlier computation. We know this sequence cannot introduce inf or nan. This enables value tracking to trivially use the knowledge that the argument isn't inf or nan. The current sequence is 2 instructions too deep for the default value tracking recursion depth limit. This eliminates codegen differences in the remaining trig functions when FINITE_ONLY_OPT is deleted.
arsenm
force-pushed
the
device-libs/split-f64-fract-finite-only-check
branch
from
10333a0 to
52ea735
Compare
|
ping |
b-sumner
left a comment
b-sumner
left a comment
There was a problem hiding this comment.
Sigh. Splitting the fraction like this is just ugly and feels like a workaround.
|
!PSDB |
|
PSDB Build Link: http://mlse-bdc-20dd129:8065/#/builders/11/builds/24 |
4 participants
Break the builtin macro usage into the main sequence and the edge case fixup part. Perform the edge case checks on the original argument value, rather than the result of the earlier computation. We know this sequence cannot introduce inf or nan. This enables value tracking to trivially use the knowledge that the argument isn't inf or nan. The current sequence is 2 instructions too deep for the default value tracking recursion depth limit.
This eliminates codegen differences in the remaining trig functions when FINITE_ONLY_OPT is deleted.