1084319 - SIMD: inline Clamp

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

So it was apparently agreed that SIMD.float32x4.clamp should be kept. As a reminder:

SIMD.float32x4.clamp(x, low, high) = min(max(x[i], low[i]), high[i])

Now, the rationale behind having a separate instruction for that is to behave differently than SIMD.float32x4.min/max, in particular in the case of NaNs and maybe -0 vs +0 (?)

Dan, how should clamp behave wrt NaN and -0 vs +0?

Comment 1

[Dan Gohman [:sunfish]]

I had previously requested that we postpone thinking about clamp until we settle on how min and max should behave. There now seems to be some momentum behind the plan to make min and max follow Math.min and Math.max semantics, as it's useful and what many people find most intuitive, and to also have IEEE-754-style minNum and maxNum functions, because IEEE-754, and because there are some important use cases for them.

And in view of this, I now am contemplating recommending we remove clamp from the spec. clamp makes a few optimizations easier, but at the same time, in the spec we'd probably have to include "if (low > high) return NaN", which a user-defined or library-defined clamp could more easily skip. And, it's difficult to decide what to do when x is NaN: return NaN to avoid squelching an exception, or return low or high to maintain a stricter invariant? I haven't found agreement among prior art -- most contexts where clamp is used don't give NaN much thought. Any thoughts here before I take this proposal forward?

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

This makes sense. I've suggested removing clamp as well in [0], but Luke was the one who said we should keep it, to not depend on min/max semantics. Luke, any strong motivation for keeping this operator?

[0] https://github.com/johnmccutchan/ecmascript_simd/issues/68

Comment 3

[Luke Wagner [:luke]]

Nope, it's up to y'all.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Let's morph this into inlining clamp in Ion, as this is now trivial by using two simd comp and two selects.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: clamp.patch

This patch makes me think we really don't need clamp in the spec and would need a library apart from the polyfill, to illustrate how to use primary operations, but oh well.

Comment 6

[Dan Gohman [:sunfish]]

I don't recall a consensus about what to do with clamp, and can't find a discussion of it at the moment. There were discussions in which we agreed to keep it until after min/max had settled down. Have there been any discussions since min/max have now settled down?

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Dan Gohman [:sunfish] from comment #6)
> I don't recall a consensus about what to do with clamp, and can't find a
> discussion of it at the moment. There were discussions in which we agreed to
> keep it until after min/max had settled down. Have there been any
> discussions since min/max have now settled down?

I remember asking whether we should delete it several times in the internal email thread, because 1) it can be emulated easily with other ops (as this patch shows) and 2) it doesn't map to a single processor instruction, and the conclusion was that it is useful. Maybe worth that we bring the topic back for the next face-to-face meeting? (i will very probably be in PTO all next week, so if you could bring it on the agenda please, that would be great! :))

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Here's the discussion about SIMD.clamp, which has been closed without any other comment...
https://github.com/johnmccutchan/ecmascript_simd/issues/68

The internal email thread didn't say much more about it...

The only advantage with clamp is to guarantee optimization isn't missed, but I can't imagine how the optimization opportunity could be missed? That is, SIMD.clamp should be optimized whenever the equivalent operations sequence is optimized.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/16a9e44cd003

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Oops sorry, wrong bug number in this commit, changeset in comment 9 belongs to bug 1146363 as well :(

Comment 11

[Dan Gohman [:sunfish]]

Clamp is no longer part of the core spec: https://github.com/johnmccutchan/ecmascript_simd/pull/153