Skip to content

NFC: [CodeCompletion] Use enums in bit fields as-is #40163

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

Jump to bottom
Merged
merged 4 commits into from
Dec 21, 2021
Merged

NFC: [CodeCompletion] Use enums in bit fields as-is #40163

merged 4 commits into from
Dec 21, 2021

Conversation

rintaro
Copy link
Member

@rintaro rintaro commented Nov 12, 2021

We don't need to convert them from/to integer

@rintaro
Copy link
Member Author

rintaro commented Nov 12, 2021

@swift-ci Please smoke test

@rintaro rintaro changed the title [CodeCompletion] Use enums in bit fields as-is NFC: [CodeCompletion] Use enums in bit fields as-is Nov 12, 2021
@rintaro rintaro requested review from bnbarham and ahoppen November 12, 2021 22:36
ahoppen
ahoppen reviewed Nov 16, 2021
View reviewed changes
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Wow, I can’t believe that we didn’t do this before. LGTM, I think it might be worth also static_asserting that the values actually fit inside the specified number of bits.

include/swift/IDE/CodeCompletion.h
CodeCompletionOperatorKind KnownOperatorKind : 6;
SemanticContextKind SemanticContext : 3;
unsigned char Flair : 8;
NotRecommendedReason NotRecommended : 4;
Copy link
Member

Choose a reason for hiding this comment

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

Do we have static_asserts that e.g. NotRecommendedReason doesn’t contain more then 2^4 elements?

Copy link
Member Author

@rintaro rintaro Nov 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

No. Good idea. I added a commit.

@rintaro
Copy link
Member Author

rintaro commented Nov 16, 2021

@swift-ci Please smoke test

bnbarham
bnbarham approved these changes Nov 16, 2021
View reviewed changes
Copy link
Contributor

@bnbarham bnbarham left a comment

Choose a reason for hiding this comment

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

Much nicer! Note that the build is failing on:

SwiftEditor.cpp(611,1): error: static_assert failed due to requirement 'sizeof((anonymous namespace)::SwiftSemanticToken) == 8' "Too big"
static_assert(sizeof(SwiftSemanticToken) == 8, "Too big");

ahoppen
ahoppen reviewed Nov 17, 2021
View reviewed changes
include/swift/IDE/CodeCompletion.h
SemanticContextKind SemanticContext : 3;
unsigned char Flair : 8;
NotRecommendedReason NotRecommended : 4;
bool IsSystem : 1;
Copy link
Member

Choose a reason for hiding this comment

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

Not really related to this PR but in libSyntax’s RawSyntax, I saw measurable performance improvements when not compressing bitfields into the minimum number of bits but having them byte-aligned. I think this was because the CPU doesn’t need to do bitshifts/masks to retrieve the value.

The bits inside RawSyntax were probably accessed a lot more frequently than these, but it might be worth investigating if byte-aligning these might also be beneficial performance-wise, even though it increases the memory footprint.

@ahoppen ahoppen mentioned this pull request Nov 22, 2021
Closed
@ahoppen ahoppen mentioned this pull request Dec 3, 2021
Closed
@rintaro rintaro force-pushed the ide-completion-enumbitfield branch from e1f2d7a to bc57eaf Compare December 4, 2021 00:00
@rintaro
Copy link
Member Author

rintaro commented Dec 4, 2021

@swift-ci Please smoke test

@rintaro rintaro force-pushed the ide-completion-enumbitfield branch from bc57eaf to 4c39c90 Compare December 4, 2021 07:12
@rintaro
Copy link
Member Author

rintaro commented Dec 4, 2021

@swift-ci Please smoke test

@rintaro
Guard a static assert for MSVC
f17671c 
MSVC doens't pack diffrent underlying int types into a bitfield. e.g.

  struct S {
    int a: 1;
    char b: 1;
    int c: 1;
  };

These fields are considered three sparate bitfields.
@rintaro rintaro force-pushed the ide-completion-enumbitfield branch from 4c39c90 to f17671c Compare December 4, 2021 08:19
@rintaro
Copy link
Member Author

rintaro commented Dec 4, 2021

@swift-ci Please smoke test

@ahoppen ahoppen mentioned this pull request Dec 8, 2021
Merged
@ahoppen
Copy link
Member

ahoppen commented Dec 20, 2021

@swift-ci Please smoke test Windows

@ahoppen
Copy link
Member

ahoppen commented Dec 21, 2021

@swift-ci Please smoke test Windows

@ahoppen
Copy link
Member

ahoppen commented Dec 21, 2021

@swift-ci Please smoke test

@ahoppen
Copy link
Member

ahoppen commented Dec 21, 2021

I got this to pass by changing

- #if !SWIFT_COMPILER_IS_MSVC
+ #if !defined(_MSC_VER)

AFAICT SWIFT_COMPILER_IS_MSVC is only set if the sources are compiled by the visual studio compiler. https://github.com/apple/swift/blob/b42402bd6f1811a2ab43f58082b2c635a599b4da/include/swift/Basic/Compiler.h#L16-L20
But we are building the Swift compiler using clang (at least the build log in Windows#19400 is invoking T:\llvm\bin\clang-cl.exe). We are checking for defined(_MSC_VER) in quite a lot of places of the code base already, so I assume the check can’t be completely wrong.

@rintaro rintaro merged commit 2795a19 into swiftlang:main Dec 21, 2021
@rintaro
Copy link
Member Author

rintaro commented Dec 21, 2021

Makes sense! Merged.

@rintaro rintaro deleted the ide-completion-enumbitfield branch December 21, 2021 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahoppen ahoppen ahoppen approved these changes

@bnbarham bnbarham bnbarham approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rintaro @ahoppen @bnbarham