Skip to content

[llvm] Remove inline constexpr from AArch64TargetParser.h #71602

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

Closed
wants to merge 2 commits into from

Conversation

rsuderman
Copy link
Contributor

The use of inline constexpr appears to be a premature optimization that can cause failures on some platforms. StringRef has a heavy constructor that can be missed on dylib loading causing the Extension table values to be incorrect. Switching from inline constexpr to const results in no increase in binary size of llc indicating the optimization is unnecessary.

The use of `inline constexpr` appears to be a premature optimization that
can cause failures on some platforms. StringRef has a heavy constructor
that can be missed on dylib loading causing the `Extension` table values
to be incorrect. Switching from `inline constexpr` to `const` results
in no increase in binary size of `llc` indicating the optimization is
unnecessary.
@rsuderman rsuderman requested a review from qcolombet November 7, 2023 23:20
Copy link

github-actions bot commented Nov 7, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

@topperc
Copy link
Collaborator

topperc commented Nov 8, 2023

Does using StringLiteral instead of StringRef in the struct help? It has a more specialized constructor for constexpr.

@rsuderman
Copy link
Contributor Author

rsuderman commented Nov 8, 2023

Does using StringLiteral instead of StringRef in the struct help? It has a more specialized constructor for constexpr.

I attempted to replace with StringLiteral instead of StringRef however some of the constructors use empty list initializers which is not supported by the StringLiteral constructors. I performed the replacement for the cases where it was possible and it still did not avoid the Extension tables corruption issue.

It is worth noting I was able to simply drop the inline keyword to achieve the same effect as const if that is preferable.

@topperc
Copy link
Collaborator

topperc commented Nov 8, 2023

Does using StringLiteral instead of StringRef in the struct help? It has a more specialized constructor for constexpr.

I attempted to replace with StringLiteral instead of StringRef however some of the constructors use empty list initializers which is not supported by the StringLiteral constructors.

Does using "" instead of the empty list initializer work?

I performed the replacement for the cases where it was possible and it still did not avoid the Extension tables corruption issue.

What does where it was possible mean? What change did you make?

@qcolombet
Copy link
Collaborator

Given other targets use const in similar situation, that sounds reasonable, but I don't know that part of the code base at all.
So take my comment with a grain of salt.

@topperc
Copy link
Collaborator

topperc commented Nov 8, 2023

Why are these arrays declared in the .h file? They appear to only be used by AArch64TargetParser.cpp

@rsuderman
Copy link
Contributor Author

Why are these arrays declared in the .h file? They appear to only be used by AArch64TargetParser.cpp

Sadly they do have uses outside of the target parser. E.g. in llvm-project/clang/lib/Basic/Targets/AArch64.cpp

@rsuderman rsuderman closed this Apr 19, 2024
@tmatheson-arm
Copy link
Contributor

Just to add some context: IIRC, inline constexpr was used to guarantee that these objects had unique addresses, which was necessary because some equality checks were based on address. That turned out to be unworkable and was removed, so I think the inline constexpr is no longer necessary. It wasn't for optimisation.

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.

4 participants