llvm: replace some deprecated functions, add fixmes #109862

klensy · 2023-04-02T15:53:26Z

Replace some deprecated llvm functions, add FIXME's (for simpler future work), replace some rust custom functions with llvm ones.

rustbot · 2023-04-02T15:53:31Z

r? @WaffleLapkin

(rustbot has picked a reviewer for you, use r? to override)

klensy · 2023-04-02T15:58:39Z

compiler/rustc_codegen_llvm/src/llvm/ffi.rs

+    // FIXME: deprecated, replace with LLVMMDStringInContext2
    pub fn LLVMMDStringInContext(C: &Context, Str: *const c_char, SLen: c_uint) -> &Value;
+
+    // LLVMMDStringInContext but don't own string
+    pub fn LLVMMDStringInContext2(C: &Context, Str: *const c_char, SLen: size_t) -> &Metadata;


I can probably replace LLVMMDStringInContext with LLVMMDStringInContext2 all around, using value<->metadata wrappers around, but don't know is it worth it.

It looks like some of them are sensible to replace at least. E.g. in consts.rs LLVMMDStringInContext and LLVMMDNodeInContext can be replaced with the 2 variants (just need to replace both of them together and it works out fine).

Yes, but void LLVMAddNamedMetadataOperand(LLVMModuleRef M, const char *Name, LLVMValueRef Val)

rust/compiler/rustc_codegen_llvm/src/consts.rs

Lines 507 to 512 in 90a9f69

let meta = llvm::LLVMMDNodeInContext(self.llcx, data.as_ptr(), 2);

llvm::LLVMAddNamedMetadataOperand(

self.llmod,

"wasm.custom_sections\0".as_ptr().cast(),

meta,

);

https://github.com/llvm/llvm-project/blame/02aa966c135172885182ddd6f5d76883c2402bed/llvm/lib/IR/Core.cpp#L1292-L1300
feeds in LLVMValueRef , but LLVMMDNodeInContext2 will return LLVMMetadataRef, so wrap\unwrap (or rewrite LLVMAddNamedMetadataOperand).

Also void LLVMSetMetadata(LLVMValueRef Inst, unsigned KindID, LLVMValueRef Val) prevents refactor too, sadly.

klensy · 2023-04-03T12:28:16Z

compiler/rustc_codegen_llvm/src/llvm/ffi.rs

+    pub fn LLVMBuildCleanupPad<'a>(
        B: &Builder<'a>,
        ParentPad: Option<&'a Value>,
-        ArgCnt: c_uint,
        Args: *const &'a Value,
+        NumArgs: c_uint,
        Name: *const c_char,
    ) -> Option<&'a Value>;


Sometimes llvm type LLVMValueRef modeled in rust as Option<&'a Value>, sometimes as &'a Value, where LLVMValueRef can be nullptr:
https://github.com/llvm/llvm-project/blame/42d1b276f7793999be3f9b6a99efbb143254c729/llvm/lib/IR/Core.cpp#L3170-L3178

It feels wrong and can lead to funny results, or i'm wrong? Don't have strong c++ here, so advice requested.

klensy · 2023-04-03T12:29:58Z

@rustbot author

klensy · 2023-04-03T13:37:07Z

@rustbot ready

WaffleLapkin · 2023-04-03T15:50:57Z

r? nikic

…ugLocation2

… LLVMValueRef, some that accept LLVMMetadataRef, and replacing one with another not always possible without explicit convertion

… LLVMRustMetadataTypeInContext with LLVMMetadataTypeInContext

LLVMRustBuildCleanupPad -> LLVMBuildCleanupPad LLVMRustBuildCleanupRet -> LLVMBuildCleanupRet LLVMRustBuildCatchPad -> LLVMBuildCatchPad LLVMRustBuildCatchRet -> LLVMBuildCatchRet LLVMRustBuildCatchSwitch -> LLVMBuildCatchSwitch

LLVMRustBuildIntCast -> LLVMBuildIntCast2 LLVMRustAddHandler -> LLVMAddHandler

compiler/rustc_codegen_llvm/src/llvm/ffi.rs

nikic · 2023-04-04T18:56:46Z

compiler/rustc_codegen_llvm/src/llvm/ffi.rs

+    // FIXME: deprecated, replace with LLVMMDStringInContext2
    pub fn LLVMMDStringInContext(C: &Context, Str: *const c_char, SLen: c_uint) -> &Value;
+
+    // LLVMMDStringInContext but don't own string
+    pub fn LLVMMDStringInContext2(C: &Context, Str: *const c_char, SLen: size_t) -> &Metadata;


It looks like some of them are sensible to replace at least. E.g. in consts.rs LLVMMDStringInContext and LLVMMDNodeInContext can be replaced with the 2 variants (just need to replace both of them together and it works out fine).

compiler/rustc_codegen_llvm/src/llvm/ffi.rs

klensy · 2023-04-07T12:06:31Z

@rustbot ready

nikic · 2023-04-07T13:24:39Z

@bors r+ rollup

bors · 2023-04-07T13:24:41Z

📌 Commit c0bc001 has been approved by nikic

It is now in the queue for this repository.

llvm: replace some deprecated functions, add fixmes Replace some deprecated llvm functions, add FIXME's (for simpler future work), replace some rust custom functions with llvm ones.

Noratrieb · 2023-04-07T19:52:58Z

maybe this was the cause of https://github.com/notifications#issuecomment-1500588281?
@bors rollup=iffy
feel free to r- if you think it's likely, but I'll leave it open for now to test it again

bors · 2023-04-07T20:13:48Z

⌛ Testing commit c0bc001 with merge 7d6375c5f6ff8809fe43ad38f6fb068b850dd2d9...

bors · 2023-04-07T20:39:59Z

💔 Test failed - checks-actions

klensy · 2023-04-07T21:29:13Z

Yes, can reproduce when building stage2 compiler, but don't have idea why, for now.

fails on f41e711

klensy · 2023-04-08T09:53:55Z

@rustbot ready fixed issue with B/bool type

nikic · 2023-04-08T15:55:23Z

@bors r+

bors · 2023-04-08T15:55:25Z

📌 Commit fdfca76 has been approved by nikic

It is now in the queue for this repository.

bors · 2023-04-08T15:58:01Z

⌛ Testing commit fdfca76 with merge dd2b195...

bors · 2023-04-08T18:21:16Z

☀️ Test successful - checks-actions
Approved by: nikic
Pushing dd2b195 to master...

rust-timer · 2023-04-08T19:37:43Z

Finished benchmarking commit (dd2b195): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.1%	[2.9%, 3.3%]	2
Improvements ✅ (primary)	-2.5%	[-2.5%, -2.5%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.5%	[-2.5%, -2.5%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.4%	[3.4%, 3.4%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

rustbot assigned WaffleLapkin Apr 2, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 2, 2023

klensy changed the title ~~llvm:~~ llvm: replace some deprecated functions, add fixmes Apr 2, 2023

klensy commented Apr 2, 2023

View reviewed changes

klensy commented Apr 3, 2023

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 3, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 3, 2023

WaffleLapkin approved these changes Apr 3, 2023

View reviewed changes

rustbot assigned nikic and unassigned WaffleLapkin Apr 3, 2023

klensy added 6 commits April 4, 2023 15:12

replace deprecated LLVMSetCurrentDebugLocation with LLVMSetCurrentDeb…

0b5f9ac

…ugLocation2

add bunch of fixmes: currently there exist some functions that accept…

7d6181e

… LLVMValueRef, some that accept LLVMMetadataRef, and replacing one with another not always possible without explicit convertion

replace LLVMRustMetadataAsValue with LLVMMetadataAsValue

c53a9fa

replace LLVMRustAppendModuleInlineAsm with LLVMAppendModuleInlineAsm,…

076116b

… LLVMRustMetadataTypeInContext with LLVMMetadataTypeInContext

Use existing llvm methods, instead of rust wrappers for:

cc77ae0

LLVMRustBuildCleanupPad -> LLVMBuildCleanupPad LLVMRustBuildCleanupRet -> LLVMBuildCleanupRet LLVMRustBuildCatchPad -> LLVMBuildCatchPad LLVMRustBuildCatchRet -> LLVMBuildCatchRet LLVMRustBuildCatchSwitch -> LLVMBuildCatchSwitch

replace

f41e711

LLVMRustBuildIntCast -> LLVMBuildIntCast2 LLVMRustAddHandler -> LLVMAddHandler

nikic reviewed Apr 4, 2023

View reviewed changes

review

c0bc001

klensy force-pushed the llvm-dd branch from f79bb48 to c0bc001 Compare April 5, 2023 12:09

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 7, 2023

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Apr 7, 2023

This was referenced Apr 7, 2023

Rollup of 5 pull requests #110053

Closed

Rollup of 5 pull requests #110054

Closed

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 7, 2023

This comment has been minimized.

Sign in to view

fixup: use Bool instead of bool

fdfca76

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 8, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 8, 2023

bors merged commit dd2b195 into rust-lang:master Apr 8, 2023

rustbot added this to the 1.70.0 milestone Apr 8, 2023

bors mentioned this pull request Apr 8, 2023

Prevent redundant casts and create fewer LLVM builders #105549

Closed

workingjubilee mentioned this pull request May 30, 2024

Switch to LLVM{Get,Set}CurrentDebugLocation2. #68953

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

llvm: replace some deprecated functions, add fixmes #109862

llvm: replace some deprecated functions, add fixmes #109862

klensy commented Apr 2, 2023

rustbot commented Apr 2, 2023

klensy Apr 2, 2023

nikic Apr 4, 2023

klensy Apr 5, 2023 •

edited

Loading

klensy Apr 5, 2023

klensy Apr 3, 2023

klensy commented Apr 3, 2023

klensy commented Apr 3, 2023

WaffleLapkin commented Apr 3, 2023

nikic Apr 4, 2023

klensy commented Apr 7, 2023

nikic commented Apr 7, 2023

bors commented Apr 7, 2023

Noratrieb commented Apr 7, 2023

bors commented Apr 7, 2023

bors commented Apr 7, 2023

klensy commented Apr 7, 2023 •

edited

Loading

This comment has been minimized.

klensy commented Apr 8, 2023

nikic commented Apr 8, 2023

bors commented Apr 8, 2023

bors commented Apr 8, 2023

bors commented Apr 8, 2023

rust-timer commented Apr 8, 2023

	let meta = llvm::LLVMMDNodeInContext(self.llcx, data.as_ptr(), 2);
	llvm::LLVMAddNamedMetadataOperand(
	self.llmod,
	"wasm.custom_sections\0".as_ptr().cast(),
	meta,
	);

llvm: replace some deprecated functions, add fixmes #109862

llvm: replace some deprecated functions, add fixmes #109862

Conversation

klensy commented Apr 2, 2023

rustbot commented Apr 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klensy Apr 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klensy commented Apr 3, 2023

klensy commented Apr 3, 2023

WaffleLapkin commented Apr 3, 2023

Choose a reason for hiding this comment

klensy commented Apr 7, 2023

nikic commented Apr 7, 2023

bors commented Apr 7, 2023

Noratrieb commented Apr 7, 2023

bors commented Apr 7, 2023

bors commented Apr 7, 2023

klensy commented Apr 7, 2023 • edited Loading

This comment has been minimized.

klensy commented Apr 8, 2023

nikic commented Apr 8, 2023

bors commented Apr 8, 2023

bors commented Apr 8, 2023

bors commented Apr 8, 2023

rust-timer commented Apr 8, 2023

Overall result: no relevant changes - no action needed

klensy Apr 5, 2023 •

edited

Loading

klensy commented Apr 7, 2023 •

edited

Loading