[webkit-reviews] review granted: [Bug 201240] [WHLSL] Add an analysis to verify that functions like control barriers and derivatives are not called in non-uniform control flow. : [Attachment 378768] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Robin Morisset
<rmorisset at apple.com>'s request for review:
Bug 201240: [WHLSL] Add an analysis to verify that functions like control
barriers and derivatives are not called in non-uniform control flow.
https://bugs.webkit.org/show_bug.cgi?id=201240

Attachment 378768: Patch

https://bugs.webkit.org/attachment.cgi?id=378768&action=review




--- Comment #24 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 378768
  --> https://bugs.webkit.org/attachment.cgi?id=378768
Patch

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

r=me

I'd like you to address my earlier comments that weren't addressed before
landing. Most of them are uninteresting comments, like creating bugs, and
changing TODO to FIXME, etc.

> Source/WebCore/ChangeLog:24
> +	   Its error messages are also still terrible, I hope to improve them
in a later patch that will make the source accessible to all passes.

please link to a bug for this here

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLCodeLocation.h:94
> +	   if (m_startOffset != std::numeric_limits<unsigned>::max())
> +	       out.print(m_startOffset, "-", m_endOffset);

this is still wrong. See my earlier comment

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPrepare.cpp:94
> +static constexpr bool alwaysDumpPassFailures = true;

revert this

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLStatementBehaviorChecker.cpp:45
> + The former is a fairly straightforward kind of analysis listing for each
statement the ways that control-flow can exit it (its "Behaviors"), in order to
forbid things like "continue;" outside of loops, reaching the end of non-void
functions, "fallthrough;" outside of switches, etc..

"etc.." => "etc."

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLStatementBehaviorChecker.cpp:56
> + 17

remove

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLStatementBehaviorChecker.cpp:89
> +    // Invariant 2: value -> controlFlow, meaning that the value can only be
uniform if the controlFlow is as well.

"controlFlow" => "controlFlowAfter"

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLStatementBehaviorChecker.cpp:101
> +    // TODO: decide whether or not to add an InfiniteLoop behavior.

As I mentioned in prior comments, these should be FIXMEs. All ideally linking
to a bug

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLStatementBehaviorChecker.cpp:126
> +    // so controlFlow is always MustBeUniformToReturnUniformly or lower.

"or lower" => "or lower for non-void functions"

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLStatementBehaviorChecker.cpp:146
> +	       ASTDumper dumper;
> +	      
dumper.visit(const_cast<AST::FunctionDefinition&>(functionDefinition));
> +	       dataLogLn(dumper.toString());

same comment as prior comment

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLStatementBehaviorChecker.cpp:155
> +	   if (m_functionMetadata.contains(&m_functionDefinition))
> +	       return m_functionMetadata.get(&m_functionDefinition);

you should avoid two hash lookups here by doing:

{
    auto iter = m_functionMetadata.find(&m_functionDefinition);
    if (item != m_functionMetadata.end())
	return iter->value;
}

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLStatementBehaviorChecker.cpp:163
> +	       RELEASE_ASSERT(node);

why?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLStatementBehaviorChecker.cpp:196
> +	       auto node = m_cannotBeUniform;
> +	       StringBuilder stringBuilder;
> +	       dataLogLn("Uniformity error!");
> +	       while (node != m_mustBeUniform) {
> +		   dataLogLn("\tBecause of location ", parents[node].second, "
(node ", parents[node].first, " -> ", node, ")");
> +		   node = parents[node].first;
> +	       }
> +	       return makeUnexpected(Error("This function has a uniformity
error "));
> +	       // TODO: give a good error message.

you didn't address my comments here from previous patch

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLStatementBehaviorChecker.cpp:430
> +	   const auto& lastStatement = statements[statements.size() - 1];

nit: you want .last()

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLStatementBehaviorChecker.cpp:465
> +		   } else { // Native function declarations

same as previous nit: no need for this comment

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLStatementBehaviorChecker.cpp:656
> +	       // FIXME: we should never arrive here, but for now I have this
code as a work-around for a bug in the property resolver.

can you just run this before the property resolver? Seems like there is no need
for this code. PropertyResolver should not change the uniformity of programs.