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

Remove unneeded Storage Class attributes #568

Open
XAMPPRocky opened this issue Mar 31, 2021 · 4 comments · May be fixed by #807 or #1142
Open

Remove unneeded Storage Class attributes #568

XAMPPRocky opened this issue Mar 31, 2021 · 4 comments · May be fixed by #807 or #1142
Labels
t: enhancement A new feature or improvement to an existing one. t: good first issue Good for newcomers

Comments

@XAMPPRocky
Copy link
Member

XAMPPRocky commented Mar 31, 2021 •
edited

With recent changes to how we specify the storage class of a variable, we've managed to eliminate the need for a few of the storage class attributes in cases where we can successfully infer all possible valid uses. Mainly the input, output, private, function, generic, and uniform_constant attributes at the time of this writing are no longer needed and should be removed now that they can't be used for anything useful.
@XAMPPRocky XAMPPRocky added t: enhancement A new feature or improvement to an existing one. t: good first issue Good for newcomers labels Mar 31, 2021
@andrewleverette
Copy link

andrewleverette commented Nov 24, 2021 •
edited

I would be happy to work on this. I've found the type and have removed the specified variants. I'm curious how you would want to handle cases where the variants are used. In some cases where they are used for error handling, I think those checks can just be removed. But a case that isn't as clear is handling cases that are dependent on module version like this:

/rustc_codegen_spriv/src/linker/entry_interface.rs

        // Base case: the global itself is an interface-relevant `OpVariable`.
        let interface_relevant_var = inst.class.opcode == Op::Variable && {
            if version > (1, 3) {
                // SPIR-V >= v1.4 includes all OpVariables in the interface.
                true
            } else {
                let storage_class = inst.operands[0].unwrap_storage_class();
                // SPIR-V <= v1.3 only includes Input and Output in the interface.
                storage_class == StorageClass::Input || storage_class == StorageClass::Output
            }
        };

Should the else case just return false? Or is there some other logic that should be added here?

Another case is when a default is used like here:
rustc_codegen_spriv/src/codegen_cx/entry.rs

        // If storage class was not inferred nor specified, compute the default (i.e. input/output)
        let storage_class = inferred_storage_class_from_ty
            .or(attr_storage_class)
            .unwrap_or_else(|| match (is_ref, mutbl) {
                (false, _) => StorageClass::Input,
                (true, hir::Mutability::Mut) => StorageClass::Output,
                (true, hir::Mutability::Not) => self.tcx.sess.span_fatal(
                    hir_param.ty_span,
                    &format!(
                        "invalid entry param type `{}` (expected `{}` or `&mut {1}`)",
                        layout.ty, value_ty
                    ),
                ),
            });

Should this just call the span_fatal if the storage calls was not inferred?

@msiglreith
Copy link
Contributor

msiglreith commented Nov 24, 2021 •
edited

@andrewleverette I think this only refers to the #spirv(..) attributes which can be explicitly specified by the user and not the storage classes as a whole as they are still needed (just inferred by compiler).

rust-gpu/crates/rustc_codegen_spirv/src/symbols.rs

Lines 138 to 165 in d789b58

const STORAGE_CLASSES: &[(&str, StorageClass)] = {
use StorageClass::*;
&[
("uniform_constant", UniformConstant),
("input", Input),
("uniform", Uniform),
("output", Output),
("workgroup", Workgroup),
("cross_workgroup", CrossWorkgroup),
("private", Private),
("function", Function),
("generic", Generic),
("push_constant", PushConstant),
("atomic_counter", AtomicCounter),
("image", Image),
("storage_buffer", StorageBuffer),
("callable_data", StorageClass::CallableDataKHR),
(
"incoming_callable_data",
StorageClass::IncomingCallableDataKHR,
),
("ray_payload", StorageClass::RayPayloadKHR),
("hit_attribute", StorageClass::HitAttributeKHR),
("incoming_ray_payload", StorageClass::IncomingRayPayloadKHR),
("shader_record_buffer", StorageClass::ShaderRecordBufferKHR),
("physical_storage_buffer", PhysicalStorageBuffer),
]
};

@andrewleverette
Copy link

Okay so sorry if this is a dumb question, but is this issue literally just removing those specified attributes from STORAGE_CLASSES?

@andrewleverette andrewleverette linked a pull request Nov 24, 2021 that will close this issue
Open
@andrewleverette
Copy link

I think the linked pull request will resolve this issue. I'm still new to open source, so I'm definitely open to any feedback.

JulianKnodt added a commit to JulianKnodt/rust-gpu that referenced this issue Apr 10, 2024
@JulianKnodt
Removed unused storage classes
27a91bc 
Fixes EmbarkStudios#568
@JulianKnodt JulianKnodt linked a pull request Apr 10, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
t: enhancement A new feature or improvement to an existing one. t: good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove unused attributes from storage class
Removed unused storage classes JulianKnodt/rust-gpu
3 participants
@XAMPPRocky @msiglreith @andrewleverette