Bug 195771 - [WHLSL] Implement zero-filling of types
Summary: [WHLSL] Implement zero-filling of types
Status: RESOLVED DUPLICATE of bug 198433
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on: 198426
Blocks:
  Show dependency treegraph
 
Reported: 2019-03-14 15:19 PDT by Myles C. Maxfield
Modified: 2019-06-19 12:54 PDT (History)
3 users (show)

See Also:


Attachments
patch (40.42 KB, patch)
2019-05-31 02:34 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2019-03-14 15:19:41 PDT
Implement zero-filling of types
Comment 1 Radar WebKit Bug Importer 2019-05-13 17:07:45 PDT
<rdar://problem/50746291>
Comment 2 Myles C. Maxfield 2019-05-25 09:46:38 PDT
We should also consider how to zero-fill unused inputs/outputs of entry points.
Comment 3 Saam Barati 2019-05-31 02:34:39 PDT
Created attachment 371047 [details]
patch
Comment 4 Saam Barati 2019-05-31 02:35:35 PDT
(In reply to Myles C. Maxfield from comment #2)
> We should also consider how to zero-fill unused inputs/outputs of entry
> points.

I didn't do anything here. I should figure out what needs to be done.
Comment 5 Myles C. Maxfield 2019-06-05 10:01:44 PDT
Comment on attachment 371047 [details]
patch

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

Does the Metal compiler still produce warnings now? GPURenderPipelineMetal.mm has a FIXME with a link to this bug.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLAutoInitializeVariables.cpp:54
> +        // Skip argument declarations.

Cool.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLAutoInitializeVariables.cpp:66
> +        printStream.print(TypeDumper(*type));

🤔 This seems unwise, though I do understand how it isn't observable. Are you confident this is a good idea?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLChecker.cpp:-360
> -        auto* overload = resolveFunctionOverloadImpl(*getterFuncs, argumentTypeReferences, nullptr);

Excellent.

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLVariableDeclaration.h:73
> +    void setInitializer(UniqueRef<Expression> expression)

Doesn't this need to take an rvalue reference? UniqueRef is move-only (I think???)

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:453
>          m_stringBuilder.append(makeString(m_typeNamer.mangledNameForType(*variableDeclaration.type()), ' ', variableName, ";\n"));

Is this dead code now?

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLNativeFunctionWriter.cpp:110
> +            stringBuilder.append(makeString("    for (size_t i = 0; i < sizeof(", metalReturnName, "); ++i) {\n"));
> +            stringBuilder.append("        ptr[i] = 0;\n");
> +            stringBuilder.append("    }\n");

Can we remove the { } and pull it up into one line?
Comment 6 Myles C. Maxfield 2019-06-05 10:02:05 PDT
(In reply to Saam Barati from comment #4)
> (In reply to Myles C. Maxfield from comment #2)
> > We should also consider how to zero-fill unused inputs/outputs of entry
> > points.
> 
> I didn't do anything here. I should figure out what needs to be done.

Is there another bug filed for this?
Comment 7 Myles C. Maxfield 2019-06-05 10:05:26 PDT
Comment on attachment 371047 [details]
patch

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

> Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLNativeFunctionWriter.cpp:109
> +            stringBuilder.append("        ptr[i] = 0;\n");

I think this "memset" is going to be needed for the follow-up task (of inputs and outputs). Can we pull this out into a standalone metal function?
Comment 8 Saam Barati 2019-06-05 23:29:00 PDT
Comment on attachment 371047 [details]
patch

I should have obsoleted this patch
Comment 10 Saam Barati 2019-06-10 11:36:30 PDT
(In reply to Myles C. Maxfield from comment #7)
> Comment on attachment 371047 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=371047&action=review
> 
> > Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLNativeFunctionWriter.cpp:109
> > +            stringBuilder.append("        ptr[i] = 0;\n");
> 
> I think this "memset" is going to be needed for the follow-up task (of
> inputs and outputs). Can we pull this out into a standalone metal function?

I think we should do this when it's needed, as it has added complexity. I also think there is a chance it's not needed, depending on if we define things to be a compile time error or not.
Comment 11 Saam Barati 2019-06-19 12:54:36 PDT
All that's left here is:
https://bugs.webkit.org/show_bug.cgi?id=198433

So marking as a dupe
Comment 12 Saam Barati 2019-06-19 12:54:39 PDT

*** This bug has been marked as a duplicate of bug 198433 ***