[SYCL] Handle address space casts in the LowerWGScope #3120
Conversation
|
LGTM, however, please address CI failures. |
| @@ -0,0 +1,83 @@ | |||
| ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py | |||
| ; RUN: opt < %s -LowerWGScope -S | FileCheck %s | |||
|
|
|||
There was a problem hiding this comment.
Please add a comment what this test checks.
| ; CHECK-NEXT: [[TMP5:%.*]] = addrspacecast %struct.bar* [[ARG1]] to [[STRUCT_BAR:%.*]] addrspace(4)* | ||
| ; CHECK-NEXT: [[TMP6:%.*]] = addrspacecast [[STRUCT_SPAM]] addrspace(4)* [[TMP4]] to %struct.spam* | ||
| ; CHECK-NEXT: call spir_func void @widget(%struct.bar addrspace(4)* dereferenceable_or_null(32) [[TMP5]], %struct.spam* byval(%struct.spam) align 8 [[TMP6]]) | ||
| ; CHECK-NEXT: ret void |
There was a problem hiding this comment.
Nit:
Can the number of checks be reduced to addrspace cast dependence chain only?
There was a problem hiding this comment.
I think it's important to validate all the code generated by the pass to ensure correctness.
These checks should be easy to maintain as they are auto-generated (see the note at line 1).
There was a problem hiding this comment.
I agree with Alexey here.
There was a problem hiding this comment.
I think it's important to validate all the code generated by the pass to ensure correctness.
I think regression tests are written for particular fixes and should be focused on testing the fix, not entire pass/feature, as there should exist other tests. But anyway, that was a nit, so either way is fine with me.
The script for automated maintenance is a good argument too.
I know, but I'd like you confirm that the fix is correct. :) UPDATE: I just noticed that checks in the test are auto-generated, so the right way to fix it is to run the script again. I made my changes manually. |
| auto *Cast = dyn_cast<AddrSpaceCastInst>(LambdaObj); | ||
| if (Cast) |
There was a problem hiding this comment.
From https://github.com/intel/llvm/pull/2864/files#r567811589:
Combine into if (auto *Cast = dyn_cast<AddrSpaceCastInst>(LambdaObj))
Thanks, fixed. |
| ; CHECK-NEXT: [[TMP5:%.*]] = addrspacecast %struct.bar* [[ARG1]] to [[STRUCT_BAR:%.*]] addrspace(4)* | ||
| ; CHECK-NEXT: [[TMP6:%.*]] = addrspacecast [[STRUCT_SPAM]] addrspace(4)* [[TMP4]] to %struct.spam* | ||
| ; CHECK-NEXT: call spir_func void @widget(%struct.bar addrspace(4)* dereferenceable_or_null(32) [[TMP5]], %struct.spam* byval(%struct.spam) align 8 [[TMP6]]) | ||
| ; CHECK-NEXT: ret void |
There was a problem hiding this comment.
I think it's important to validate all the code generated by the pass to ensure correctness.
I think regression tests are written for particular fixes and should be focused on testing the fix, not entire pass/feature, as there should exist other tests. But anyway, that was a nit, so either way is fine with me.
The script for automated maintenance is a good argument too.
No description provided.