Skip to content

Fix storeEnumTagSinglePayload on big-endian systems #22208

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 21, 2019
Merged

Fix storeEnumTagSinglePayload on big-endian systems #22208

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ab8312
Fix storeEnumTagSinglePayload on big-endian systems
mundaym Jan 29, 2019
43bfbd5
Use llvm::Triple to test target endianness
mundaym Jan 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 27 additions & 27 deletions lib/IRGen/EnumPayload.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,22 +162,22 @@ void EnumPayload::insertValue(IRGenFunction &IGF, llvm::Value *value,
subvalue = IGF.Builder.CreateLShr(subvalue,
llvm::ConstantInt::get(valueIntTy, valueOffset));
subvalue = IGF.Builder.CreateZExtOrTrunc(subvalue, payloadIntTy);
#if defined(__BIG_ENDIAN__)
if ((valueBitWidth == 32 || valueBitWidth == 16 || valueBitWidth == 8 || valueBitWidth == 1) &&
payloadBitWidth > (payloadValueOffset + valueBitWidth)) {
unsigned shiftBitWidth = valueBitWidth;
if (valueBitWidth == 1) {
shiftBitWidth = 8;
if (IGF.IGM.Triple.isLittleEndian()) {
if (payloadValueOffset > 0)
subvalue = IGF.Builder.CreateShl(subvalue,
llvm::ConstantInt::get(payloadIntTy, payloadValueOffset));
} else {
if ((valueBitWidth == 32 || valueBitWidth == 16 || valueBitWidth == 8 || valueBitWidth == 1) &&
payloadBitWidth > (payloadValueOffset + valueBitWidth)) {
unsigned shiftBitWidth = valueBitWidth;
if (valueBitWidth == 1) {
shiftBitWidth = 8;
}
subvalue = IGF.Builder.CreateShl(subvalue,
llvm::ConstantInt::get(payloadIntTy, (payloadBitWidth - shiftBitWidth) - payloadValueOffset));
}
subvalue = IGF.Builder.CreateShl(subvalue,
llvm::ConstantInt::get(payloadIntTy, (payloadBitWidth - shiftBitWidth) - payloadValueOffset));
}
#else
if (payloadValueOffset > 0)
subvalue = IGF.Builder.CreateShl(subvalue,
llvm::ConstantInt::get(payloadIntTy, payloadValueOffset));
#endif


// If there hasn't yet been a value stored here, we can use the adjusted
// value directly.
if (payloadValue.is<llvm::Type *>()) {
Expand Down Expand Up @@ -230,21 +230,21 @@ llvm::Value *EnumPayload::extractValue(IRGenFunction &IGF, llvm::Type *type,
llvm::IntegerType::get(IGF.IGM.getLLVMContext(), payloadBitWidth);

value = IGF.Builder.CreateBitOrPointerCast(value, payloadIntTy);
#if defined(__BIG_ENDIAN__)
if ((valueBitWidth == 32 || valueBitWidth == 16 || valueBitWidth == 8 || valueBitWidth == 1) &&
payloadBitWidth > (payloadValueOffset + valueBitWidth)) {
unsigned shiftBitWidth = valueBitWidth;
if (valueBitWidth == 1) {
shiftBitWidth = 8;
if (IGF.IGM.Triple.isLittleEndian()) {
if (payloadValueOffset > 0)
value = IGF.Builder.CreateLShr(value,
llvm::ConstantInt::get(value->getType(), payloadValueOffset));
} else {
if ((valueBitWidth == 32 || valueBitWidth == 16 || valueBitWidth == 8 || valueBitWidth == 1) &&
payloadBitWidth > (payloadValueOffset + valueBitWidth)) {
unsigned shiftBitWidth = valueBitWidth;
if (valueBitWidth == 1) {
shiftBitWidth = 8;
}
value = IGF.Builder.CreateLShr(value,
llvm::ConstantInt::get(value->getType(), (payloadBitWidth - shiftBitWidth) - payloadValueOffset));
}
value = IGF.Builder.CreateLShr(value,
llvm::ConstantInt::get(value->getType(), (payloadBitWidth - shiftBitWidth) - payloadValueOffset));
}
#else
if (payloadValueOffset > 0)
value = IGF.Builder.CreateLShr(value,
llvm::ConstantInt::get(value->getType(), payloadValueOffset));
#endif
if (valueBitWidth > payloadBitWidth)
value = IGF.Builder.CreateZExt(value, valueIntTy);
if (valueOffset > 0)
Expand Down
12 changes: 6 additions & 6 deletions lib/IRGen/GenEnum.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4202,12 +4202,12 @@ namespace {
} else if (!CommonSpareBits.empty()) {
// Otherwise the payload is just the index.
payload = EnumPayload::zero(IGM, PayloadSchema);
#if defined(__BIG_ENDIAN__) && defined(__LP64__)
// Code produced above are of type IGM.Int32Ty
// However, payload is IGM.SizeTy in 64bit
if (numCaseBits >= 64)
tagIndex = IGF.Builder.CreateZExt(tagIndex, IGM.SizeTy);
#endif
if (!IGF.IGM.Triple.isLittleEndian()) {
if (IGF.IGM.Triple.isArch64Bit() && numCaseBits >= 64) {
// Need to set the full 64-bit chunk on big-endian systems.
tagIndex = IGF.Builder.CreateZExt(tagIndex, IGM.SizeTy);
}
}
// We know we won't use more than numCaseBits from tagIndex's value:
// We made sure of this in the logic above.
payload.insertValue(IGF, tagIndex, 0,
Expand Down
16 changes: 8 additions & 8 deletions stdlib/public/runtime/EnumImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,14 +170,14 @@ inline void storeEnumTagSinglePayloadImpl(

// Store into the value.
#if defined(__BIG_ENDIAN__)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This really needs to be key'ed off of the triple. If you are fixing the big endian codepath, please fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I've added another commit to make those changes to IRGen.

unsigned numPayloadTagBytes = std::min(size_t(4), payloadSize);
if (numPayloadTagBytes)
small_memcpy(valueAddr,
reinterpret_cast<uint8_t *>(&payloadIndex) + 4 -
numPayloadTagBytes,
numPayloadTagBytes, true);
if (payloadSize > 4)
memset(valueAddr + 4, 0, payloadSize - 4);
uint64_t payloadIndexBuf = static_cast<uint64_t>(payloadIndex);
if (payloadSize >= 8) {
memcpy(valueAddr, &payloadIndexBuf, 8);
memset(valueAddr + 8, 0, payloadSize - 8);
} else if (payloadSize > 0) {
payloadIndexBuf <<= (8 - payloadSize) * 8;
memcpy(valueAddr, &payloadIndexBuf, payloadSize);
}
if (numExtraTagBytes)
small_memcpy(extraTagBitAddr,
reinterpret_cast<uint8_t *>(&extraTagIndex) + 4 -
Expand Down