Skip to content

difftest: cleanup wgpu runner#587

Merged
Firestar99 merged 8 commits into
mainfrom
difftest_runner_cleanup
Jul 13, 2026
Merged

difftest: cleanup wgpu runner#587
Firestar99 merged 8 commits into
mainfrom
difftest_runner_cleanup

Conversation

@Firestar99

@Firestar99 Firestar99 commented Apr 24, 2026

Copy link
Copy Markdown
Member

Extracted from #334

@Firestar99 Firestar99 force-pushed the difftest_runner_cleanup branch 2 times, most recently from 603efd7 to 408a0bb Compare April 24, 2026 11:29
@Firestar99 Firestar99 force-pushed the difftest_runner_cleanup branch from 408a0bb to c0cb9e0 Compare April 24, 2026 11:31
Base automatically changed from difftest_refactor to main April 24, 2026 14:02
@Firestar99 Firestar99 force-pushed the difftest_runner_cleanup branch 3 times, most recently from 81640a0 to f1250b9 Compare April 27, 2026 12:21

@eddyb eddyb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM modulo a few small things, anything else blocking this?

Comment thread tests/difftests/tests/lib/src/scaffold/compute/wgpu_runner.rs Outdated
Comment thread tests/difftests/tests/lib/src/scaffold/compute/wgpu_runner.rs Outdated
Comment thread tests/difftests/tests/lib/src/scaffold/compute/wgpu_runner.rs Outdated
Comment on lines +244 to +251
let (sender, receiver) = futures::channel::oneshot::channel();
buffer_slice.map_async(wgpu::MapMode::Read, move |res| {
let _ = sender.send(res);
});
device.poll(wgpu::PollType::wait_indefinitely())?;
block_on(receiver)
.context("mapping canceled")?
.context("mapping failed")?;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does the wgpu example runner do it this way? Or does it just unwrap the Result in the closure passed to map_async?

@Firestar99 Firestar99 May 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It actually does, that said, I'm not a huge fan of the download code AI wrote so let me refactor it a little. encoder.map_buffer_on_submit() is way cleaner as a map_async() replacement.

Also noticed it was only ever writing out the first writable buffer, if you had two the second one would just be ignored... I'll just append all the buffers. But I also don't want to spend too much time on this and rather completely refactor the difftest framework yet again, will propose my idea on zulip today.

@Firestar99 Firestar99 marked this pull request as ready for review July 8, 2026 10:25
@Firestar99 Firestar99 requested a review from LegNeato as a code owner July 8, 2026 10:25

@eddyb eddyb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should've landed this already - feel free to bump wgpu to 30 if it's straightforward.

@Firestar99 Firestar99 force-pushed the difftest_runner_cleanup branch from 6c892fd to dbbdea8 Compare July 13, 2026 11:44
@Firestar99

Copy link
Copy Markdown
Member Author

Updated it to wgpu 30, will merge when ready.

I also found the branch eddyb/checked-saturating-spv-difftest from I guess the discussions of PR #561, adding a new test for checked and saturating add, which is failing on current main.

@Firestar99 Firestar99 enabled auto-merge July 13, 2026 11:47
@Firestar99 Firestar99 added this pull request to the merge queue Jul 13, 2026
Merged via the queue into main with commit 5209165 Jul 13, 2026
23 checks passed
@Firestar99 Firestar99 deleted the difftest_runner_cleanup branch July 13, 2026 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants