Closed Bug 1084319 Opened 5 years ago Closed 5 years ago

SIMD: inline Clamp

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED WONTFIX
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

Attachments

(1 file)

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?
Flags: needinfo?(sunfish)
Blocks: 1023404
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?
Flags: needinfo?(sunfish)
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
Flags: needinfo?(luke)
Nope, it's up to y'all.
Flags: needinfo?(luke)
Let's morph this into inlining clamp in Ion, as this is now trivial by using two simd comp and two selects.
Assignee: nobody → benj
Blocks: 1112155
No longer blocks: 1023404
Status: NEW → ASSIGNED
Summary: SIMD backend: Implement Clamp → SIMD: inline Clamp
Attached patch clamp.patchSplinter Review
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.
Attachment #8582450 - Flags: review?(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?
(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! :))
Flags: needinfo?(sunfish)
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.
https://hg.mozilla.org/mozilla-central/rev/16a9e44cd003
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Oops sorry, wrong bug number in this commit, changeset in comment 9 belongs to bug 1146363 as well :(
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Clamp is no longer part of the core spec: https://github.com/johnmccutchan/ecmascript_simd/pull/153
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Flags: needinfo?(sunfish)
Resolution: --- → WONTFIX
Attachment #8582450 - Flags: review?(sunfish)
You need to log in before you can comment on or make changes to this bug.