Closed Bug 1077175 Opened 9 years ago Closed 9 years ago

SIMD: Avoid dependence on Math.fround for SIMD.float32x4?

Categories

(Core :: JavaScript Engine: JIT, enhancement)

Product:
Core
Component:
JavaScript Engine: JIT
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla36

People

(Reporter: sunfish, Assigned: luke)

Details

Attachments

(1 file)

doublelit
16.60 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review 
As was reported in [0], SIMD asm.js validation currently requires the input to e.g. the SIMD.float32x4 constructor to be explicitly typed with Math.fround calls. This makes sense, however it means that one can't easily use float32x4 (presumably via the polyfill) on browsers which lack Math.fround.

Is it worth loosening up the rules to allow the inputs to the SIMD.float32x4 constructor, splat, and insert, to accept 64-bit arguments?

[0] https://github.com/kripken/emscripten/issues/2856
Arg, this one is kinda tricky and bbouvier and I spent some time discussing it earlier.  The basic issue is that asm.js uses bottom-up typing so in type-checking float32x4.splat(1.0), we first type check "1.0", see that it has type double, then lookup 'splat' on stdlib and see it has type floatish->float32x4, and reject b/c we have a double.  I'd really rather not add a double overload to all these operators since, if the argument isn't a constant, this hides a non-zero-cost double-to-float conversion.

Based on how fixnum works (it is a subtype of signed and unsigned) you might think we can do the same for float/double literals, but then we have ambiguities when passing this type to double/float-overloaded stdlib functions (ceil/floor, min/max, abs, sqrt).

One possible option I hadn't considered before: we could have a double-literal type (that is, the type of numeric literals that have type double) and make this a subtype of both double and floatish. This would be ok because anything that accepts a floatish necessarily calls Math.fround() on the argument and thus there can't be any double/float ambiguity.  It also means all our existing SIMD type checking code wouldn't need to be changed; we'd just add the new type and subtype relation.

What do you think Benjamin?  Am I forgetting a constraint?
Needinfo'ing myself, I'll try unless somebody beats me to it.
Flags: needinfo?(benj)
Attached patch doublelit — — Splinter Review 
Writing the patch forced me to recall the other constraints: a 'doublelit' type doesn't have an a priori MIR type which makes it somewhat hazardous to use in Odin.

This patch makes a slightly less elegant change to achieve the desired effect for splat/ctor:
 - a new type 'doublelit' is added for double numeric literals; it is only a subtype of double and has MIRType_Double
 - a new 'doublelit' overload is added to splat/float32x4.  When the argument has type doublelit, we just emit a new float32 constant (ignoring the original double constant)

This requires duplicating some code in CheckSimdCallArgs, but this seemed preferable to adding an MDefinit
Assignee: nobody → luke
Status: NEW → ASSIGNED
Attachment #8508185 - Flags: review?(benj)
Comment on attachment 8508185 [details] [diff] [review]
doublelit

Review of attachment 8508185 [details] [diff] [review]:
-----------------------------------------------------------------

With this patch giving access to the MDefinition to the CheckArg function object, i think shuffle arguments checking could be done ~the same way.

::: js/src/asmjs/AsmJSValidate.cpp
@@ -603,5 @@
> -          case Void:
> -            break;
> -        }
> -        MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE("Invalid SIMD Type");
> -    }

ha, good catch

@@ -4923,5 @@
>  }
>  
>  typedef Vector<MDefinition*, 4, SystemAllocPolicy> DefinitionVector;
>  
> -namespace {

I think this namespace opening was here because otherwise the compiler would not compile or warn about Type being defined in an anonymous namespace but used in a non-anonymous namespace. Didn't you get any issue with this?

@@ +4963,5 @@
> +    Type formalType_;
> +
> +  public:
> +    CheckSimdScalarArgs(Type simdType)
> +      : simdType_(simdType), formalType_(simdType.simdToCoercedScalarType()) {}

nit: can you move the {} to the next line, now that the initializers list isn't on the same line?

@@ +4974,5 @@
> +            // re-emitting them as float32 constants.
> +            if (!simdType_.isFloat32x4() || !actualType.isDoubleLit()) {
> +                return f.failf(arg, "%s is not a subtype of %s%s",
> +                               actualType.toChars(), formalType_.toChars(),
> +                               simdType_.isFloat32x4() ? "or doublelit" : "");

nit: " or doublelit" (note the space after the opening double quote)

@@ +4979,5 @@
> +            }
> +
> +            AsmJSNumLit doubleLit = ExtractNumericLiteral(f.m(), arg);
> +            MOZ_ASSERT(doubleLit.which() == AsmJSNumLit::Double);
> +            *def = f.constant(doubleLit.scalarValue(), Type::Float);

This will clobber a MDefinition that was already added to the graph beforehands and will stay around until DCE, right? Isn't there a way to replace it instead?
Attachment #8508185 - Flags: review?(benj) → review+
Annnnnd no more needinfo for me, thanks Luke.
Flags: needinfo?(benj)
(In reply to Benjamin Bouvier [:bbouvier] from comment #4)
> > -namespace {
> 
> I think this namespace opening was here because otherwise the compiler would
> not compile or warn about Type being defined in an anonymous namespace but
> used in a non-anonymous namespace. Didn't you get any issue with this?

Wow, weird; that seems like a poor warning.  I tried again with gcc and no warning (I also tried a tiny testcase compiled with -Wall -pedantic and also no warning).  Really, though, we do this for all the Check* functions.

> This will clobber a MDefinition that was already added to the graph
> beforehands and will stay around until DCE, right? Isn't there a way to
> replace it instead?

That's right.  IIUC, the replace* functions don't do anything extra to super-kill a node, they just repoint uses/defs to a new node.  In this case, there shouldn't be any such uses (not that we really care if there are, all we care about is that the result of this expression is MIRType_Float32.
Well Benjmain I guess there is the mysterious namespace error you were talking about.  The error messages say there is an ambiguity between (unnamed namespace)::Type and js::types::Type.  I'm guessing unififed builds are putting a 'using namespace js::types;' in this file and that's causing the ambiguity and the fact that this is a template function somehow confounds this.  I wonder if ::Type fixes it.
 https://tbpl.mozilla.org/?tree=Try&rev=c65461469eb0
Flags: needinfo?(luke)
https://hg.mozilla.org/mozilla-central/rev/b743be2b24ab
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.