This is an archive of the discontinued LLVM Phabricator instance.

[flang] Implement tand intrinsic
ClosedPublic

Authored by DavidTruby on Jul 6 2023, 8:03 AM.

Details

Summary

This implements the tand intrinsic by performing a multiplication
by pi/180 to the argument before calling tan inline.

This is a commonly provided extension that is used by OpenRadioss

Diff Detail

Event Timeline

DavidTruby created this revision.Jul 6 2023, 8:03 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
DavidTruby requested review of this revision.Jul 6 2023, 8:03 AM

Could you add this to flang/docs/Extensions.md?

flang/lib/Optimizer/Builder/IntrinsicCall.cpp
5006

Can you add a TODO for the complex type?

Thank you for the changes! It looks good to me.

Will you also support constant folding for tand (in flang/lib/Evaluate/fold-real.cpp)? If not, please put a note about that into flang/docs/Extensions.md.

If I understand correctly, one of the (more important?) use cases for tand is that it returns exactly 0 for large multiples of 180. In contrast tan might return a floating point number that might slightly deviate from 0 for large multiples of pi.

Is the proposed implementation "more exact" for large multiples of 180 (compared to large multiples of pi with tan)?

If I understand correctly, one of the (more important?) use cases for tand is that it returns exactly 0 for large multiples of 180. In contrast tan might return a floating point number that might slightly deviate from 0 for large multiples of pi.

Is the proposed implementation "more exact" for large multiples of 180 (compared to large multiples of pi with tan)?

This implementation won't do that no, although the implementation in classic flang and gfortran doesn't appear to do that either. Classic flang and gfortran do both provide for tand(90) being Inf, which it won't be with this implementation though. I'm not really aware of how important this is, or how other compilers do it. I suppose we could have a runtime function that checks for both these cases?

For what it's worth, tand is in the draft Fortran 2023 standard, in which no guarantees are given about these edge cases (in fact, it explicitly uses tand(180) being approximately 0 as an example). I'm open to adding the edge cases if those are necessary for applications though; this patch will need reworking to call into a runtime library function instead of doing inline MLIR so there would be a performance tradeoff.

Would it make sense to do the reduction to the interval from -90 to +90 before the conversion from degrees to radians?
That reduction would most likely be more accurate for integers (most specific multiples of 180) than what can be done in finite precision after the conversion (i.e., by the tan function).

I think we can assume that reduction in range will be being done by the implementation of tan we call anyway, would there be an advantage to doing it again outside?

I guess my feeling is this: the standard doesn't provide for any kind of extra accuracy to special values in this function, and based on other compilers it doesn't seem like the user can assume extra accuracy across different compilers. We also don't have any kind of special information inside the runtime that the caller doesn't have anyway, so the user could check these special cases themself and will have to if they want portability.

As a result I say we should just do the fastest thing, and let users that need the extra accuracy check for it outside the intrinsic call. This will avoid the special case happening for users that don't need it, and avoid it happening twice for those that do.

I don't feel particularly strongly about this though, if others think we do need extra checks I'm happy to update the patch with them.

Thank you for the changes! It looks good to me.

Will you also support constant folding for tand (in flang/lib/Evaluate/fold-real.cpp)? If not, please put a note about that into flang/docs/Extensions.md.

I think I will do this in a separate patch, if that's ok? I can put a todo in this patch meanwhile

kiranchandramohan accepted this revision.EditedJul 17 2023, 2:15 PM

LG.

I think @mmuetzel probably makes a good point from a user perspective. I tried to look into this in a bit more detail but could not find details about this motivation.

The early j3 papers (https://j3-fortran.org/doc/year/18/18-139.txt, https://j3-fortran.org/doc/year/18/18-272.txt) do not mention exactly 0 for large multiples of 180 as a requirement or motivation. The motivation seems to be that this is implemented in multiple compilers and hence should be standardized.

The implementations in compilers seems to broadly follow what is being done here.
https://gcc.gnu.org/onlinedocs/gfortran/Extended-math-intrinsics.html

For advanced users, it may be important to know the implementation of these functions. They are simply wrappers around the standard radian functions, which have more accurate builtin versions. These functions convert their arguments (or results) to degrees (or radians) by taking the value modulus 360 (or 2*pi) and then multiplying it by a constant radian-to-degree (or degree-to-radian) factor, as appropriate. The factor is computed at compile-time as 180/pi (or pi/180).

https://docs.oracle.com/cd/E19205-01/820-4180/man3m/trig_sun.3m.html

sind(x), cosd(x), and tand(x) return trigonometric functions
of   degree   arguments.    sind(x):=   sin(x*pi/180).

Note: The j3 proposals page for this is https://github.com/j3-fortran/fortran_proposals/issues/164.

Please wait a day before submitting incase there are different opinions.

This revision is now accepted and ready to land.Jul 17 2023, 2:15 PM
This revision was landed with ongoing or failed builds.Jul 19 2023, 6:45 AM
This revision was automatically updated to reflect the committed changes.

I have pushed this as-is, and we can add some extra precision afterwards if we think it's necessary. Hopefully that's ok!

I have pushed this as-is, and we can add some extra precision afterwards if we think it's necessary. Hopefully that's ok!

The degree and half-cycle trig functions like TAND that are becoming standard in Fortran 2023 should not be implemented like this (multiplication of arguments or results). Libraries with accurate implementations should be used instead.

Could you introduce a TandOp for optimizations?