Skip to content

Conversation

@resistor
Copy link
Contributor

No description provided.

Copy link
Member

@colesbury colesbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty ambivalent about naming these Vec256::min and Vec256::max as they differ in semantics from std::min and std::max. So far we've tried to match the semantics of either the standard library or the underlying vector instructions to keep things simple to reason about.

I'd prefer a different name for the NaN propagating min/max.

@resistor
Copy link
Contributor Author

resistor commented Oct 31, 2018

@colesbury Depressingly, there's not a good precedent to follow here.

  • IEEE 754-201X defines operators maximum and maximumNumber which respectively propagate NaNs and squash NaNs.

  • std::max uses a ternary operator (a > b) ? a : b which does not match either of the defined IEEE 754 operations, and is not commutative.

  • C differs from C++, with fmax matching IEEE 754 maximumNumber

  • NumPy defines max which matches IEEE 754 maximum and maxnan which matches maximumNumber

  • X86: SSE MAXPS matches std::max definition, with the same issues. AVX-512 VRANGE can implement the IEEE 754 operators.

  • ARMv8: NEON provides fmax which matches IEEE 754 maximum and fmaxnm which matches maximumNumber

Based on prior discussion, my understanding is that we want to follow the NumPy example in PyTorch of propagating NaNs by default. In my opinion the best option, if we don't want to keep the existing name, is to standardize on the IEEE 754 operator names, since all of the others are overloaded across architectures and languages.

@colesbury
Copy link
Member

@resistor yeah, minimum and maximum seem like reasonable names as they propagate NaNs in IEEE draft and also NumPy.

Mostly, I'm thinking of operations that need to be written for both scalar types (e.g. float) and the vector types:

[=](scalar_t a, scalar_t b) -> scalar_t { return a + b; },
[=](Vec256<scalar_t> a, Vec256<scalar_t> b) { return a + b; });

Along these lines, it may also be worth adding a implementation for scalar types. Something like:

template <class T> 
inline T minimum(const T& a, const T& b) { ... }

With the idea being that one could call vec256::minimum(float, float) and vec256::minimum(Vec256, Vec256) and it would perform the same operation.

An example of this is fmadd

@resistor resistor force-pushed the nan branch 2 times, most recently from 65dc2b0 to e144429 Compare November 6, 2018 21:00
@resistor
Copy link
Contributor Author

@colesbury ping

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@resistor has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

zdevito pushed a commit to zdevito/ATen that referenced this pull request Nov 27, 2018
Summary: Pull Request resolved: pytorch/pytorch#13399

Differential Revision: D13199957

Pulled By: resistor

fbshipit-source-id: 1565e079b13c5d4f42f2033830a7c997b7d824bc
@ezyang ezyang added the merged label Jun 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants