Fix tan returning -nan for inputs where abs(x) >= 2^63

[pineapplemachine]

The fptan instruction pushes a 1.0 onto the FPU register stack after a successful operation, but when abs(input) >= 2^63 the C2 flag is set to indicate that the input was out of bounds, and it doesn't push the 1.0.

Prior to this PR, the top value of the FPU stack was popped irrespective of whether C2 was set, which in the case of an out-of-bounds input caused the input to be removed from the stack and ultimately resulted in an incorrect return value of -nan. This PR changes this behavior, only popping after fptan when C2 was not set.

See: http://x86.renejeschke.de/html/file_module_x86_id_109.html

[pineapplemachine]

Attn @WalterBright who I assume wrote the original asm?

[WebDrake]

git blame suggests it was @donc who wrote the original implementation. Don, can you advise?

[andralex]

cc @ibuclaw @redstar @klickverbot

[ibuclaw]

Missing unittests?

[ibuclaw]

We should probably document what is the domain of this function, as well as others. And what the behaviour should be when it is too far outside that range - I'd have to do some looking up on that though.

[pineapplemachine]

@ibuclaw on that topic it's worth noting that expi suffers from a similar issue, though in that case it's due to having no handling for the case of an out-of-range input rather than having incorrect handling. An example of correctly handling out-of-range inputs to fsincos is here: https://github.com/pineapplemachine/mach.d/blob/master/mach/math/trig/sincos.d#L77

I added unit tests to verify the new behavior. The output is imprecise for such large inputs, but I think imprecision is preferable to NaN.

[ibuclaw]

Yeah, I just looked at the cephes code that I used for reference for the generic implementations (gdc uses this). It says that it should return 0.0 if x > 2^39 for 80-bit, and x > 2^30 for 64-bit reals.

[pineapplemachine]

It's worth noting that the documentation here suggests using fprem to bring the input into range when it's outside of it, which is what the implementation was unsuccessfully doing before and should correctly do now.

Either way is fine by me, but if my change isn't the desired behavior I'll leave it up to someone else to make it right.

[pineapplemachine]

This is a straightforward bugfix that does not alter functionality except fixing the presumed undesirable case of returning NaN for large positive or negative inputs, in which cases the result may not be accurate but is still well-defined. The functionality change made by this PR was clearly the intended functionality when the implementation was written. The diff is tiny; this is the least intrusive conceivable change that will address this bug.

I suggest that this PR should be merged and, if there are any decisions made later about the desired behavior of the tangent function for large positive or negative inputs, then those should be addressed in a separate PR and not withhold the correction of what is obviously incorrect and unexpected behavior.

[wilzbach]

that will address this bug.

Please add a bugzilla issue and link it with this PR, so that the change is visible in the changelog.
Basically it's just a git commit message starting with "Fix" and then followed by the bugzilla title. More infos:

https://github.com/dlang-bots/dlang-bot

I suggest that this PR should be merged

CIs need to pass first - you seem to have trailing whitespace. Do you need help fixing this?

[don-clugston-sociomantic]

git blame suggests it was @donc who wrote the original implementation. Don, can you advise?

It actually wasn't written by me, the code dates back to at least DMD 0.043, it was probably written by Walter in 2003 or earlier, maybe even in the 80's.
LGTM.

[JackStouffer]

@pineapplemachine With don signing off on this, just fix the whitespace issue and I'll pull.

[pineapplemachine]

The spaces have been nuked

[pineapplemachine]

Also added an issue to bugzilla: https://issues.dlang.org/show_bug.cgi?id=17562

[PetarKirov]

Auto-merging since Don gave LGTM.
I've rebased the PR in order to squash the commits and to add the number of the fixed Bugzilla issue to the commit message.

[wilzbach]

@ZombineDev

You forgot to approve the PR:

2017-06-27T20:32:35.218672+00:00 app[web.1]: [275DAFDA:2093F7DA WRN] PUT https://api.github.com/repos/dlang/phobos/pulls/5114/merge failed;  Method Not Allowed 405.
2017-06-27T20:32:35.218685+00:00 app[web.1]: [275DAFDA:2093F7DA WRN] {"message":"At least one approved review is required by reviewers with write access.","documentation_url":"https://help.github.com/articles/about-protected-branches"}

I think the bot could/should warn about this, see also:
dlang/dlang-bot#104 or dlang/dlang-bot#108

[dlang-bot]

Thanks for your pull request, @pineapplemachine! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

• smaller, focused PRs are easier to review than big ones

• try not to mix up refactoring or style changes with bug fixes or feature enhancements

• provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Fix	Bugzilla	Description
✓	17562	Tangent function returns NaN for abs(input) >= 2^63

[wilzbach]

Closed/reopened to show that the bot recognized the issue (not sure what went wrong with the first hook -I have enabled more logging (I have opened a PR to enable more logging: dlang/dlang-bot#105)

Auto-merging since Don gave LGTM.

Hehe, our bot requires this formally ;-)

[ibuclaw]

Just saw this, was the cephes (non x86 as) path tested? If not the gdc and ldc gods will come for you. 😉