[Feature Request] Make hlsl::IntrinsicOp enum values stable

[tex3d]

Is your feature request related to a problem? Please describe.
The hlsl::IntrinsicOp enum values identify the operation in a high-level intrinsic call in the intermediate IR before lowering to DXIL. Currently, when any operations change in this list, it can impact the value for all operations after the changes in sorted order. Also, the ENABLE_SPIRV_CODEGEN definition adds Vk operations into the middle of the enum definition, which makes the following values depend on this setting.

In the original design, these operations were not intended to be stable between different compiler builds. However, this makes it impossible to write a stable pass test for any pass that starts before dxilgen is complete. We should be testing the dxilgen pass with a pass test, rather than only using end-to-end HLSL to DXIL tests as we have in the past, as well as any other changes we make in passes run before dxilgen.

Describe the solution you'd like
I'd like to add to the files generated at build time (-update-generated-files), a .json file that keeps an assigned value for each HLSL intrinsic operation by name. The automatic parsing and generating of intrinsics from gen_intrin_main.txt would preserve any already-assigned opcodes from the .json, and assign new ones starting just after the highest opcode value currently assigned. When using the option -update-generated-files, the committed .json file would be updated with any new opcodes assigned during this process. Without this option, if the opcodes assignments need to be extended, a build error will be produced, just as it is for other generated files that are part of the project source code. When committing changes that include new opcodes, the .json file changes would be part of that commit, fixing those opcodes to specific values in all future versions.

For the hlsl::IntrinsicOp enum, explicit value assignments will be added so that the existing sorting behavior can be left unchanged. Enum values for the Vk opcodes will be unconditionally defined, even though they may not be used when ENABLE_SPIRV_CODEGEN is undefined. New enum values will continue to appear in sorted order inside the enum definition, but with explicit assignments to new higher opcode values, and the explicit assignments of all existing opcodes would remain unchanged.

This approach preserves the current ordering of opcodes to prevent large changes to the lowering table defined in HLOperationLower.cpp. In the future, all new opcodes would appear at the end of this table. One additional change for the table is that Vk opcodes need to always have table entries, even when ENABLE_SPIRV_CODEGEN is disabled. The entries may have a special lowering function entry that just causes a diagnostic failure from the pass, since HLOperationLower should never encounter a Vk opcode in practice. In addition, this table has always had the requirement that the index of the entry in the table match the hlsl::IntrinsicOp value for that entry, but this has never been enforced explicitly. Additional static (compile-time) checking will be added to guarantee that this requirement is met.

Moving Vk opcodes into a separate group
One thing that could help is to move the Vk opcodes into a separate intrinsic grouping (HLOpcodeGroup). This would prevent these operations from being interspersed with normal HLSL intrinsic operations that must be handled in HLOperationLower, for instance. This has been a kind of backburner desire for quite some time.

An argument could be made that if we do this for Vk intrinsics, we should do the same for upcoming Dx intrinsics (which are intended to be limited to DXIL), but that would introduce yet another HLOpcodeGroup, and require another lowering table. This might be ok, but it's looking like a generalized pattern for defining multiple intrinsic tables driven from gen_intrin_main.txt might need to be designed.

The main downside is the additional infrastructure work to support multiple intrinsic tables generated from gen_intrin_main.txt.

New groups would also require a new generated intrinsic enum for each new group of generated intrinsic operations. New tables defining each grouping of intrinsics would need to be defined and generated. SemaHLSL is already designed to handle multiple tables to a large extent, specifically to support the extension mechanism. For lowering, additional lowering tables could be added relatively easily. However, some code handling intrinsics would need to be extended to check for these new tables. There would need to be multiple lowering tables in HLOperationLower.cpp, to account for the new Dx intrinsics that still need to be lowered there. In SemaHLSL, there is an expectation that there is only one built-in table (see: IsBuiltinTable, HasUnsignedIntrinsicOpcode) in various places. These would need to be revised.

This change is not strictly necessary for the main purpose of this issue: to make HLSL intrinsic opcodes stable. However, doing this work after fixing the HLSL intrinsic opcodes would complicate things, requiring the leaving of gaps and special entries across various tables to accommodate the removed and now reserved opcodes in the main intrinsic enum and tables. Ideally, we either decide we will never make this change, or we make this change before fixing the opcode values according to this issue.

Describe alternatives you've considered

• Using the order of declaration in gen_intrin_main.txt. This has some drawbacks:
  • Intrinsics functions are currently grouped together by similarity, and methods are grouped under object namespaces. An intrinsic of the same name will share the same IOP_ opcode, and a method of the same name will share the same MOP_ opcode. Keeping opcodes stable would require adding new instances of existing namespaces to the end of the file to contain new methods, and new intrinsics at the end of the file, instead of grouping them logically alongside other methods or functions, as is currently done. hctdb.py code may need to be updated to handle duplicate namespace definitions properly in parsing.
  • New overloads with the same name could still be added alongside other overloads. In fact, the order of overloads defined in this file may have an impact on overload resolution behavior in some cases. The unsigned opcode override would complicate the rules further, especially if it differs for an overload. This might lead to inconsistent requirements for new entries, potentially leading to errors.
  • This would make it harder to use this file to see the full set of intrinsics and overloads defined in some context.
  • It would be easy to mistakenly add something in the wrong place, which would cause the original problem to recur.
  • It would dramatically change the ordering of all existing HLSL intrinsic opcodes, forcing a major reordering of the intrinsic table entries in HLOperationLower.cpp. This would be a very large and difficult change to reliably make and review.
• Explicitly assigning opcodes on intrinsic definitions in gen_intrin_main.txt. This has drawbacks:
  • It is a lot of opcodes to manually get right across a lot of intrinsic overloads, including requiring the same opcode to be assigned in many separate places in the file. It needs a way to also define the value for the unsigned_op opcode on the same definition, which may or may not be the same for all overloads with the same normal intrinsic opcode.
  • It's much more difficult and error-prone to manually assign opcode values in this way, even for new operations and overloads added in the future.
  • It would be a large change, difficult to review.
  • It would require extra infrastructure to verify that no mistakes were made in manual assignment (all same-named opcodes have the same value, there are no gaps in values, etc...).
  • It would complicate the intrinsic overload definition with additional internal details that could have been reliably generated instead.

Additional context
Recent additions of dxilgen lowering tests for some SM 6.9 work in progress already created problems depending on ENABLE_SPIRV_CODEGEN, or when adding another hlsl::IntrinsicOp for another part of the work. A temporary mitigation was committed to always build with ENABLE_SPIRV_CODEGEN defined, but that doesn't solve the underlying issue, particularly when new intrinsics will be soon added, requiring manual changes to any existing IR tests for each addition.

Since many more intrinsics and IR tests will be added for SM 6.9 work, this issue needs to be solved as soon as possible to prevent the painful, blocking problems that will inevitably arise otherwise.