Skip to content

fixed incorrect packed_int3 conversion generated for Metal - #9355

Merged
ErichDonGubler merged 1 commit into
gfx-rs:trunkfrom
39ali:metal-type3
Apr 2, 2026
Merged

fixed incorrect packed_int3 conversion generated for Metal#9355
ErichDonGubler merged 1 commit into
gfx-rs:trunkfrom
39ali:metal-type3

Conversation

@39ali

@39ali 39ali commented Apr 1, 2026

Copy link
Copy Markdown
Contributor

Connections
#9091

Description
translated MSL now correctly unpacks packed_type3 before using it with other unpacked types.

Testing
added a test that test against this issue

Squash or Rebase?
squash

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy --tests. If applicable, add:
    • --target wasm32-unknown-unknown
  • Run cargo xtask test to run tests.
  • If this contains user-facing changes, add a CHANGELOG.md entry.

Comment thread naga/tests/out/msl/wgsl-globals.metal Outdated
metal::float3 svm0_ = data.v3_ * 2.0;
metal::float3 svm1_ = 2.0 * data.v3_;
metal::float3 l0_ = metal::float3(data.v3_);
metal::float2 l1_ = metal::float3(metal::float3(data.v3_)).zx;

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.

question: This double cast seems incorrect, though I suspect it's harmless, but I'm not sure. @teoxoy or @jimblandy, do you know? It's also suspect that this is already being emitted in some cases, but not others. Perhaps we need to plumb a need to cast somewhere else, rather than doing it in multiple places?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

after inspecting the metalIR this will get optimized away, in fact the compiler will handle most of the conversion between packed and unpacked, and it looks like the only issue is with the bit-cast from packed type to unpacked, i'll update the code to handle that

@ErichDonGubler ErichDonGubler 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.

I'll take on review. This broadly looks like it improves things, but I have questions about the approach WRT double casts.

@ErichDonGubler ErichDonGubler 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.

Noice! Much simpler. 👍🏻

@ErichDonGubler
ErichDonGubler merged commit c580922 into gfx-rs:trunk Apr 2, 2026
58 checks passed
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