-
Notifications
You must be signed in to change notification settings - Fork 115
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
[CIR][CIRGen] Add padding to unions #1289
Conversation
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.
Overall looks good, you could go ahead and remove the lines you commented (and FIXME/TODOs that don't apply anymore).
Seems like even though it's adding the padded bit to regular structs (as updated testcases show), it's not using that information to do anything for them, is that true or did I miss something?
58ad6c6
to
01d6488
Compare
@bcardosolopes updated!
It's true! Btw, maybe it will be good idea to use this |
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, turns out isPadded()
isn't used anywhere and you just removed it, I get it now, thanks! LGTM
This PR adds padding for union type, which is necessary in some cases (e.g. proper offset computation for an element of an array).
The previous discussion is here #1281
The idea is to add a notion about padding in the
StructType
in the same fashion as it's done for packed structures - as a bool argument in the constructor.Now we can compute the proper union type size as a size of the largest element + size of padding type.
There are some downsides though - I had to add this
padded
word in many places. So take a look please!There are many tests fixed and one new -
union-padding