Fix attribute collision for HL intrinsics #5451
Conversation
HL Intrinsic functions share declarations with those that match group and function signature, regardless of the original intrinsic name. This means that intrinsics with differing attributes can be collapsed into the same HL functions, leading to incorrect attributes for some HL intrinsics. This fixes this issue by adding the attributes to the HL operation mangling, the same way this issue was fixed for the HLWaveSensitive attribute before.
lib/HLSL/HLOperations.cpp
Outdated
| Attribute::ReadOnly)) | ||
| mangledNameStr << "ro"; | ||
| if (attribs.hasAttribute(AttributeSet::FunctionIndex, HLWaveSensitive)) | ||
| mangledNameStr << "wave"; |
There was a problem hiding this comment.
What about other attributes? I see we ignore nounwind based on the tests, but maybe we should do that explicitly and warn or error or something if we see something odd, otherwise this is just going to pop up again.
There was a problem hiding this comment.
The only relevant attributes for this issue are those defined in gen_intrin_main
There was a problem hiding this comment.
nounwind is always added to these intrinsic functions, so isn't presumed to be a dimension for overloading. Since these are just for mangling the built-in intrinsics, there is an assumption that the attributes used are limited to a certain set.
However, I do see that we have special cases where we add attributes under SetHLFunctionAttribute(), including potentially NoDuplicate. That isn't accounted for in this code.
Ideally, one place would define the correct attributes on the original functions, and the mangling would follow, but this code isn't written to be able to do that, since special cases for attributes are applied after the mangling was defined, and the function looked up. I think combining the attribute adding and attribute mangling code into the same function would prevent them from getting out of sync so easily, at least.
Some of these attributes are definable in the intrinsic table input generated from gen_intrin_main.txt (ro/rn/wave), and I think they all should be (at least the ones that could be different). There also should be use of ArgMemOnly in SM 6.8 branch, and another old PR I had up here changed the way the attributes are defined in the intrinsic tables to allow for easier expansion of attributes as needed. But some of these functions don't come from the intrinsic tables, and are generated elsewhere as built-in operators or intermediate operations, and some of those are special-cased in SetHLFunctionAttribute().
lib/HLSL/HLOperations.cpp
Outdated
| // Need to add attributes to name to prevent clashes with intrinsics with | ||
| // different attributes | ||
| if (attribs.hasAttribute(AttributeSet::FunctionIndex, Attribute::ReadNone)) | ||
| mangledNameStr << "rn"; |
There was a problem hiding this comment.
I'm surprised there are no test updates with rn - do we not have any ops that have this attribute or do we just not have any tests for them? Similarly, it'd be nice to have tests for cases with multiple attributes if they exist.
There was a problem hiding this comment.
The issue here is mistakenly temporarily treating HL intrinsics that have the same signature as the same intrinsic, which causes attributes as defined in gen_intrin_main to bleed into those that match. The only attributes there are ro, rn, and wv. There are none that have more than one.
Attributes set/transferred in one place before mangling performed. Attribute mangling added when necessary in GetHLFullName(). Mangling checks that all function attributes are handled to catch any not captured into mangling. GetOrCreateHLFunction checks that function type and attributes match when function exists. Adjusted buf_index fcgl test for name mangling change. Checks added for ReadNone and ReadOnly attributes used together to catch bugs. Issue found with lowerHLMatSubscript, where subscript attributes passed to create the load - subscript is ReadNone, load is ReadOnly. FIXME: Found potential bug in HLMatrixLowerPass::lowerHLMatSubscript, where the load may need to added at the pointer users, instead of at the subscript.
tools/clang/test/HLSLFileCheck/hlsl/objects/RayQuery/readnone_intrinsicAttributeCollision.hlsl
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,46 @@ | |||
| // RUN: %dxc -T ps_6_5 -E PS -fcgl %s | FileCheck %s | |||
| // RUN: %dxc -T ps_6_5 -E PS %s | FileCheck %s -check-prefix=CHECKDXIL | |||
There was a problem hiding this comment.
It seems we allow QuadAny with shader models earlier than 6.7 😣
We shouldn't though. One of us should file a bug for that and this test should be updated to require 6.7
There was a problem hiding this comment.
Don't we have a fallback for QuadAny on shader models earlier than 6.7?
There was a problem hiding this comment.
Made an issue here:
#5521
HL Intrinsic functions share declarations with those that match group and function signature, regardless of the original intrinsic name. This means that intrinsics with differing attributes can be collapsed into the same HL functions, leading to incorrect attributes for some HL intrinsics. This fixes this issue by adding the attributes to the HL operation mangling, the same way this issue was fixed for the HLWaveSensitive attribute before. Fixes microsoft#3505 --------- Co-authored-by: Joshua Batista <jbatista@microsoft.com>
HL Intrinsic functions share declarations with those that match group and function signature, regardless of the original intrinsic name. This means that intrinsics with differing attributes can be collapsed into the same HL functions, leading to incorrect attributes for some HL intrinsics. This fixes this issue by adding the attributes to the HL operation mangling, the same way this issue was fixed for the HLWaveSensitive attribute before. Fixes microsoft#3505 --------- Co-authored-by: Joshua Batista <jbatista@microsoft.com> (cherry picked from commit d9c07e9)
HL Intrinsic functions share declarations with those that match group and function signature, regardless of the original intrinsic name. This means that intrinsics with differing attributes can be collapsed into the same HL functions, leading to incorrect attributes for some HL intrinsics. This fixes this issue by adding the attributes to the HL operation mangling, the same way this issue was fixed for the HLWaveSensitive attribute before. Fixes #3505 --------- Co-authored-by: Joshua Batista <jbatista@microsoft.com> (cherry picked from commit d9c07e9) Co-authored-by: Tex Riddell <texr@microsoft.com>
The argmemonly attrib caused an assert because it wasn't handled by the mangling code. This doesn't include mangling for it yet as there are no intrinsics that use it yet, but it prevents the assert. Followon to microsoft#5451
This PR aims to add some functionality to hctdb_instrhelp.py to efficiently search and find specific instructions. For example, in #5451, a parser needed to be written to parse through gen_intrin_main.txt to search for any set of instructions that: 1. had at least one function attribute that was "readnone" 2. All had the same return type 3. All had the same argument list (types and length) This search could've been made much easier if hctdb_instrhelp.py had a method to search through instructions in a database-like manner. This PR allows the user to define a query function with which to filter out dxil instructions in hctdb.py. The instructions can be filtered out based on function attributes, return type, function name, and the function argument list. It should allow the user to quickly find instructions that satisfy any arbitrary requirements. On the command line, the user may run: `python3 hctdb_instrhelp.py --query-dxil=<filename-no-extension>` This command will instruct hctdb_instrhelp.py to run a query using the inst_query_dxil function defined by the filename argument in the command line. Alternatively, for hl operations, one can instead use the --query-hlsl argument, and define a function called inst_query_hlsl in that file instead. And then the query functionality of the script will be run, which applies the user-defined query function to all instructions and outputs the query results. Fixes #5556
HL Intrinsic functions share declarations with those that match group and function signature, regardless of the original intrinsic name. This means that intrinsics with differing attributes can be collapsed into the same HL functions, leading to incorrect attributes for some HL intrinsics.
This fixes this issue by adding the attributes to the HL operation mangling, the same way this issue was fixed for the HLWaveSensitive attribute before.
Fixes #3505