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

Slightly fixed shaders building pipeline #19

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

yaroslavyaroslav
Copy link
Collaborator

Previously it included debug symbols unconditionally.

Previously it included debug symbols unconditionally.
Copy link
Collaborator

@FoodChain1028 FoodChain1028 left a comment

Choose a reason for hiding this comment

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

Overall Looks Great to me, but having some personal questions :)

.map(|profile| profile == "release")
.unwrap_or(false)
{
if cfg!(feature = "profiling-release") {
args.push("-frecord-sources");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some questions here:

  1. Do we generate Metal Library Symbol File via this argument (-frecord-release)?
  2. do we use the symbol files to do the profile our shaders?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, sure

  1. We do, according to this -frecord-release this attribute does exactly this, that's why I moved it into the separate feature.
  2. It's not quite, we're not required these symbols to be provided to exactly be capable to profile, but we still required these symbols to be provided to be capable to see what exactly shader is running. They're pretty much the same as the symbol files for ordinary binaries, they do nothing more but to enrich debug data with the exact function names (shader names in case of metal shaders obviously).
  3. According the same source we can skip the .air intermediate compilation step and to compile shaders to its binary representation in a single shot, but I decided to implement this improvement after the refactor(metal_mont_backend): adapted from https://github.com/geometryxyz/msl-secp256k1 #13 will be merged.

@yaroslavyaroslav yaroslavyaroslav merged commit a553c49 into main Dec 16, 2024
4 checks passed
@yaroslavyaroslav yaroslavyaroslav deleted the shaders-pipeline-fix branch December 16, 2024 20: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