Bug 200352 – [WHLSL] Do simple nullptr check elimination using basic data flow analysis when generating metal code

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

[Saam Barati]

...

[Saam Barati]

Created attachment 375341 [details]
patch

[EWS Watchlist]

Attachment 375341 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:562:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:564:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:607:  Extra space before [.  [whitespace/brackets] [5]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:681:  Extra space before [.  [whitespace/brackets] [5]
Total errors found: 4 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Robin Morisset]

Comment on attachment 375341 [details]
patch

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

LGTM overall, just a couple questions inline

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:175
> +    void appendRightValueWithNullability(AST::Expression&, String value, Nullability nullability)

Is there a reason to make it a separate function instead of just adding an optional parameter "Nullability nullability = Nullability::CanBeNull" to appendRightValue?

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:562
> +    auto [pointerName, nullability] = takeLastLeftValue();

This is a very sweet syntax, but it seems only allowed in C++17. Have we officially given up support of C++14?

[Myles C. Maxfield]

Comment on attachment 375341 [details]
patch

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

> Source/WebCore/ChangeLog:9
> +        When doing metal code generation, we frequently know whether something
> +        is null or not. This patch does a basic propagation of this information

Sorry for being dumb, but do you think you could provide an example?

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:196
> +    std::pair<String, Nullability> takeLastValueAndNullability()

Adding this stuff into a pair seems yucky. I expect in the future we'll be adding more and more data here. Do you think it would be better design to put this into a struct? Or just use StackItem directly?

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:668
> +        auto lValue = takeLastLeftValue().first;

Again, a struct type would be a better design here. Unclear what "first" means.

[Saam Barati]

Comment on attachment 375341 [details]
patch

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

>> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:175
>> +    void appendRightValueWithNullability(AST::Expression&, String value, Nullability nullability)
> 
> Is there a reason to make it a separate function instead of just adding an optional parameter "Nullability nullability = Nullability::CanBeNull" to appendRightValue?

I originally did it in the same function. I moved to separate function for grep-ability of it. I don't have a strong preference.

>> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:196
>> +    std::pair<String, Nullability> takeLastValueAndNullability()
> 
> Adding this stuff into a pair seems yucky. I expect in the future we'll be adding more and more data here. Do you think it would be better design to put this into a struct? Or just use StackItem directly?

I'll make a struct.

>> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:562
>> +    auto [pointerName, nullability] = takeLastLeftValue();
> 
> This is a very sweet syntax, but it seems only allowed in C++17. Have we officially given up support of C++14?

Yeah :-)

[Saam Barati]

Comment on attachment 375341 [details]
patch

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

>> Source/WebCore/ChangeLog:9
>> +        is null or not. This patch does a basic propagation of this information
> 
> Sorry for being dumb, but do you think you could provide an example?

will add an example.

[Saam Barati]

Created attachment 375358 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 375358 [details]
patch for landing

Clearing flags on attachment: 375358

Committed r248141: <https://trac.webkit.org/changeset/248141>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53837824>