Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[spv-in] Convert conditional backedges to break if. #2290

Merged
merged 1 commit into from
May 12, 2023

Conversation

eddyb
Copy link
Contributor

@eddyb eddyb commented Mar 23, 2023


The only test I added was the reduced do-while example (which only passes with my changes), but also note that the more complex Rust-GPU+SPIR-T motivating example, involving a Rust for loop, works as well now:

Before this PRAfter this PR
thread 'main' panicked at 'wgpu error: Validation Error

Caused by:
    In Device::create_shader_module
    
Shader validation error: 
  ┌─ :1:1
  │
1 │ 
  │   naga::Function [1]


    Function [1] '' is invalid
    The `break` is used outside of a `loop` or `switch` context

image

@codecov-commenter
Copy link

Codecov Report

Merging #2290 (2c825d9) into master (53d62b9) will decrease coverage by 0.02%.
The diff coverage is 63.63%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master    #2290      +/-   ##
==========================================
- Coverage   82.19%   82.17%   -0.02%     
==========================================
  Files          82       82              
  Lines       44267    44342      +75     
==========================================
+ Hits        36384    36437      +53     
- Misses       7883     7905      +22     
Impacted Files Coverage Δ
src/front/spv/function.rs 67.88% <60.75%> (-1.17%) ⬇️
src/front/spv/mod.rs 53.43% <66.66%> (-0.01%) ⬇️
src/block.rs 87.00% <100.00%> (+8.42%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than handling this in BlockContext::lower, it seems like it would be cleaner to detect conditional backedges in Frontend::next_block's match arm for Op::BranchConditional.

Rather than pushing a BodyFragment::If onto the end of ctx.bodies[body_idx] which BlockContext::lower then has to analyze, next_block could notice that the block it's finishing is a continue target (by checking ctx.mergers), verify that the conditional branch's true_id and false_id each refer to the loop header or loop merge block, synthesize a negation expression as necessary, and then stash the proper condition expression handle somewhere. (Perhaps BodyFragment::Loop should have a break_if: Option<Handle<Expression>> field?)

I think this should be less circuitous - we'll be taking advantage of what SPIR-V promises us, rather than building weird Naga IR and then taking it apart again.

@eddyb eddyb changed the title [WIP] [spv-in] Convert conditional backedges to break if. [spv-in] Convert conditional backedges to break if. Apr 27, 2023
@eddyb eddyb marked this pull request as ready for review April 27, 2023 10:03
@eddyb eddyb requested a review from jimblandy April 27, 2023 11:05
Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good - thank you for the fix!

I worry that that Op::BranchConditional match arm is getting pretty long and difficult to follow. I wonder if it's not possible to organize it a bit. But, that doesn't have to be part of this PR.

src/front/spv/mod.rs Show resolved Hide resolved
src/front/spv/mod.rs Outdated Show resolved Hide resolved
src/front/spv/mod.rs Outdated Show resolved Hide resolved
src/front/spv/mod.rs Show resolved Hide resolved
@jimblandy jimblandy merged commit 423a069 into gfx-rs:master May 12, 2023
@eddyb eddyb deleted the spv-in-break-if branch May 12, 2023 20:59
jimblandy pushed a commit that referenced this pull request Jun 22, 2023
…eak if`). (#2387)

* [spv-in] Convert conditional backedges to `break if`.

* Work around pre-Rust2021 semantics (array `.into_iter()` and closure captures).

* Update `do-while` expected output for naga v0.10 backend behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants