-
Notifications
You must be signed in to change notification settings - Fork 183
[CIR][OpenCL] Support lowering of OCL opaque types #2020
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Implement handling for zero-initialization casts to OpenCL opaque types in CIR. This covers cases like `event_t e = async_work_group_copy(..., 0)`. - `VisitCastExpr`: CK_ZeroToOCLOpaqueType now returns a null pointer of the appropriate opaque type instead of `llvm_unreachable`. - `CIRGenTypes::convertType`: Added proper CIR type conversions for OpenCL opaque types including event, queue, and reserve_id types. - Provides consistent CIR representation for OpenCL opaque objects.
bcardosolopes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a testcase
a85d27e to
e808d7f
Compare
I added In addition I commented out some logic as I keep hitting this assert: llvm_unreachable("Requires address space cast which is NYI");@RiverDave claimed it will be fixed in his PR: #1986 Have said this, |
e808d7f to
0eb9550
Compare
seven-mile
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: There's also an issue #802 tracking the design of OpenCL opaque types.
It's not necessary to give a final design for them, but we'd better avoid potential miscompilation here!😉
|
|
||
| // CIR: cir.call @_Z21async_work_group_copyPU3AS3iPU3AS1Kim9ocl_event(%{{.*}}, %{{.*}}, %{{.*}}, %{{.*}}) : (!cir.ptr<!s32i, addrspace(offload_local)>, !cir.ptr<!s32i, addrspace(offload_global)>, !u64i, !cir.ptr<!void>) -> !cir.ptr<!void> | ||
| // LLVM: call spir_func ptr @_Z21async_work_group_copyPU3AS3iPU3AS1Kim9ocl_event(ptr addrspace(3) %{{.*}}, ptr addrspace(1) %{{.*}}, i64 %{{.*}}, ptr null) | ||
| // OG-LLVM: call spir_func target("spirv.Event") @_Z21async_work_group_copyPU3AS3iPU3AS1Kim9ocl_event(ptr addrspace(3) noundef %{{.*}}, ptr addrspace(1) noundef %{{.*}}, i64 noundef %{{.*}}, target("spirv.Event") zeroinitializer No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a difference between ours and the original LLVM (target("spirv.Event") vs ptr addrspace(0)). Is this expected? (I believe the assertion about address space cast you encountered is also a little alarm for us.)
Simply using ptr addrspace(0) here usually works. But the original RFC of LLVM opaque types mentions that it's actually undefined behavior in LLVM IR.
If this ever becomes a huge blocker, I believe it can be implemented using the existing code in the incubator. I'll try to land my PR's from upstream by the end of the week/early next (hopefully). |
|
@mahmood82 can you implement this as if #1986 isn't happening? Whenever @RiverDave works on it he'd have to change it to conform to his work. Even if slightly not ideal
maybe you could do this until it lands? |
|
#1986 is not a blocker here, and it’s not restricting the main feature of this PR (defining the OCL event type and passing zero to When I generate LLVM IR without CIR in the pipeline, my non-SPIRV64 target produces: %call = call ptr @_Z21async_work_group_copyPU3AS3iPU3AS1Kij9ocl_event(ptr addrspace(3) noundef %0, ptr addrspace(1) noundef %1, i32 noundef %2, ptr null) #2Notice that the OpenCL event becomes a plain SPIR-V uses a target-specific path: A robust solution for CIR therefore seems to require preserving the OCL type identity through AST->CIR, and letting the CIR->LLVM lowering eventually call into ResultType = cir::OCLEventType::get(Builder.getContext());(and do the same for the other OpenCL builtin types). I initially attempted to define such a type in Got this CIR result: %7 = "cir.const"() <{value = #cir.ptr<null> : !cir.ptr<!cir.ocl.event, addrspace(offload_private)>}> : () -> !cir.ptr<!cir.ocl.event, addrspace(offload_private)>
%8 = "cir.cast"(%7) <{kind = 1 : i32}> : (!cir.ptr<!cir.ocl.event, addrspace(offload_private)>) -> !cir.ocl.event
%9 = "cir.call"(%3, %4, %6, %8) <{ast = #cir.call.expr.ast, callee = @_Z21async_work_group_copyPU3AS3iPU3AS1Kim9ocl_event, calling_conv = 3 : i32, extra_attrs = #cir<extra({convergent But that fails verification with: I would appreciate guidance on whether this direction is correct, and on how best to complete the addition of the new |
Implement handling for zero-initialization casts to OpenCL opaque types in CIR. This covers cases like
event_t e = async_work_group_copy(..., 0).VisitCastExpr: CK_ZeroToOCLOpaqueType now returns a null pointer of the appropriate opaque type instead ofllvm_unreachable.CIRGenTypes::convertType: Added proper CIR type conversions for OpenCL opaque types including event, queue, and reserve_id types.