Bug 177031 – Figure out how WSL will support field overloads like float4.xz and friends

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGPU

[Filip Pizlo]

I think that we do need operator[] and operator[]=, unlike what I concluded in https://bugs.webkit.org/show_bug.cgi?id=176958#c2 and subsequently landed.

Otherwise, what does this return:

    ? operator&.xz(thread float4^)

It cannot return float2, because it needs to return a pointer to a field. It cannot return float2^, because xz are not next to each other.

But it would work if we could instead override:

    float2 operator.xz(float4)
    void operator.xz=(float4, float2)

[Filip Pizlo]

Here are some notes I took while pondering this.  I think I have an algorithm now.

--

array[index] += foo();

1. ptr = &array[index];
2. load from ptr
3. foo()
4. store to ptr



native struct Foo;
native struct Bar;
native struct Baz;

Bar operator.bar(Foo);
Foo operator.bar=(Foo, Bar);

thread Baz^ operator&.baz(thread Foo^);

Foo foo;

RMW(foo.bar, anonVar<0>, anonVar<1>, newValueExp(anonVar<0>), anonVar<1>)
=> RMW(foo, anonVar<2>, anonVar<3>,
       (anonVar<0>, anonVar<1>,
        anonVar<0> = operator.bar(anonVar<2>),
        anonVar<1> = newValueExp(anonVar<0>),
        operator.bar=(anonVar<2>, anonVar<1>)),
       anonVar<1>)

RMW(foo.baz, anonVar<0>, anonVar<1>, newValueExp(anonVar<0>), anonVar<1>)
=> (anonVar<0>, anonVar<1>, anonVar<2>,
    anonVar<2> = &foo.baz,
    anonVar<0> = ^anonVar<2>,
    anonVar<1> = newValueExp(anonVar<0>),
    ^anonVar<2> = anonVar<1>)


native struct Thingy;

Thingy operator.thingy(Bar);
Bar operator.thingy=(Bar, Thingy);

RMW(foo.bar.thingy, anonVar<0>, anonVar<1>, newValueExp(anonVar<0>), anonVar<1>)
=> RMW(foo.bar, anonVar<2>, anonVar<3>,
       (anonVar<0>, anonVar<1>,
        anonVar<0> = operator.thingy(anonVar<2>),
        anonVar<1> = newValueExp(anonVar<0>),
        operator.thingy=(anonVar<2>, anonVar<1>)),
       anonVar<1>)
=> RMW(foo, anonVar<4>, anonVar<5>,
       (anonVar<2>, anonVar<3>,
        anonVar<2> = operator.bar(anonVar<4>),
        anonVar<3> = (anonVar<0>, anonVar<1>,
                     anonVar<0> = operator.thingy(anonVar<2>),
                     anonVar<1> = newValueExp(anonVar<0>),
                     operator.thingy=(anonVar<2>, anonVar<1>)),
        operator.bar=(anonVar<4>, anonVar<3>)),
       anonVar<1>)


foo.bar = thingy
=> RMW(foo, anonVar<0>, anonVar<1>,
       (anonVar<2>,
        anonVar<2> = thingy,
        operator.bar=(anonVar<0>, anonVar<2>)),
       anonVar<2>)

foo.baz = thingy
=> ^operator&.baz(&foo) = thingy


foo.bar
=> operator.bar(foo)

foo.baz
=> ^operator&.bar(&foo)

foo.bar.thingy
=> operator.bar(foo).thingy
=> operator.thingy(operator.bar(foo))

foo.baz.stuff
=> ^operator&.baz(&foo)
=> ^operator&.stuff(operator&.baz(&foo))


load
assign
dereference
rmw


1. figure out what the load conversion would do
2. if it yields a Dereference, then use the pointer deconstruction
3. otherwise use the normal deconstruction

It's not clear if this deconstruction should happen in Checker or later.  Might be easier if it happens later. That would require factoring visitCallExpression out.  That shouldn't be hard.

First the checker would do its thing pretending that DotExpression and IndexExpression are just valid lvalues. Then a subsequent pass will convert those into calls or other things depending on context.

This would mean that the only places that have to know the & and @ tricks that CallExpression currently does are the places that handle the load case. We'll need both IndexExpression and DotExpression to know how to do it.

[Filip Pizlo]

Created attachment 321007 [details]
more

[Filip Pizlo]

Created attachment 321010 [details]
most of it is written

This isn't done yet.  I still have to drag RMWExpression and IndexExpression through all of the visitors.

But, I think the hard parts are done.

[Filip Pizlo]

Created attachment 321017 [details]
starting to test it

[Filip Pizlo]

Created attachment 321018 [details]
it passes so many tests

[Filip Pizlo]

Created attachment 321019 [details]
passes all of our current tests

[Filip Pizlo]

Created attachment 321023 [details]
it's passing significant tests

Passes all old tests plus some large new ones.

[Filip Pizlo]

Created attachment 321025 [details]
the patch

[Filip Pizlo]

Created attachment 321050 [details]
added another substantial test

[Filip Pizlo]

Created attachment 321052 [details]
more tests

[Filip Pizlo]

Created attachment 321053 [details]
the patch

Even more crazy testing.

[Filip Pizlo]

Created attachment 321057 [details]
wrote more tests

[Filip Pizlo]

Created attachment 321058 [details]
more tests

One easy way to create new tests is to take an existing test and try to make its functions as generic as possible.

[Filip Pizlo]

Created attachment 321059 [details]
the patch

[Filip Pizlo]

Created attachment 321076 [details]
the patch

[Filip Pizlo]

Created attachment 321106 [details]
the patch

[JF Bastien]

Comment on attachment 321106 [details]
the patch

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

Lots of this is over my head for now, first look at WSL :)

r=me with that caveat.

> Tools/WebGPUShadingLanguageRI/Checker.js:40
>          }

Ah no semicolon!

> Tools/WebGPUShadingLanguageRI/Checker.js:160
> +            throw new WTypeError(node.origin.originString, "LHS of read-modify-write is not an LValue: " + node.lVale);

Typo "node.lVale"

> Tools/WebGPUShadingLanguageRI/Checker.js:236
> +            throw new Error("Cannot get typeForAnd");

Is there some context that can be thrown here? Looked like you have origin laying around that helps figure out where in the source this comes from?

> Tools/WebGPUShadingLanguageRI/FindHighZombies.js:27
> +function findHighZombies(program)

👍

> Tools/WebGPUShadingLanguageRI/Parse.js:784
> +            let token = consume("+", "-", "*", "/", "%", "^", "&", "|", "<", ">", "<=", ">=", "==", "++", "--", "&", ".", "~", "<<", ">>", "[");

Does order in "consume" matter here? Because you have "<" before "<<", would it consume "<" first? I guess you already had "-" before "--" so no?

You also have "&" twice.

[Filip Pizlo]

(In reply to JF Bastien from comment #17)
> Comment on attachment 321106 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=321106&action=review
> 
> Lots of this is over my head for now, first look at WSL :)
> 
> r=me with that caveat.
> 
> > Tools/WebGPUShadingLanguageRI/Checker.js:40
> >          }
> 
> Ah no semicolon!
> 
> > Tools/WebGPUShadingLanguageRI/Checker.js:160
> > +            throw new WTypeError(node.origin.originString, "LHS of read-modify-write is not an LValue: " + node.lVale);
> 
> Typo "node.lVale"

fixed

> 
> > Tools/WebGPUShadingLanguageRI/Checker.js:236
> > +            throw new Error("Cannot get typeForAnd");
> 
> Is there some context that can be thrown here? Looked like you have origin
> laying around that helps figure out where in the source this comes from?

We throw Error when it's an ICE, not an error to be reported to the user.  I put this in there as basically an assertion; I don't expect it to ever fire.

> 
> > Tools/WebGPUShadingLanguageRI/FindHighZombies.js:27
> > +function findHighZombies(program)
> 
> 👍
> 
> > Tools/WebGPUShadingLanguageRI/Parse.js:784
> > +            let token = consume("+", "-", "*", "/", "%", "^", "&", "|", "<", ">", "<=", ">=", "==", "++", "--", "&", ".", "~", "<<", ">>", "[");
> 
> Does order in "consume" matter here? Because you have "<" before "<<", would
> it consume "<" first? I guess you already had "-" before "--" so no?
> 
> You also have "&" twice.

The order doesn't matter.  The lexer takes care of having the right regexp's to parse these.

I'll clean up this list.

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/222169/webkit

[Radar WebKit Bug Importer]

<rdar://problem/34693376>

[Myles C. Maxfield]

Migrated to https://github.com/gpuweb/WHLSL/issues/131