Skip to content

Carry float flags through lowering and lifting - #48

Open
jlogan03 wants to merge 2 commits into
Rust-GPU:mainfrom
jlogan03:jlogan/float-flags
Open

jlogan03 wants to merge 2 commits into
Rust-GPU:mainfrom
jlogan03:jlogan/float-flags

Conversation

@jlogan03

@jlogan03 jlogan03 commented Oct 7, 2026 •

Copy link
Copy Markdown

This will simplify the addition of float controls to rust-gpu, which would otherwise require a workaround to preserve the flags.

@jlogan03
jlogan03 requested a review from eddyb as a code owner October 7, 2026 02:12

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

I've been playing with a slightly different version of this, that can also support e.g. OpDecorateId, if we ever need that, etc.

(leaving this review so we don't accidentally land both)

@jlogan03

jlogan03 commented Oct 7, 2026 •

Copy link
Copy Markdown
Author

@eddyb Want me to take a swing at generalizing this on my branch as well? I test-drove generalizing it locally and it actually made the change about 10 lines shorter, which feels like a good sign

@jlogan03

jlogan03 commented Oct 7, 2026

Copy link
Copy Markdown
Author

I went ahead and pushed the generalization since it's an improvement over the original even without considering the added functionality.

@eddyb

eddyb commented Oct 12, 2026

Copy link
Copy Markdown
Member

I test-drove generalizing it locally and it actually made the change about 10 lines shorter, which feels like a good sign

I was hoping to avoid anyone else spending any more effort on this, by pushing my version sooner, but it's up now, so you can check out the first 3 commits from:

(notably, Attr::SpvAnnotation is extended, forcing all use sites of Attr::SpvAnnotation to consider whether they need to care about ID operands, and spv::Inst is still outside the OrdAssertEq<Vec<_>>)

This branch has not been deployed

No deployments
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