Bug 176285 - WSL should support the bool type
Summary: WSL should support the bool type
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks: 176199
  Show dependency treegraph
 
Reported: 2017-09-02 17:24 PDT by Filip Pizlo
Modified: 2018-10-13 17:07 PDT (History)
1 user (show)

See Also:


Attachments
WIP (18.48 KB, patch)
2017-09-03 12:20 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (21.46 KB, patch)
2017-09-03 13:00 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
WIP (27.18 KB, patch)
2017-09-03 23:16 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (39.09 KB, patch)
2017-09-04 12:17 PDT, Myles C. Maxfield
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2017-09-02 17:24:48 PDT
It should be declared as a native type. Then there should be a BoolLiteral that works more-or-less like the current UintLiteral.

This unlocks the ability to add control flow constructs.
Comment 1 Filip Pizlo 2017-09-02 18:01:25 PDT
Also, it seems like it would be best to remove the notion that operator! is a customizable operator. Instead, go straight to explicit operator bool.

In this world, operator! is built-in.

operator! and all of the conditional contexts ("if (_)", "while (_)", and "for (;_;)") are parsed so that they first emit a call to "operator bool(T)". Essentially we can turn this:

    !e

into this:

    !(operator bool(e))

Where you can declare an operator bool for whatever types you like.  We could declare a default one that looks like this:

operator bool<T>(T value)
{
    T defaultValue;
    return value != defaultValue;
}

Note that a!=b is parsed as !(a==b).  Currently override resolution is order-based: we parse the signatures in order and we pick the first one that matches.  We could say that the standard library has a suffix portion that is parsed after the program, which contains the following:

operator bool<T>(T value)
{
    T defaultValue;
    return value != defaultValue;
}
native bool operator==<T>(T a, T b); // Does bitwise equality

By making this a suffix, it means that the programmer can always rely on these as defaults but can easily override them for their type.

So, in this world, this totally works.

struct MyPoint { int x; int y; }
void foo()
{
    MyPoint p;
    ...
    if (p)
        do stuff if p is not the origin
}

Does this seem useful, or overkill?  Do you want to try to do this?
Comment 2 Myles C. Maxfield 2017-09-03 12:20:06 PDT
Created attachment 319794 [details]
WIP
Comment 3 Myles C. Maxfield 2017-09-03 12:22:53 PDT
(In reply to Filip Pizlo from comment #1)
> 
> operator bool<T>(T value)
> {
>     T defaultValue;
>     return value != defaultValue;
> }

You aren't placing any type restrictions on T. Does this mean that we will require every type in the whole language to be default-constructible?
Comment 4 Filip Pizlo 2017-09-03 12:25:37 PDT
(In reply to Myles C. Maxfield from comment #3)
> (In reply to Filip Pizlo from comment #1)
> > 
> > operator bool<T>(T value)
> > {
> >     T defaultValue;
> >     return value != defaultValue;
> > }
> 
> You aren't placing any type restrictions on T. Does this mean that we will
> require every type in the whole language to be default-constructible?

That's already true, since we don't have constructors or destructors or any member functions.  Structs have a default value (just give all fields the default value).  Arrays have a default value (just give all array elements the default value).
Comment 5 Myles C. Maxfield 2017-09-03 13:00:57 PDT
Created attachment 319797 [details]
WIP
Comment 6 Myles C. Maxfield 2017-09-03 13:06:56 PDT
(In reply to Filip Pizlo from comment #4)
> (In reply to Myles C. Maxfield from comment #3)
> > (In reply to Filip Pizlo from comment #1)
> > > 
> > > operator bool<T>(T value)
> > > {
> > >     T defaultValue;
> > >     return value != defaultValue;
> > > }
> > 
> > You aren't placing any type restrictions on T. Does this mean that we will
> > require every type in the whole language to be default-constructible?
> 
> That's already true, since we don't have constructors or destructors or any
> member functions.  Structs have a default value (just give all fields the
> default value).  Arrays have a default value (just give all array elements
> the default value).

ArrayRefs will be null?
Comment 7 Filip Pizlo 2017-09-03 13:27:55 PDT
(In reply to Myles C. Maxfield from comment #6)
> (In reply to Filip Pizlo from comment #4)
> > (In reply to Myles C. Maxfield from comment #3)
> > > (In reply to Filip Pizlo from comment #1)
> > > > 
> > > > operator bool<T>(T value)
> > > > {
> > > >     T defaultValue;
> > > >     return value != defaultValue;
> > > > }
> > > 
> > > You aren't placing any type restrictions on T. Does this mean that we will
> > > require every type in the whole language to be default-constructible?
> > 
> > That's already true, since we don't have constructors or destructors or any
> > member functions.  Structs have a default value (just give all fields the
> > default value).  Arrays have a default value (just give all array elements
> > the default value).
> 
> ArrayRefs will be null?

Word. Zero length and zero base pointer.
Comment 8 Myles C. Maxfield 2017-09-03 23:16:52 PDT
Created attachment 319842 [details]
WIP
Comment 9 Myles C. Maxfield 2017-09-04 12:17:01 PDT
Created attachment 319858 [details]
Patch
Comment 10 Filip Pizlo 2017-09-04 13:01:26 PDT
Comment on attachment 319858 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319858&action=review

r=me. 

Would be good to clean up the way the parser and leader interact. It’s easier in the long run if the lexer doesn’t know too much about keywords. Please also fix the WTypeError issue and the origin issue. The rest (like naming) is optional.

> Tools/WebGPUShadingLanguageRI/Checker.js:229
> +            throw new Error("Trying to negate something that isn't a bool: " + node.value);

This should be a WTypeError since this case can be reached with bad input. (The Error above is fine because that will only happen with a bug in our code.)

> Tools/WebGPUShadingLanguageRI/Intrinsics.js:209
> +            throw new WTypeError(thing.toString(), "Unrecognized intrinsic: " + thing);

The first arg should be the origin string since that’s what we do everywhere else. Can we do that here?

> Tools/WebGPUShadingLanguageRI/Lexer.js:106
> +            if (isAddressSpace(RegExp.lastMatch))

Why can’t this get handled as a keyword?  If it’s not a keyword, why are address spaces still in the keyword regezp?

> Tools/WebGPUShadingLanguageRI/Lexer.js:108
> +            if (["true", "false"].includes(RegExp.lastMatch))

Ditto. 

Compilers usually recognize all such things as keywords. It’s up to the parser to decide how to categorize keywords.

> Tools/WebGPUShadingLanguageRI/LogicalNegationExpression.js:27
> +class LogicalNegationExpression extends Expression {

This is usually called LogicalNot

> Tools/WebGPUShadingLanguageRI/Parse.js:188
> +        if (token = tryConsumeKind("boolLiteral"))

You could have said tryConsume(“true”, “false”)
Comment 11 Filip Pizlo 2017-09-04 13:44:10 PDT
Comment on attachment 319858 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319858&action=review

> Tools/WebGPUShadingLanguageRI/Parse.js:629
> +        result.name = "operator " + castType;

Oh now I get why TypeDef resolution doesn't work!  Ha!

Yeah, the way to get around this (which you can do in a separate patch) is:
- Make these guys always be called "operator cast".  That is always the name.
- Thread returnType through resolveOverloadImpl and everyone who calls it.
    - This must be null if calling anything other than "operator cast".
    - When calling "operator cast", Checker knows the desired return type, and so it will pass that as the returnType.
    - resolveOverloadImpl will attempt unification on the returnType if it is given one.  It will do this the same way it does for argument types.

You may want a subclass of CallExpression for these casts, since those call expressions know the desired return type syntactically.  Visitors and the Rewriter will have to visit and rewrite that desired return type.

If you do that, then TypeDefs will magically resolve themselves because the TypeDef resolver will simply find the TypeDef in the returnType and replace it will something else.

> Tools/WebGPUShadingLanguageRI/StandardLibrary.js:56
> +native operator int(int);
> +native operator uint(uint);
> +native operator bool(bool);

Why are there self casts?

Shouldn't they be provided by the language?
Comment 12 Myles C. Maxfield 2017-09-05 14:12:22 PDT
Committed r221634: <http://trac.webkit.org/changeset/221634>
Comment 13 Myles C. Maxfield 2017-09-06 12:34:45 PDT
Committed r221690: <http://trac.webkit.org/changeset/221690>
Comment 14 Radar WebKit Bug Importer 2017-09-27 12:39:14 PDT
<rdar://problem/34693690>
Comment 15 Myles C. Maxfield 2018-10-13 17:07:45 PDT
Migrated to https://github.com/gpuweb/WHLSL/issues/159