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

Migrate to LLVM{Get,Set}ValueName2 #64223

Closed
hanna-kruppe opened this issue Sep 6, 2019 · 6 comments · Fixed by #67033
Closed

Migrate to LLVM{Get,Set}ValueName2 #64223

hanna-kruppe opened this issue Sep 6, 2019 · 6 comments · Fixed by #67033
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@hanna-kruppe
Copy link
Contributor

We currently use the C APIs LLVM{Get,Set}ValueName which return/take a 0-terminated char *, but those APIs have been deprecated a while ago in favor of LLVM{Get,Set}ValueName2 which return/take pairs of char * + length (the C-friendly lowering of llvm::StringRef) which need not be 0-terminated. We should migrate to those APIs. Besides avoiding silly error cases for names that contain a 0 byte, it would also make conversions from &str to LLVM name free, and reduce the cost of the opposite direction to an UTF-8 validity check (plus, I suspect many or all code paths that do that could just as well use &[u8], and that conversion actually is 100% free).

@hanna-kruppe hanna-kruppe added C-cleanup Category: PRs that clean code up or issues documenting cleanup. A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. labels Sep 6, 2019
@eddyb
Copy link
Member

eddyb commented Sep 6, 2019

Any idea how old the new APIs are? I don't see any reason not to switch other than that.

@hanna-kruppe
Copy link
Contributor Author

They were added in llvm/llvm-project@025c78f which is in the 7.0 release, what's our minimum supported LLVM version these days?

@hanna-kruppe
Copy link
Contributor Author

The current minimum is 6, but considering the constraints raised when it was bumped from 5 to 6 (#56642) it seems to me that bumping to 7 might be OK already. Worth a try IMO.

@nikic
Copy link
Contributor

nikic commented Sep 6, 2019

@rkruppe We'll have to wait for #63649 to land first, as emscripten is still on LLVM 6.

@jonas-schievink jonas-schievink added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 10, 2019
@hanna-kruppe
Copy link
Contributor Author

I just realized, if we cared enough we could also provide our own equivalent of these functions in src/rustllvm (and just remove them when we drop LLVM 6 support). The C API was just lagging behind, the C++ APIs have supported it for much longer.

@cuviper
Copy link
Member

cuviper commented Dec 2, 2019

FYI, #66973 bumps to LLVM 7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants