[CIR] Add limited support for array new #1286
Conversation
This change adds initial support for array new expressions where the array size is constant. This includes implementing the array cookie generation in the CIR CXXABI code, but only for the base Itanium ABI. A subsequent patch will create a subclass of CIRGenItaniumCXXABI to provide the proper array cookie support for AppleARM64 targets. This patch does not support variable array sizes or asan handling, both of which are marked as "NYI".
|
I realize that the test cases here don't actually require an allocation cookie. I'm working on that. |
There was a problem hiding this comment.
Thanks for working on this Andy. If the testcases don't exercise the cookie logic, perhaps this PR should look at the version without the cookie and introduce the cookie in the next PR. If you could make this more self contained and incrementally improve the cookie support that would be best (PR is a bit big right now).
Would you prefer to see an initial implementation that only handles the cases where no cookie is needed? That would be a much smaller patch. |
|
To clarify, the test case I first uploaded didn't exercise the cookie logic, but I've since added test cases that do. I could definitely cut that out for now and bring it back in a follow-up PR. |
|
Sorry for the noise. I think I've got this down to the minimum set of changes to implement array new with a constant size now. However, after looking at #1172 I have questions about the general direction. Should we introduce cir.array.new instead of the literal CIR generation I'm doing here? Having not been involved in any of the discussion that led to #1172 I can't tell exactly what the plan was to handle cookies there. A comment indicates that the cookies will be handled in LoweringPreparePass, but with no further details. @ChuanqiXu9 did you have a direction in mind for array new? |
My personal opinion is, it is better to handle cookies in later passes (e.g., LoweringPrepareItaniumCXXABI.cpp or LoweringPrepareX86CXXABI.cpp or LoweringPrepareAArch64CXXABI.cpp). The theory is, in this manner, we hide the details in the just generated CIR. Then it is better for the analysis to work. |
That would be the best! |
|
@andykaylor I agree with @ChuanqiXu9, but I suggest you the given steps: (a) Land the first PR without cookies My rationale from introducing (b) before (c) is for you to create all testcases, understand all that you need to get down to LLVM, get more familiar with CIRGen and defer new operation discussions to (c). |
|
@bcardosolopes I like that plan. I've been exploring (c) and I definitely agree that having done (b) first will be very useful in terms of establishing a road map. I noticed a couple more things from my original commit that can be removed ( |
andykaylor
changed the title
|
@bcardosolopes I think this is ready for your review now. It's doing the minimum necessary to handle only fixed size array new with basic types, and all new code is exercised by the added tests. |
There was a problem hiding this comment.
This looks great, thanks for putting the extra effort to make this smaller. LGTM with a minor nit: can you please add checks for the LLVM output as well?
We try to match the OG LLVM output as much as possible, so for features like this we'd like to stamp it so it doesn't regress (or if it's different, we want to create an issue to track the difference).
This change adds initial support for array new expressions where the array size is constant. This includes implementing the array cookie generation in the CIR CXXABI code, but only for the base Itanium ABI. A subsequent patch will create a subclass of CIRGenItaniumCXXABI to provide the proper array cookie support for AppleARM64 targets.
This patch does not support variable array sizes or special asan handling, both of which are marked as "NYI".