https://github.com/andykaylor requested changes to this pull request.
I'm afraid this is an instance where the implementation in the incubator is incorrect. In particular, this doesn't handle the case of deleting an array of objects that require destruction. https://godbolt.org/z/bTsqWzbPG I think this needs to be fixed in the incubator (using the test case I linked above as a reference) before it is upstreamed. If you want to take that on, you'll need to use the behavior in classic codegen as a guide. Because this implementation (when done correctly) involves reading the array cookie, which is target-specific, it would be nice to have a high-level CIR abstraction like `cir.delete.array`, but that will eventually need to incorporate array cookie handling when it is lowered, and it needs to get the name of the delete function from the AST, because `_ZdaPv` (which is hard-coded in the incubator implementation and this PR) isn't always the correct function to call. I would suggest starting with a lower-level implementation (in the incubator) which follows classic codegen more closely and calls the delete function directly. This will be analogous to the way we are currently handling array new. Later, after this is working, we can introduce higher level `cir.array.new` and `cir.array.delete` operations to remove the ABI-specific details. @wizardengineer is starting to look into this kind of upleveling of the CIR implementation, and I think it's important to have a correctly working implementation as the basis of the upleveling work. It seems `cir.delete.array` is an example of an operation where we were too hasty in pushing to the higher level representation and missed some important details in the process. https://github.com/llvm/llvm-project/pull/165225 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
