Is it necessary to flatten everything in a single commit? Or could we e.g. first do IF/ELSE/ENDIF, then LOOP/ENDLOOP/BREAK/CONTINUE, SWITCH/ENDSWITCH/CASE/etc.?
IMHO in this specific case splitting commits creates more complexity rather than reducing it, mostly because the intermediate steps do not make much sense. For example, if on 4/7 I try to compile this pseudo-code:
float4 main(float4) { for (...) { if (...) { do_something(); } } }then the selection construct will cause the loop body to be split in different blocks. In particular, the `loop` and `endloop` will end up in different blocks, which doesn't make a lot of sense to me, even syntactically.
This could be worked around by not translating constructs that appear inside the body of an outer construct which is not itself translated yet in intermediate versions.
Right.
However, to me, the easiest way to check that the modifications Conor is doing are sensible is to verify the new version from scratch, which is not that difficult even if it's a lot of lines, because the code is well structured and relatively straightforward. Having more commits is more complicated because then I have to check the intermediate versions too (and, as I said, I'm not even sure of what they're supposed to do anyway). Since is very short lived code anyway, I'll just check the final version (or, rather, I already did that).
It's not just about review though. It's at least as much about being able to get helpful results from bisects when things break, possibly on a remote machine.
+static void src_param_init_label(struct vkd3d_shader_src_param *param, unsigned int label_id0, unsigned int label_id1) +{ + param->swizzle = 0; + param->modifiers = VKD3DSPSM_NONE; + vsir_register_init(¶m->reg, VKD3DSPR_LABEL, VKD3D_DATA_UINT, 1 + !!label_id1); + param->reg.dimension = VSIR_DIMENSION_NONE; + param->reg.idx[0].offset = label_id0; + param->reg.idx[1].offset = label_id1; +}
I'm not sold on using a single VKD3DSPR_LABEL register to hold two targets. Is there a specific issue this is addressing, or is this simply an attempt to reduce the number of parameters to the instruction?
case VKD3DSIH_ENDSWITCH: + { + struct vkd3d_shader_src_param *src_params; + unsigned int j; + + if (!cf_info->u.switch_.default_block_id) + cf_info->u.switch_.default_block_id = cf_info->u.switch_.merge_block_id; + + cf_flattener_emit_label(flattener, cf_info->u.switch_.merge_block_id); + + /* The SWITCH instruction is completed when the endswitch + * instruction is processed because we do not know the number + * of case statements or the default block id in advance.*/ + dst_ins = &flattener->instructions[cf_info->u.switch_.ins_location]; + if (!(src_params = instruction_src_params_alloc(dst_ins, cf_info->u.switch_.cases_count + 2, flattener))) + { + vkd3d_free(cf_info->u.switch_.cases); + return VKD3D_ERROR_OUT_OF_MEMORY; + } + src_params[0] = *cf_info->u.switch_.condition; + src_param_init_label(&src_params[1], cf_info->u.switch_.default_block_id, + cf_info->u.switch_.merge_block_id); + for (j = 0; j < cf_info->u.switch_.cases_count; ++j) + { + src_param_init_label(&src_params[j + 2], cf_info->u.switch_.cases[j].block_id, 0); + src_params[j + 2].reg.dimension = VSIR_DIMENSION_SCALAR; + src_params[j + 2].reg.u.immconst_uint[0] = cf_info->u.switch_.cases[j].value; + }
Somewhat similarly, do we have a reason to avoid VKD3DSPR_IMMCONST here?
+ location = vkd3d_spirv_stream_current_location(stream); + vkd3d_spirv_build_op_switch(builder, val_id, + spirv_compiler_get_label_id(compiler, src[1].reg.idx[0].offset), NULL, 0); + for (i = 2; i < instruction->src_count; ++i) + { + vkd3d_spirv_build_word(stream, src[i].reg.u.immconst_uint[0]); + vkd3d_spirv_build_word(stream, spirv_compiler_get_label_id(compiler, src[i].reg.idx[0].offset)); + } + vkd3d_spirv_stream_add_word_count(stream, location, (instruction->src_count - 2) * 2u);
Is the idea here to avoid calling vkd3d_calloc()?