Fix storeEnumTagSinglePayload on big-endian systems #22208
Conversation
The part of the tag stored in the payload can currently be up to 8 bytes in size (though only the 'low' 4 bytes can be non-zero). On little-endian machines this doesn't matter, we can always just store up to 4 bytes and zero the remaining payload bytes. On big- endian systems however we may need to store more than 4 bytes. The store implementation now mirrors the runtime code that fetches the tag on big-endian systems which already treats the payload tag as an 8 byte integer. This is a spot fix but longer term we might want to consider refactoring this code to reduce the number of differences between big- and little-endian implementations. For example, we could centralise some of the copying logic and/or make the payload tag a 4 byte field on all platforms.
| @@ -170,14 +170,14 @@ inline void storeEnumTagSinglePayloadImpl( | |||
|
|
|||
| // Store into the value. | |||
| #if defined(__BIG_ENDIAN__) | |||
There was a problem hiding this comment.
This really needs to be key'ed off of the triple. If you are fixing the big endian codepath, please fix that.
There was a problem hiding this comment.
Isn't the triple for codegen rather than the stdlib/runtime (which is always compiled for the target architecture)?
If we do want to change how we select which code to use we should do that in a separate fix in my opinion.
There was a problem hiding this comment.
This code has remained this way for some time now, and I think that it has come up a couple of times, that IRGen needs to be able to test this on little endian and big endian.
There was a problem hiding this comment.
I'm sorry, I still don't understand what you want to change in this PR?
We should definitely change IRGen to use triples, but that is a completely separate issue from this PR AFAIK.
There was a problem hiding this comment.
Yes, I agree that it is a different issue. However, since you are already changing the code around it, I would like that you take care of that, especially since this is not intended for the 5.0 branch.
There was a problem hiding this comment.
OK I've added another commit to make those changes to IRGen.
The current checks won't work when cross compiling from a machine with a different endianness to the target.
mundaym
force-pushed
the
s390x-storetag-fix
branch
from
79bf1a0 to
43bfbd5
Compare
|
Ping. I've made the suggested changes. |
|
Ping. |
1 similar comment
|
Ping. |
|
@swift-ci Please test |
|
@compnerd It looks like your review comments have been addressed, and the tests pass; any concerns before I merge? |
The part of the tag stored in the payload can currently be up to
8 bytes in size (though only the 'low' 4 bytes can be non-zero).
On little-endian machines this doesn't matter, we can always just
store up to 4 bytes and zero the remaining payload bytes. On big-
endian systems however we may need to store more than 4 bytes.
The store implementation now mirrors the runtime code that fetches
the tag on big-endian systems which already treats the payload tag
as an 8 byte integer.
This is a spot fix but longer term we might want to consider
refactoring this code to reduce the number of differences between
big- and little-endian implementations. For example, we could
centralise some of the copying logic and/or make the payload tag
a 4 byte field on all platforms.