[llvm] [DirectX] Set shader feature flags MinimumPrecision and NativeLowPrecision (PR #139623)

[Deric C. via llvm-commits]

Icohedron wrote:

> I think we need to refactor / clean up the handling of the MinimumPrecision/NativeLowPrecision and LowPrecisionPresent/UseNativeLowPrecision flags. This code is starting to become very hard to follow.
> 
> 1. I'm not 100% convinced we really need all four, and if we do we should probably do some renaming as it's very confusing to have "NativeLowPrecision" and "UseNativeLowPrecision" be so subtly different.
> 2. It's really awkward how setting these is spread out across three different places.
> 
> For (1), it does look like we need MinimumPrecision, NativeLowPrecision, and LowPrecisionPresent. LowPrecisionPresent seems to be set in the container regardless of whether 16 bit types are enabled so I don't think it can be folded in to the other two. UseNativeLowPrecision on the other hand feels redundant to me. The one place that's tricky here is related to a comment in [use-native-low-precision-1.ll](https://github.com/llvm/llvm-project/blob/ad66e5495d598c9f64cfafba92ba48f2a24b8278/llvm/test/CodeGen/DirectX/ShaderFlags/use-native-low-precision-1.ll#L21-L23) - if we set the function flag purely based on the compiler flag but the container flag depends on actual use, then they do need to be separate. Is this done by dxc intentionally or is it a bug? It seems weird to have a flag that says there are 16 bit types when there aren't any present.
> 
> For (2), I think we should move the logic that reads the module metadata out of the loop and precalculate a bool to look it up rather than calculate it every time, and then put all of the logic to set these correctly where we handle LowPrecisionPresent today. IIUC The module flags aren't involved in the function flag calculation, so it should be fine to set those here, and that way we can have one place that has all of this logic and can be commented appropriately to explain what's happening.

Good points. I will refactor the logic for these shader flags to hoist the reading of the dx.nativelowprec module flag out of loops and consolidate the code for setting MinimumPrecision, NativeLowPrecision, and LowPrecisionPresent into where LowPrecisionPresent is currently being set to make the code more readable.

I do agree that having two similar but subtly different flags UseNativeLowPrecision and NativeLowPrecision is rather confusing. Perhaps UseNativeLowPrecision could be renamed to NativeLowPrecisionAvailable, and NativeLowPrecision could be renamed to UsesNativeLowPrecision? The former to indicate availability of native low-precision data types because they have been enabled via a command-line flag / module flag (and the hardware supports it), and the latter to indicate the actual use of native low-precision data types?

Regarding the comment in [use-native-low-precision-1.ll](https://github.com/llvm/llvm-project/blob/ad66e5495d598c9f64cfafba92ba48f2a24b8278/llvm/test/CodeGen/DirectX/ShaderFlags/use-native-low-precision-1.ll#L21-L23) and whether or not it is a bug in DXC: I think it probably doesn't matter if it is a bug in DXC. It would probably be fine to set UseNativeLowPrecision only when there are low-precision data types present in the module.
If a module doesn't use low-precision data types, then whether or not the UseNativeLowPrecision flag is set does not matter -- it shouldn't change the semantics of the shader, and it should still pass the Validator. This would need verification/testing, but if this works then the UseNativeLowPrecision and the NativeLowPrecision could be combined into a single SHADER_FEATURE_INFO entry in DXContainerConstants.def.


https://github.com/llvm/llvm-project/pull/139623