Enable abs and sqrt to return a param in more cases #24127

mppf · 2024-01-02T19:13:34Z

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 params 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!

full comm=none testing

Future Work:

Should Chapel consider formal constness when disambiguating calls? #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

damianmoz · 2024-01-02T22:52:22Z

Not urgent

DanilaFe · 2024-01-05T19:13:03Z

modules/standard/AutoMath.chpl

+  proc abs(param x: real(32)) param : real(32) {
+    return if x < 0 then -x else x;
+  }


Floating points are weird, and this worries me a little. Consider the following Python:

>>> float("-0") -0.0 >>> float("-0") < 0 False >>> abs(float("-0")) 0.0

In other words, numerically this differs from the behavior of Python's abs:

>>> def myabs(x): return -x if x < 0 else x ... >>> myabs(float("-0")) -0.0 >>> abs(float("-0")) 0.0

I can wire this up to call an abs primitive in the compiler to run fabs. Do you feel that would address the issue?

Yes, I do -- fabs seems ideal.

mppf · 2024-01-05T22:42:16Z

@DanilaFe - OK, I've added the primitive. From this point it would be trivial to also add param abs support for complex as well as param sqrt support. Do you think I should proceed with those in this PR?

DanilaFe · 2024-01-17T19:58:41Z

Sorry for the delayed response. I think you shouldn't worry about sqrt and complex here -- it can be left for future work (a "good first issue" perhaps?).

DanilaFe · 2024-01-17T20:18:10Z

frontend/lib/resolution/prims.cpp

+primAbsGetType(Context* context, const CallInfo& ci) {
+  QualifiedType ret = QualifiedType();
+
+  if (ci.numActuals() != 1) return ret;
+
+  QualifiedType actualType = ci.actual(0).type();
+  if (auto comp = actualType.type()->toComplexType()) {
+    int w = comp->componentBitwidth();
+    ret = QualifiedType(QualifiedType::CONST_VAR, RealType::get(context, w));
+  } else if (auto img = actualType.type()->toImagType()) {
+    int w = img->bitwidth();
+    ret = QualifiedType(QualifiedType::CONST_VAR, RealType::get(context, w));
+  } else {
+    // otherwise just use the original type
+    ret = QualifiedType(QualifiedType::CONST_VAR, actualType.type());


Note that the production compiler treats ABS and SQRT as param-only primitives. That leads me to think that Dyno should also implement the param logic, and not support the non-param case.

OK, I added some comments along these lines, and will note it in Future Work.

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

The C++ overloading handles the size --- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

bradcray · 2024-01-17T21:58:21Z

Nice, thanks!

Signed-off-by: Vassily Litvinov <vasslitvinov@users.noreply.github.com>

@mppf

This PR reacts to #24127 analogously to #24157, following the same argument in favor of changing the line numbers vs. introducing a prediff. Additionally, it reflects a reduction in the allocation/freeing size of `[domain(1,int(64),one)] int(64)` from 168 to 160 bytes. I did not investigate where that comes from. reviewed, although favored the prediff solution: @mppf

@DanilaFe

Follow-up to PR #24127. After #24127, the new `sqrtparams` test started to fail on Mac OS X system. Upon investigating, I identified that the difference in behavior from what the test expects is due to differences in how `std::sqrt` is implemented for complex numbers in libc++ vs stdlibc++. Also, I noticed that the issue is limited to the C++ implementation of the complex square root. This PR changes the implementation to use a C implementation of complex square root since it appears to have more reliable precision. Note that this C implementation required some workarounds for `CMPLX` not being available on all systems (as with PR #24184 and #24211). In particular, `CMPLX` does not seem to be available when using `clang` on Ubuntu 23.10, even though it is available with `gcc` on that system. I filed an issue against libc++ about the problem, but in discussion there it sounds like complex `sqrt` isn't required to have any particular precision. * llvm/llvm-project#78738 Reviewed by @DanilaFe - thanks! - [x] full comm=none testing

This was referenced Jan 2, 2024

feature request: abs for param complex #24125

Closed

Problem with Absolute Value of Floating Point PARAM #24119

Closed

DanilaFe reviewed Jan 5, 2024

View reviewed changes

mppf force-pushed the param-abs branch from 387f361 to 9244fcd Compare January 17, 2024 19:46

DanilaFe reviewed Jan 17, 2024

View reviewed changes

mppf added 13 commits January 17, 2024 16:35

Enable abs on param real/imag to return a param real

3183f40

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Add future for abs on param complex returning param

0af4387

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Add abs and sqrt primitives

299bfdd

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Use the new primitive in the param real/imag abs

878de96

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Add 'abs' for param complex

9a477ad

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Enable param sqrt and .re / .im for complex

fd3f247

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Clarify complex section of spec

9ec8243

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Remove debug code

07b80d8

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Add test of .re / .im producing param

bcdc4db

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Add more testing

3940049

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Don't use fsqrt in C++ code

22a1d3f

The C++ overloading handles the size --- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Avoid compilation error for sqrt(-1) for tests of nan

1aabf9c

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Add comments

4cf0b5f

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

mppf force-pushed the param-abs branch from 58a5d37 to 4cf0b5f Compare January 17, 2024 21:35

mppf merged commit bfecaab into chapel-lang:main Jan 17, 2024
7 checks passed

mppf deleted the param-abs branch January 17, 2024 21:53

vasslitvinov added a commit to vasslitvinov/chapel that referenced this pull request Jan 18, 2024

Update line numbers in .good for chapel-lang#24127

f224d78

Signed-off-by: Vassily Litvinov <vasslitvinov@users.noreply.github.com>

vasslitvinov mentioned this pull request Jan 18, 2024

Update line numbers in .good for #24127 #24226

Merged

mppf changed the title ~~Enable abs of a param real/imag to produce a param real~~ Enable abs of a param real/imag/complex to produce a param real Jan 18, 2024

mppf changed the title ~~Enable abs of a param real/imag/complex to produce a param real~~ Enable abs and sqrt to return a param in more cases Jan 18, 2024

mppf mentioned this pull request Jan 18, 2024

Adjust complex sqrt in compiler to use the C functions #24231

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable abs and sqrt to return a param in more cases #24127

Enable abs and sqrt to return a param in more cases #24127

mppf commented Jan 2, 2024 •

edited

damianmoz commented Jan 2, 2024

DanilaFe Jan 5, 2024

mppf Jan 5, 2024

DanilaFe Jan 5, 2024

mppf commented Jan 5, 2024

DanilaFe commented Jan 17, 2024

DanilaFe Jan 17, 2024

mppf Jan 17, 2024

bradcray commented Jan 17, 2024

Enable abs and sqrt to return a param in more cases #24127

Enable abs and sqrt to return a param in more cases #24127

Conversation

mppf commented Jan 2, 2024 • edited

damianmoz commented Jan 2, 2024

DanilaFe Jan 5, 2024

Choose a reason for hiding this comment

mppf Jan 5, 2024

Choose a reason for hiding this comment

DanilaFe Jan 5, 2024

Choose a reason for hiding this comment

mppf commented Jan 5, 2024

DanilaFe commented Jan 17, 2024

DanilaFe Jan 17, 2024

Choose a reason for hiding this comment

mppf Jan 17, 2024

Choose a reason for hiding this comment

bradcray commented Jan 17, 2024

mppf commented Jan 2, 2024 •

edited