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.

Sign up for GitHub

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

Jump to bottom

[hlsl-out] clear named_expressions inserted by duplicated blocks #2116

Merged
merged 1 commit into from
Jan 31, 2023
Merged

[hlsl-out] clear named_expressions inserted by duplicated blocks #2116

merged 1 commit into from
Jan 31, 2023

Conversation

teoxoy
Copy link
Member

@teoxoy teoxoy commented Nov 4, 2022

fixes #1972

@teoxoy teoxoy requested a review from jimblandy November 5, 2022 10:02
jimblandy
jimblandy reviewed Nov 9, 2022
View reviewed changes
src/back/hlsl/writer.rs Outdated Show resolved Hide resolved
@teoxoy
Copy link
Member Author

teoxoy commented Nov 10, 2022

Another way of doing this (which avoids the recursion) would be to use indexmap::IndexMap for named_expressions and use .len() + .truncate(prev_len).

I'll push a commit that does this (which I can revert if we don't want to go this route).

@teoxoy
Copy link
Member Author

teoxoy commented Nov 10, 2022

Seems like we need indexmap-rs/indexmap#215 for that to work...

@teoxoy
Copy link
Member Author

teoxoy commented Nov 10, 2022

I'll revert the last commit, adding Arbitrary support to indexmap also requires adding it to hashbrown. If we want to make it a newtype instead, it also gets really ugly since we need to expose all the methods of the inner IndexMap type.

@cuviper
Copy link

cuviper commented Nov 17, 2022

adding Arbitrary support to indexmap also requires adding it to hashbrown.

I'm not sure why you think so... indexmap-rs/indexmap#246

@teoxoy
Copy link
Member Author

teoxoy commented Nov 17, 2022

@cuviper a PR, that's great! I was looking into deriving it (which would have required RawTable in hashbrown to also impl the trait).

@teoxoy
Copy link
Member Author

teoxoy commented Nov 17, 2022

@cuviper do you have an ETA on that PR landing and possibly a new release?

@cuviper
Copy link

cuviper commented Nov 17, 2022

Ah, deriving the fields directly would be unlikely to produce a coherent map. I just followed how each Arbitrary is implemented for a regular HashMap, with an iterator of (K, V) items to collect, or just T items for a set.

I went ahead and merged that on master. That's currently in a limbo 2.0.0 state though, which I should probably get around to publishing already, but I could also backport it to the 1.x series if you need that.

@teoxoy
Copy link
Member Author

teoxoy commented Nov 17, 2022

If it's not too much trouble, it would be greatly appreciated :)

@cuviper
Copy link

cuviper commented Nov 17, 2022

Done -- indexmap 1.9.2

@teoxoy teoxoy force-pushed the clear_named_expressions branch 3 times, most recently from 04ccbab to 9522e2a Compare November 18, 2022 16:30
@ErichDonGubler
Copy link
Member

@jimblandy: Were you still going to review this for another round? I'm thinking of trying to tackle this.

@jimblandy
Copy link
Member

Now that I actually understand what it's doing (from reviewing the switch PR), I was going to look it over again. Although, I'm pretty buried in #2075 right now.

ErichDonGubler
ErichDonGubler approved these changes Jan 24, 2023
View reviewed changes
Copy link
Member

@ErichDonGubler ErichDonGubler left a comment

Choose a reason for hiding this comment

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

LGTM! Some non-blocking meta nits:

  • I sorta wish the justification for indexmap would land in the message of the commit introducing it, rather than just being GH discussion. That would make it easier for folks examining history. git commit --fixup=amend… is your friend!
  • It's sorta odd to introduce a solution to the problem then immediately replace it in the same PR, but I don't see any issue with it.

tests/out/hlsl/control-flow.hlsl Outdated
Comment on lines 122 to 133
switch(0) {
case 2: {
{
}
{
int i_1 = int(1);
}
break;
}
default: {
int i_2 = int(1);
break;
Copy link
Member

Choose a reason for hiding this comment

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

thought: It's interesting that the hlsl backend doesn't eliminate the dead code in this test case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we don't have any optimization passes. All the compilers naga targets have their own already.

@teoxoy
Copy link
Member Author

teoxoy commented Jan 31, 2023

Thanks for the review!

It's sorta odd to introduce a solution to the problem then immediately replace it in the same PR, but I don't see any issue with it.

I left all commits in for now because I wanted some feedback on both approaches and was planning to squash everything when rebasing (before merging).

I sorta wish the justification for indexmap would land in the message of the commit introducing it, rather than just being GH discussion. That would make it easier for folks examining history. git commit --fixup=amend… is your friend!

Will do.

@teoxoy
Copy link
Member Author

teoxoy commented Jan 31, 2023

Squashed and rebased. I had to remove the test since we removed support for fallthrough for the wgsl frontend in #2031.

ErichDonGubler
ErichDonGubler approved these changes Jan 31, 2023
View reviewed changes
Copy link
Member

@ErichDonGubler ErichDonGubler left a comment

Choose a reason for hiding this comment

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

Still LGTM!

@teoxoy
[hlsl-out] clear named_expressions inserted by duplicated blocks
8fe0970 
changed the type of `named_expressions` from `HashMap` to `IndexMap` so that insertion order is preserved
jimblandy
jimblandy approved these changes Jan 31, 2023
View reviewed changes
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 great - much simpler than the original fix. It's a pity we can't test this.

@jimblandy jimblandy merged commit a2b39e4 into gfx-rs:master Jan 31, 2023
@teoxoy teoxoy deleted the clear_named_expressions branch January 31, 2023 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimblandy jimblandy jimblandy approved these changes

@ErichDonGubler ErichDonGubler ErichDonGubler approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[hlsl-out] scoping issue with switch statement fallthrough
4 participants
@teoxoy @cuviper @ErichDonGubler @jimblandy