[SYCL] Update spec constant handling for struct hierarchies #17204
Conversation
| const Module &M = *InsertBefore->getModule(); | ||
| auto DL = M.getDataLayout(); |
There was a problem hiding this comment.
It's just a nit, so feel free to ignore, but I'd recommend to move this closer to its uses.
|
@AlexeySachkov you might want to have a look at this one. |
|
Hi @jzc |
|
@asudarsa Hi, I have updated the description now |
There was a problem hiding this comment.
@intel/llvm-gatekeepers Code formatting is failing due to undef deprecator failing. I documented the uses of undef in the PR review, and plan on addressing removing the uses of undef in the separate PR #17341, as the changes are mostly unrelated and requires changes unrelated tests. Can this PR still proceed with a merge?
| %struct.foo.base = type <{ i32, [4 x i8], [5 x i64], [5 x %struct.anon.0], [5 x i8] }> | ||
|
|
||
| @__usid_str = private unnamed_addr constant [44 x i8] c"uid52dfb70f8b72bae7____ZL16scary_spec_const\00", align 1 | ||
| @_ZL16scary_spec_const = internal addrspace(1) constant { { float, i8, i32, %struct.anon, [4 x i8], i32, [5 x i64], [5 x %struct.anon.0], [5 x i8], [15 x i8] } } { { float, i8, i32, %struct.anon, [4 x i8], i32, [5 x i64], [5 x %struct.anon.0], [5 x i8], [15 x i8] } { float 0.000000e+00, i8 98, i32 0, %struct.anon zeroinitializer, [4 x i8] undef, i32 3, [5 x i64] [i64 5, i64 0, i64 0, i64 0, i64 0], [5 x %struct.anon.0] [%struct.anon.0 { i32 1 }, %struct.anon.0 { i32 2 }, %struct.anon.0 zeroinitializer, %struct.anon.0 zeroinitializer, %struct.anon.0 zeroinitializer], [5 x i8] c"abc\00\00", [15 x i8] undef } }, align 16 |
There was a problem hiding this comment.
These undef uses are generated by clang, I do not plan on removing them because I want to test that the SpecConstantsPass can handle what clang generates.
There was a problem hiding this comment.
You should not use LLVM IR test format for checking "what clang generates". Clang will generate something different in the future, which this test won't cover. You should use SYCL end-to-end tests to validate end-to-end test behavior.
Please, follow the LLVM community guidelines for the test written in LLVM IR.
| ; CHECK: %[[#SCV30:]] = {{.*}}@{{.*}}SpecConstant{{.*}}(i32 21, i8 0) | ||
| ; CHECK: %[[#SCV31:]] = {{.*}}@{{.*}}SpecConstant{{.*}}(i32 22, i8 0) | ||
| ; CHECK: %[[#SCV32:]] = {{.*}}@{{.*}}SpecConstantComposite{{.*}}(i8 %[[#SCV27]], i8 %[[#SCV28]], i8 %[[#SCV29]], i8 %[[#SCV30]], i8 %[[#SCV31]]) | ||
| ; CHECK: %[[#SCV33:]] = {{.*}}@{{.*}}SpecConstantComposite{{.*}}(i32 %[[#SCV9]], [4 x i8] undef, [5 x i64] %[[#SCV15]], [5 x %struct.anon.0] %[[#SCV26]], [5 x i8] %[[#SCV32]]) |
There was a problem hiding this comment.
These undef uses are generated by SpecConstantsPass for handling padding fields. The plan is to generate zero instead of undef for padding fields in #17341.
| if (isa<UndefValue>(ElemDefaultValue)) | ||
| HandleUndef(ElemDefaultValue); | ||
| if (It == DefinedElements.end() || It->first != ElOffset) | ||
| HandleUndef(UndefValue::get(ElTy)); |
There was a problem hiding this comment.
Will be changed to Constant::getNullValue(ElTy) in #17341
IMO, yes, we can proceed with the merge. We saw same |
I don't think we should ignore them. We should try to address them. |
After #16976, CTS failed to compile due to an assertion being raised in
sycl-post-link, more specifically while processing avecspec constants atllvm/llvm/lib/SYCLLowerIR/SpecConstants.cpp
Line 593 in 35fba19
However, the problem was more general and could be attributed to struct hierarchies.
getElemDefaultValuewas essentially assuming the return type for the spec constant instruction and the type used to initialize the default value for the spec constant had the same "structure" (modulo some padding fields). However, in general, this assumption does not seem to hold. The initializer will essentially have a flattened out structure, while the return type will retain the nested structure.Because of this
emitSpecConstantRecursiveImplhas been updated to consider this. First, before recursion starts, we recurse the default value initializer incollectDefinedElements, collecting information on the children elements of the structure, noting what offset they exists at. Then, we recurse the return type inemitSpecConstantRecursiveImpl. When we reach a child element, we default value by using the information we collected earlier. There is some consideration needed for padding - before recursing, we check to see if any child element exists at the offset we are trying to recurse in. If there is none, then we must be at some padding fields.