-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[CIR] Add basic support for operator new #145802
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,6 +66,16 @@ struct CIRGenTypeCache { | |
| unsigned char PointerSizeInBytes; | ||
| }; | ||
|
|
||
| /// The size and alignment of size_t. | ||
| union { | ||
| unsigned char SizeSizeInBytes; // sizeof(size_t) | ||
|
||
| unsigned char SizeAlignInBytes; | ||
| }; | ||
|
|
||
| clang::CharUnits getSizeAlign() const { | ||
| return clang::CharUnits::fromQuantity(SizeAlignInBytes); | ||
| } | ||
|
|
||
| clang::CharUnits getPointerAlign() const { | ||
| return clang::CharUnits::fromQuantity(PointerAlignInBytes); | ||
| } | ||
|
|
||
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.
This concerns me, we're setting 1 half of the union, but only adding accessor for the other side?
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.
The union is there to guarantee that both members have the same value, so we could set either one. The fact that I'm only using one in this PR is an artifact of the partial implementation, and the fact that the union is initialized with the one I'm not using is just a matter of reproducing the way it's done in the original code.
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.
Ah, huh... that is undefined behavior...
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.
In general this happens for the other unions as well.
We could get rid of the unions by forcing usage to go through helpers (like getSizeSize(), etc) and the helper itself can use the whatever type it's supposed to match, but this requires cleaning up usages beforehand.
Perhaps this cleanup can be done when
TypeSizeInfoAttris introduced (at which point we perhaps can kill theCIRGenTypeCachealtogether, given it should be cheap to grab these types because MLIR value semantics (which is different from the LLVM type representation)).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.
Perhaps we could also use type aliases for these: llvm/clangir#360
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.
Oh, right. You caught me thinking like a C programmer (very old habit). I'm pretty sure that is the intent of the original code though. This part, at least, I can confidently fix. I agree with @bcardosolopes about the long-term solution, but I will avoid introducing this union for now.