This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Provide additional templates for valarray transcendentals that satisfy the standard synopsis
Needs ReviewPublic

Authored by petpav01 on Sep 30 2015, 6:44 AM.

Details

Summary

Libc++ provides valarray transcendentals with replacement types. These functions are implemented either as template<class _Expr> or template<class _Expr1, class _Expr2>, where _Expr can be __val_expr or valarray.

The patch provides additional function templates for valarray transcendentals that as a parameter use _Tp which is a type of elements in the valarray. This is required by the standard and is needed if the user tries to explicitly instantiate the transcendental functions using _Tp, for example, std::abs<int>(int_valarray).

New templates do not take an additional _Expr parameter and so the functions accept only valarray as their parameter. This means that if the user explicitly instantiates these function templates and passes __val_expr to them, it first needs to be converted to valarray and the benefit of the expression template optimization is not present. No performance is lost in the currently possible case where template parameters are deduced because using __val_expr as an argument will lead to instantiation of the already present templates.

Diff Detail

Event Timeline

petpav01 updated this revision to Diff 36098.Sep 30 2015, 6:44 AM
petpav01 retitled this revision from to [libc++] Provide additional templates for valarray transcendentals that satisfy the standard synopsis.
petpav01 updated this object.
petpav01 added a subscriber: cfe-commits.

It would be probably better if the patch changed the original templates to take only __val_expr as there is now no need for them to match valarray too. This should be a simple change but requires additional tests so I will wait for initial feedback that this approach is preferred.

EricWF added a subscriber: EricWF.

This is required by the standard and is needed if the user tries to explicitly instantiate the transcendental functions using _Tp, for example, std::abs<int>(int_valarray).

These may in fact be required by the standard but not for the reason you say. The standard essentially prohibits users from doing anything with standard library functions other than calling them with a list of arguments. This includes specifying default template parameters. The relevant text from the current C++ draft:

[17.6.5.4] global.functions`

  1. A call to a global or non-member function signature described in Clauses 18 through 30 and Annex D shall

behave as if the implementation declared no additional global or non-member function signatures

However the overloads you need are required for standard conformance because they provide an interface that requires an implicit conversion to valarray<Tp> be performed. This subtly changes its ranking in overload resolution. For this reason we need this patch.

At first look this patch looks good, and it adds lots of tests which is even better.

EricWF requested changes to this revision.Oct 14 2015, 12:20 AM
EricWF edited edge metadata.

One thing does need to change though. For each overload you added you need to change all of the uses of __is_val_expr in the original overload to a new metafunction that only accepts __val_expr templates and not valarray.

I'm marking as "changes requested" so phabricator properly bins this. Thanks again for the patch.

This revision now requires changes to proceed.Oct 14 2015, 12:20 AM

Thank you for having a look at this patch. I should get to updating it as requested soon. Apologies for the delay.

petpav01 updated this revision to Diff 40486.Nov 18 2015, 3:06 AM
petpav01 edited edge metadata.

I am not sure if I understand the comment about [17.6.5.4] correctly. It is not clear to me how this part of the standard prevents the user from specifying explicit template parameters for standard functions. It seems odd that the standard would disallow, for example, to use std::max<double>(1, 2.0).

New patch changes the transcendentals that accepted any Expr (i.e. valarray or __val_expr) to only accept __val_expr. In two cases (atan2() and pow()) the original template had to be split into four functions:

  • func(const __val_expr&, const __val_expr&)
  • func(const __val_expr&, const valarray&)
  • func(const valarray&, const __val_expr&)
  • func(const valarray&, const valarray&)

is_same checks on value_type of both operands are also added in these cases.

Missing thing is a better test coverage. The tests currently do not instantiate the __val_expr templates. I am working on this. Could you please send me an example that shows the problem with the ranking in overload resolution so I can add a test for it too?

petpav01 updated this revision to Diff 42506.Dec 11 2015, 12:46 AM
petpav01 edited edge metadata.

Updated patch adds more tests and fixes a problem introduced in the previous revision where templates taking __val_expr were not correctly protected by SFINAE from immediate context (it introduced same problem with explicit template parameters that I am trying to solve).

@Eric: Could you please send me an example that shows the problem with the ranking in overload resolution so I can add a test for it too? I would like to understand this case because if it is not needed then a number of atan2() and pow() templates can be reduced (to the same amount as in the original patch).

Ping. I would still like to address this problem. Could I please get a review on the last version of the patch?

Thanks,
Petr

Ping.
Any review for this patch? It would be great if we can get this issue fixed.

wow this is old. I'll try and re-review before the new year.

petpav01 updated this revision to Diff 86217.Jan 29 2017, 7:05 AM

Patch rebased.

I've raised PR31966 to poke this :)