Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Problem with Absolute Value of Floating Point PARAM #24119

Closed
damianmoz opened this issue Dec 24, 2023 · 5 comments
Closed

Problem with Absolute Value of Floating Point PARAM #24119

damianmoz opened this issue Dec 24, 2023 · 5 comments

Comments

@damianmoz
Copy link

damianmoz commented Dec 24, 2023

Beyond the 5 basic mathematical operations, addition, subtraction, multiplication, division and square root which can affect IEEE 754 exceptions, there are also simple operations which never raise an IEEE 754 exception

copy(source)
negate(course)
abs(source)

In Chapel, the first two are implemented for params. Because Chapel nominally implements the last with a routine from the system matrhematical library, it cannot return the absolute value of a param as a param. One can argue that this is an oversight because the compiler can figure that out for itself for a param argument and when the argument is a const the optimizer will most likely on modern hardware discard any reference to the implied routine and implement the operation as a single machine instruction.

inline proc abs(param x : real(?w)) param do if x < 0 then -x else x;

I am not suggesting that this implementation is the way to go. It just provides a workaround.

@damianmoz
Copy link
Author

Technically there is a fourth simple operation which is the assignment to the absolute value of one source of the negative (or sign) bit of a second source, nominally called copysign(). But I did not want to complicate the discussion.

@damianmoz damianmoz changed the title Problematic Absolute Value Problem with Magnitude (Absolute Value) of Floating Point PARAM Dec 24, 2023
@damianmoz damianmoz changed the title Problem with Magnitude (Absolute Value) of Floating Point PARAM Problem with Absolute Value of Floating Point PARAM Dec 24, 2023
@mppf
Copy link
Member

mppf commented Jan 2, 2024

@damianmoz - thanks for filing the issue. I've created PR #24127 to add the missing param overloads for real/imag abs. I understand that you'd also like to have abs on a param complex return a param. That one is a bit more complicated to implement so I created issue #24125 to record it as a feature request. Please feel free to comment there (or here) about the urgency of that feature.

@damianmoz
Copy link
Author

Not urgent. I figure that param complex(w) is a lot more difficult and will take even longer. It was more that I noticed the issue across the break and I figured that Chapel needed this completeness of features across all of its floating point and integral types eventually.

@bradcray
Copy link
Member

@damianmoz / @mppf: Is this issue now complete other than the abs on param complex returning complex, now spun off to #24125? If so, is it OK if I close this one in favor of that one?

@damianmoz
Copy link
Author

Yes (as far as I can tell) to the first question and Yes to the second question.

mppf added a commit that referenced this issue Jan 17, 2024
For issue #24119.
Resolves #24125.

This PR adds overloads to AutoMath to implement `param` versions of:
 * `abs` for all numeric types
 * `sqrt` for floating-point types.
 
Additionally, while testing, I realized that `complex.re` and
`complex.im` did not have `param` returning versions, so this PR also
adds such param-returning versions to ChapelBase.

To implement these, I:
* added `PRIM_SQRT` and `PRIM_ABS`. For now, these are just used for
computing these functions on `param`s and they will cause the compiler
to error out one makes it to code-generation time.
* implemented folding for `PRIM_ABS`, `PRIM_SQRT`, `PRIM_GET_REAL`, and
`PRIM_GET_IMAG` in num.cpp
* enabled `postFoldPrimop` to fold `PRIM_ABS`, `PRIM_SQRT`,
`PRIM_GET_REAL`, and `PRIM_GET_IMAG`

This PR also updates the spec description of `complex` to make it
clearer. While there, it removes `proc complex.bits` from the spec,
since this method does not exist other than in the spec.

Reviewed by @DanilaFe - thanks!

- [x] full comm=none testing

Future Work:
 * #22809
* in particular, resolve two confusing errors I posted as comments based
on work on this PR
* update the dyno resolver to assert `PRIM_ABS`, `PRIM_SQRT` are only
used on `param` values
* update the dyno resolver to implement `PRIM_GET_REAL` and
`PRIM_GET_IMAG` on `param` values
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants