Skip to content
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

[IRGen] Fix misalignment of conditional invertible requirement counts #73277

Merged
merged 2 commits into from May 1, 2024

Conversation

Azoy
Copy link
Member

@Azoy Azoy commented Apr 26, 2024

The conditional invertible protocol set is alone as a 16 bit slot in the generic context, but items here are expected to be 4 bytes large, so some padding needs to accompany this value. If the number of invertible protocols that we're conditional on is even, we can stick a 0 after all of the requirement counts. Also, we were using countBitsUsed to determine how many requirement counts there were, but this is really testing the first bit in [1, 64] that is turned on instead of actually saying how many bits are turned on.

@Azoy Azoy requested a review from DougGregor April 26, 2024 01:01
@Azoy
Copy link
Member Author

Azoy commented Apr 26, 2024

@swift-ci please smoke test

Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

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

Very nice find

@Azoy
Copy link
Member Author

Azoy commented Apr 26, 2024

@swift-ci please smoke test Windows

1 similar comment
@Azoy
Copy link
Member Author

Azoy commented Apr 26, 2024

@swift-ci please smoke test Windows

@Azoy
Copy link
Member Author

Azoy commented Apr 26, 2024

@swift-ci please smoke test

@Azoy Azoy force-pushed the fix-conditional-invertible branch from 5209398 to 2a2d68f Compare April 26, 2024 17:12
@Azoy
Copy link
Member Author

Azoy commented Apr 26, 2024

@swift-ci please smoke test

@Azoy Azoy force-pushed the fix-conditional-invertible branch from 2a2d68f to 517c05a Compare April 28, 2024 20:38
@Azoy
Copy link
Member Author

Azoy commented Apr 28, 2024

@swift-ci please smoke test

add include

Define a popcount util

Update MathUtils.h
@Azoy Azoy force-pushed the fix-conditional-invertible branch from 517c05a to 852ef0b Compare April 30, 2024 16:16
@Azoy
Copy link
Member Author

Azoy commented Apr 30, 2024

@swift-ci please smoke test

@Azoy Azoy merged commit d380bf9 into apple:main May 1, 2024
3 checks passed
@Azoy Azoy deleted the fix-conditional-invertible branch May 1, 2024 15:10
@hjyamauchi
Copy link
Collaborator

hjyamauchi commented May 1, 2024

@Azoy I haven't precisely reproduced but this PR might have caused this build error on windows ARM64 (it is fine on X86_64):

   Creating library lib\_InternalSwiftStaticMirror.lib and object lib\_InternalSwiftStaticMirror.exp
swiftStaticMirror.lib(ObjectFileContext.cpp.obj) : error LNK2019: unresolved external symbol __popcnt referenced in function "protected: unsigned int __cdecl swift::TrailingGenericContextObjects<struct swift::TargetAnonymousContextDescriptor<struct swift::External<struct swift::NoObjCInterop<struct swift::RuntimeTarget<4> > > >,struct swift::TargetGenericContextDescriptorHeader,struct swift::TargetMangledContextName<struct swift::External<struct swift::NoObjCInterop<struct swift::RuntimeTarget<4> > > > >::getNumConditionalInvertibleProtocolsRequirementCounts(void)const " (?getNumConditionalInvertibleProtocolsRequirementCounts@?$TrailingGenericContextObjects@U?$TargetAnonymousContextDescriptor@U?$External@U?$NoObjCInterop@U?$RuntimeTarget@$03@swift@@@swift@@@swift@@@swift@@UTargetGenericContextDescriptorHeader@2@U?$TargetMangledContextName@U?$External@U?$NoObjCInterop@U?$RuntimeTarget@$03@swift@@@swift@@@swift@@@2@@swift@@IEBAIXZ)

bin\_InternalSwiftStaticMirror.dll : fatal error LNK1120: 1 unresolved externals

It seems like __popcnt isn't available on Windows ARM64 according to https://developercommunity.visualstudio.com/t/-popcnt-popcnt64-intrinsics-not-provided-for-aarch/344160#T-N637429

I think we could use this from llvm rather than rolling our own:

llvm/include/llvm/ADT/bit.h:[[nodiscard]] inline int popcount(T Value) noexcept {

@Azoy
Copy link
Member Author

Azoy commented May 1, 2024

@hjyamauchi that header isn't available to the runtime unfortunately. We can just manually define our own popcount like I did here: #73384

@hjyamauchi
Copy link
Collaborator

@Azoy Alejandro, gotcha. Thanks for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants