Bug 189502 – [WHLSL] Improve test suite type safety

• Component: New Bugs
• Status: RESOLVED FIXED

[Thomas Denney]

[WHLSL] Improve test suite type safety

[Thomas Denney]

Created attachment 349399 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 349399 [details]
Patch

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

> Tools/WebGPUShadingLanguageRI/Casts.js:45
> +function castToBool(value)
> +{
> +    return !!value;
> +}

Can we put this with the rest of the casts?

> Tools/WebGPUShadingLanguageRI/Test.js:83
> -    return TypedValue.box(program.intrinsics.int, value);
> +    return TypedValue.box(program.intrinsics.int, castToInt(value));
>  }
>  
>  function makeUint(program, value)
>  {
> -    return TypedValue.box(program.intrinsics.uint, value);
> +    return TypedValue.box(program.intrinsics.uint, castToUint(value));
>  }
>  
>  function makeUchar(program, value)
>  {
> -    return TypedValue.box(program.intrinsics.uchar, value);
> +    return TypedValue.box(program.intrinsics.uchar, castToUchar(value));
>  }
>  
>  function makeBool(program, value)
>  {
> -    return TypedValue.box(program.intrinsics.bool, value);
> +    return TypedValue.box(program.intrinsics.bool, castToBool(value));
>  }
>  
>  function makeFloat(program, value)
>  {
> -    return TypedValue.box(program.intrinsics.float, value);
> +    return TypedValue.box(program.intrinsics.float, castToFloat(value));
>  }
>  
>  function makeHalf(program, value)
>  {
> -    return TypedValue.box(program.intrinsics.half, value);
> +    return TypedValue.box(program.intrinsics.half, castToHalf(value));

perhaps instead of silently casting, we should throw if the input thing isn't already the right type. The caller should know if the only reason the test is passing is that the type system is interfering

> Tools/WebGPUShadingLanguageRI/Test.js:2702
> -    checkUchar(program, callFunction(program, "foo", [makeUchar(program, 65535), makeUint(program, 2)]), 255);
> -    checkUchar(program, callFunction(program, "foo", [makeUchar(program, -1), makeUint(program, 5)]), 255);
> +    checkUchar(program, callFunction(program, "foo", [makeUchar(program, 65535), makeUint(program, 2)]), 63);
> +    checkUchar(program, callFunction(program, "foo", [makeUchar(program, -1), makeUint(program, 5)]), 7);

What? Why is this necessary?

[Thomas Denney]

(In reply to Myles C. Maxfield from comment #2)
> Comment on attachment 349399 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349399&action=review
> 
> > Tools/WebGPUShadingLanguageRI/Casts.js:45
> > +function castToBool(value)
> > +{
> > +    return !!value;
> > +}
> 
> Can we put this with the rest of the casts?

It is!

> 
> > Tools/WebGPUShadingLanguageRI/Test.js:83
> > -    return TypedValue.box(program.intrinsics.int, value);
> > +    return TypedValue.box(program.intrinsics.int, castToInt(value));
> >  }
> >  
> >  function makeUint(program, value)
> >  {
> > -    return TypedValue.box(program.intrinsics.uint, value);
> > +    return TypedValue.box(program.intrinsics.uint, castToUint(value));
> >  }
> >  
> >  function makeUchar(program, value)
> >  {
> > -    return TypedValue.box(program.intrinsics.uchar, value);
> > +    return TypedValue.box(program.intrinsics.uchar, castToUchar(value));
> >  }
> >  
> >  function makeBool(program, value)
> >  {
> > -    return TypedValue.box(program.intrinsics.bool, value);
> > +    return TypedValue.box(program.intrinsics.bool, castToBool(value));
> >  }
> >  
> >  function makeFloat(program, value)
> >  {
> > -    return TypedValue.box(program.intrinsics.float, value);
> > +    return TypedValue.box(program.intrinsics.float, castToFloat(value));
> >  }
> >  
> >  function makeHalf(program, value)
> >  {
> > -    return TypedValue.box(program.intrinsics.half, value);
> > +    return TypedValue.box(program.intrinsics.half, castToHalf(value));
> 
> perhaps instead of silently casting, we should throw if the input thing
> isn't already the right type. The caller should know if the only reason the
> test is passing is that the type system is interfering

OK, I’ll add this now.

> 
> > Tools/WebGPUShadingLanguageRI/Test.js:2702
> > -    checkUchar(program, callFunction(program, "foo", [makeUchar(program, 65535), makeUint(program, 2)]), 255);
> > -    checkUchar(program, callFunction(program, "foo", [makeUchar(program, -1), makeUint(program, 5)]), 255);
> > +    checkUchar(program, callFunction(program, "foo", [makeUchar(program, 65535), makeUint(program, 2)]), 63);
> > +    checkUchar(program, callFunction(program, "foo", [makeUchar(program, -1), makeUint(program, 5)]), 7);
> 
> What? Why is this necessary?

makeUchar of -1 yields the value 0xFF (likewise with 65535), so right shifting these values by 2 and 5 yields 63 and 7 respectively.

[Myles C. Maxfield]

Comment on attachment 349399 [details]
Patch

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

>>> Tools/WebGPUShadingLanguageRI/Casts.js:45
>>> +}
>> 
>> Can we put this with the rest of the casts?
> 
> It is!

Nevermind, you're right.

>>> Tools/WebGPUShadingLanguageRI/Test.js:2702
>>> +    checkUchar(program, callFunction(program, "foo", [makeUchar(program, -1), makeUint(program, 5)]), 7);
>> 
>> What? Why is this necessary?
> 
> makeUchar of -1 yields the value 0xFF (likewise with 65535), so right shifting these values by 2 and 5 yields 63 and 7 respectively.

I see, the test was testing incorrect behavior before. Either way, the test shouldn't say makeUchar(program, -1). It should say makeUchar(program, 255). Same with 65535.

[Thomas Denney]

Created attachment 349429 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 349429 [details]
Patch

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

> Tools/WebGPUShadingLanguageRI/All.js:105
> +load("IsBitwiseEquivalent.js");

Why not put this in Casts.js?

> Tools/WebGPUShadingLanguageRI/Casts.js:126
> +        throw new Error(`${value} was casted to ${castedValue}`);

This message is not particularly clear why it is an error condition.

[Thomas Denney]

Created attachment 349456 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 349456 [details]
Patch

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

> Tools/WebGPUShadingLanguageRI/Test.js:-2625
> -    checkUint(program, callFunction(program, "foo", [makeUint(program, -1), makeUint(program, 5)]), 134217727);

Can we replace all of these tests with 2^32-1 instead?

[Thomas Denney]

Created attachment 350126 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 350126 [details]
Patch

Clearing flags on attachment: 350126

Committed r236217: <https://trac.webkit.org/changeset/236217>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44613690>