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

[MSL] remap_variable_type_name is largely absent from MSL code emission. #2134

Open
forbiddencactus opened this issue Apr 6, 2023 · 2 comments
Labels
question Further progress depends on answer from issue creator.

Comments

@forbiddencactus
Copy link

forbiddencactus commented Apr 6, 2023

set_variable_type_remap_callback does not work on a MSL compiler because remap_variable_type_name is largely absent in spriv_msl.cpp.

Two examples of where it's absent:

In void CompilerMSL::entry_point_args_discrete_descriptors(string &ep_args)

The image type emission should look more like this:

case SPIRType::Image:
		{
			if (!ep_args.empty())
				ep_args += ", ";

			// Use Metal's native frame-buffer fetch API for subpass inputs.
			const auto &basetype = get<SPIRType>(var.basetype);
			if (!type_is_msl_framebuffer_fetch(basetype))
			{
				auto theTypeName = image_type_glsl(type, var_id);
				remap_variable_type_name(type, r.name, theTypeName);
				ep_args += theTypeName + " " + r.name;
				if (r.plane > 0)
					ep_args += join(plane_name_suffix, r.plane);
				ep_args += " [[texture(" + convert_to_string(r.index) + ")";
				if (interlocked_resources.count(var_id))
					ep_args += ", raster_order_group(0)";
				ep_args += "]]";
			}

In string CompilerMSL::to_struct_member(const SPIRType &type, uint32_t member_type_id, uint32_t index, const string &qualifier)

The last few lines of that method should look more like this:

	string typeString = type_to_glsl(*declared_type, orig_id);
	string memberName = to_member_name(type, index);

	remap_variable_type_name(*declared_type, memberName, typeString);
	auto result = join(pack_pfx,typeString, " ", qualifier, memberName,
	                   member_attribute_qualifier(type, index), array_type, ";");

	is_using_builtin_array = false;
	return result;

The call to the remap callback is largely absent from whenever CompilerMSL overrides CompilerGLSL.

@HansKristian-Work
Copy link
Contributor

The variable remap stuff was never really meant to remap names other than the weird GLSL scenarios where it's meaningful, i.e. rename variables to remap to builtins to poke at specific extensions. Do you have a concrete use case where remapping variables would make sense in MSL? Remapping the names in these places won't work that well I think since both the declaration and any uses of the variable would have to know about the name, but it's probably solvable given some effort.

@HansKristian-Work HansKristian-Work added the question Further progress depends on answer from issue creator. label Apr 20, 2023
@forbiddencactus
Copy link
Author

forbiddencactus commented May 2, 2023

Sorry for my late reply!

I personally use it in two situations: the first... to parse Texture<float> > Texture<half> for variables with a RelaxedPrecision decorator, so it passes SPIR-V validation. The second... to remap a wrapper for the atomics issue in MSL emission where certain decorated variables are remapped into atomic<type>. I generally find it super useful to bridge the expressiveness gap between HLSL, SPIR-V, and MSL sometimes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further progress depends on answer from issue creator.
Projects
None yet
Development

No branches or pull requests

2 participants