Use opaque pointers #658
Use opaque pointers #658
Conversation
alexbaden
force-pushed
the
alex/opaque_pointers_2
branch
3 times, most recently
from
464f9ff to
eb49dde
Compare
alexbaden
force-pushed
the
alex/opaque_pointers_2
branch
2 times, most recently
from
02e9ce0 to
5f3d32c
Compare
alexbaden
force-pushed
the
alex/opaque_pointers_2
branch
from
5f3d32c to
1e0f481
Compare
This reverts commit 28d5e77.
alexbaden
force-pushed
the
alex/opaque_pointers_2
branch
from
1e0f481 to
5bead52
Compare
alexbaden
force-pushed
the
alex/opaque_pointers_2
branch
from
88038b4 to
e7a4359
Compare
alexbaden
force-pushed
the
alex/opaque_pointers_2
branch
2 times, most recently
from
d54be73 to
96e679a
Compare
|
Looks like there is an issue with the PVC runner separate from this PR - otherwise, all CI checks are passing reliably and this is ready for review. I ended up moving to LLVM 16 for SPIRV support for L0 - in LLVM 15, opaque pointers were not working in the Spirv translator. This also updates all conda environments and the docker containers to LLVM 16. |
| @@ -647,7 +643,8 @@ void CodeGenerator::codegenBufferArgs(const std::string& ext_func_name, | |||
| cgen_state_->ir_builder_.CreateStructGEP(buffer_abstraction, alloc_mem, 2); | |||
| cgen_state_->ir_builder_.CreateStore(buffer_null_extended, buffer_is_null_ptr); | |||
| } | |||
| output_args.push_back(alloc_mem); | |||
| output_args.push_back( | |||
| cgen_state_->ir_builder_.CreateLoad(buffer_abstraction, alloc_mem)); | |||
There was a problem hiding this comment.
Why do you replace a pointer to the allocated structure with an actual structure value here?
There was a problem hiding this comment.
I think it might be a bug. It adds this seemingly useless load:
%14 = load { ptr, i64, i8 }, ptr %buffer_arg_ptr, align 8
%15 = alloca { ptr, i64, i8 }, align 8
call void @array_append__4(ptr %15, ptr %buffer_arg_ptr, float %5)
The load is never used. It disappears in the optimized IR, too. But without it, the array_append function doesn't return properly. The operation is sorta hacked anyway, the struct return can be optimized in different ways depending on the size which is why I opted for an int64 in the middle. I wonder if the opaque ptr type vs the previous 64-bit pointer is the problem? At least I think it was 64-bits. Anyway, adding the load seemed to resolve the problem.
There was a problem hiding this comment.
OK, just wanted to make sure this was intentional
There was a problem hiding this comment.
Yeah, it's covered pretty well by the tests in ArrayTest. When I wrote this I knew I was hacking the ABI, and so I made sure there was good test coverage. :)
This PR enables opaque pointers under LLVM 15. It removes calls to
getPointerElementType()and replaces them with either explicit types or untyped (opaque) pointers. Unfortunately, in some instances the type info is hard coded in a different location from the pointer allocation, or from where it is used. This means if we ever change a type, we might get subtle bugs. We should look at adding type info to our frontend or otherwise refactoring in areas where this could occur.As part of this work I pushed the address space bitcasts into allocation where possible. The opaque pointer static getter includes address space as a required variable, so this should be acceptable.
I have a few more
getPointerElementType()calls but they don't seem to be surfaced in the tests, so I am working on how best to handle those. For now, this is a draft.