Improve opaque pointers support #86873

nikic · 2021-07-04T21:18:36Z

Opaque pointers are coming, and rustc is not ready.

This adds partial support by passing an explicit load type to LLVM. Two issues I've encountered:

The necessary type was not available at the point where non-temporal copies were generated. I've pushed the code for that upwards out of the memcpy implementation and moved the position of a cast to make do with the types we have available. (I'm not sure that cast is needed at all, but have retained it in the interest of conservativeness.)
The PlaceRef::project_deref() function used during debuginfo generation seems to be buggy in some way -- though I haven't figured out specifically what it does wrong. Replacing it with load_operand().deref() did the trick, but I don't really know what I'm doing here.

rust-highfive · 2021-07-04T21:18:39Z

r? @varkor

(rust-highfive has picked a reviewer for you, use r? to override)

nikic · 2021-07-04T21:20:16Z

r? @nagisa

nagisa · 2021-07-09T12:15:11Z

@bors try @rust-timer queue

rust-timer · 2021-07-09T12:15:12Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-07-09T12:15:21Z

⌛ Trying commit c4d595890da5d8387accc188d707a2ffa5c5bf37 with merge e84cde51e33cced276b7f47049d33d6df462ec20...

nagisa · 2021-07-09T12:23:48Z

compiler/rustc_codegen_ssa/src/mir/place.rs

@@ -403,15 +403,7 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> {
    }

    pub fn project_deref<Bx: BuilderMethods<'a, 'tcx, Value = V>>(&self, bx: &mut Bx) -> Self {


This seems like it was an optimisation to me. Added in #83941, which suggests that perhaps it has something to do with complexity? cc @wesleywiser do you remember why you created this method instead of the more straightforward load_operand().deref()?

I'm struggling to remember but I think I just didn't realize you could do that. From a quick glance, it seems to fine to me to replace this with load_operand().deref().

In that case I would perhaps explore inlining this method into its only use-site.

bors · 2021-07-09T12:31:26Z

💔 Test failed - checks-actions

nagisa · 2021-07-09T12:31:52Z

compiler/rustc_codegen_llvm/src/builder.rs

@@ -486,7 +488,7 @@ impl BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> {
                }
            }
            let llval = const_llval.unwrap_or_else(|| {
-                let load = self.load(place.llval, place.align);
+                let load = self.load(self.backend_type(place.layout), place.llval, place.align);


Nit: Since this is within LLVM specific code, I would probably call the more direct place.layout.llvm_type(self) here, as is done throughout the rest of this crate.

nagisa · 2021-07-09T12:50:46Z

compiler/rustc_codegen_llvm/src/va_arg.rs

@@ -39,7 +39,7 @@ fn emit_direct_ptr_va_arg(
        list.immediate()
    };

-    let ptr = bx.load(va_list_addr, bx.tcx().data_layout.pointer_align.abi);
+    let ptr = bx.load(bx.type_i8p(), va_list_addr, bx.tcx().data_layout.pointer_align.abi);


Perhaps:

let va_list_ty = bx.type_i8p() let va_list_ptr_ty = bx.type_ptr_to(va_list_ty); // <snip> let ptr = bx.load(va_list_ty, va_list_addr, bx.tcx().data_layout.pointer_align.abi);

compiler/rustc_codegen_llvm/src/va_arg.rs

nagisa · 2021-07-09T12:59:07Z

compiler/rustc_codegen_ssa/src/mir/block.rs

@@ -287,8 +287,9 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                        llval
                    }
                };
-                let addr = bx.pointercast(llslot, bx.type_ptr_to(bx.cast_backend_type(&cast_ty)));
-                bx.load(addr, self.fn_abi.ret.layout.align.abi)
+                let llty = bx.cast_backend_type(&cast_ty);


This is not necessarily a llvm type . Perhaps just ty? (though we call vals llval still...)

nagisa · 2021-07-09T12:59:58Z

Broadly speaking this seems good, but I imagine we'll need a similar change for stores too, and later down the line geps, etc?

Instead of determining it from the pointer type, explicitly pass the type to load.

This makes load generation compatible with opaque pointers. The generation of nontemporal copies still accesses the pointer element type, as fixing this requires more movement.

I'm not really sure what is wrong here, but I was getting load type mismatches in the debuginfo code (which is the only place using this function). Replacing the project_deref() implementation with a generic load_operand + deref did the trick.

Simply shift the bitcast from the store to the load, so that we can use the destination type. I'm not sure the bitcast is really necessary, but keeping it for now.

nikic · 2021-07-09T20:17:13Z

Broadly speaking this seems good, but I imagine we'll need a similar change for stores too, and later down the line geps, etc?

It's not needed for stores (because the value type is available through the value operand), but will have to be done for GEPs and calls.

nikic · 2021-07-09T21:02:57Z

@bors try @rust-timer queue

Should work after the rebase...

rust-timer · 2021-07-09T21:02:59Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-07-09T21:03:06Z

⌛ Trying commit 2ce1add with merge 66df3dd988767179a593e0afd748a4eee48991ac...

bors · 2021-07-09T21:51:14Z

☀️ Try build successful - checks-actions
Build commit: 66df3dd988767179a593e0afd748a4eee48991ac (66df3dd988767179a593e0afd748a4eee48991ac)

rust-timer · 2021-07-09T21:51:15Z

Queued 66df3dd988767179a593e0afd748a4eee48991ac with parent 3eff244, future comparison URL.

rust-timer · 2021-07-10T01:05:10Z

Finished benchmarking try commit (66df3dd988767179a593e0afd748a4eee48991ac): comparison url.

Summary: This benchmark run did not return any significant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

nagisa · 2021-07-10T12:37:19Z

@bors r+ Nice!

bors · 2021-07-10T12:37:20Z

📌 Commit 2ce1add has been approved by nagisa

bors · 2021-07-10T12:49:22Z

⌛ Testing commit 2ce1add with merge a4d6c8b00efab9c22f0a7c3b84e05056c948c1cb...

rust-log-analyzer · 2021-07-10T14:11:15Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

bors · 2021-07-10T14:11:16Z

💔 Test failed - checks-actions

nagisa · 2021-07-10T14:22:36Z

@bors retry looks like a spurious infrastructure failure.

bors · 2021-07-10T19:01:47Z

⌛ Testing commit 2ce1add with merge 432e145...

bors · 2021-07-10T21:42:35Z

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing 432e145 to master...

rust-highfive assigned varkor Jul 4, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 4, 2021

rust-highfive assigned nagisa and unassigned varkor Jul 4, 2021

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 9, 2021

nagisa reviewed Jul 9, 2021

View reviewed changes

bors 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 Jul 9, 2021

This comment has been minimized.

Sign in to view

nagisa reviewed Jul 9, 2021

View reviewed changes

compiler/rustc_codegen_llvm/src/va_arg.rs Show resolved Hide resolved

nagisa reviewed Jul 9, 2021

View reviewed changes

nikic added 4 commits July 9, 2021 22:00

Pass type when creating atomic load

33e9a6b

Instead of determining it from the pointer type, explicitly pass the type to load.

Pass type when creating load

4560efe

This makes load generation compatible with opaque pointers. The generation of nontemporal copies still accesses the pointer element type, as fixing this requires more movement.

Fix project_deref() implementation

208173f

I'm not really sure what is wrong here, but I was getting load type mismatches in the debuginfo code (which is the only place using this function). Replacing the project_deref() implementation with a generic load_operand + deref did the trick.

Don't access pointer element type for nontemporal store

2ce1add

Simply shift the bitcast from the store to the load, so that we can use the destination type. I'm not sure the bitcast is really necessary, but keeping it for now.

nikic force-pushed the opaque-ptrs branch from c4d5958 to 2ce1add Compare July 9, 2021 20:15

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 10, 2021

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 Jul 10, 2021

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 Jul 10, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 10, 2021

bors merged commit 432e145 into rust-lang:master Jul 10, 2021

rustbot added this to the 1.55.0 milestone Jul 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve opaque pointers support #86873

Improve opaque pointers support #86873

nikic commented Jul 4, 2021 •

edited

Loading

rust-highfive commented Jul 4, 2021

nikic commented Jul 4, 2021

nagisa commented Jul 9, 2021

rust-timer commented Jul 9, 2021

bors commented Jul 9, 2021

nagisa Jul 9, 2021 •

edited

Loading

wesleywiser Jul 9, 2021

nagisa Jul 9, 2021

bors commented Jul 9, 2021

This comment has been minimized.

nagisa Jul 9, 2021

nagisa Jul 9, 2021 •

edited

Loading

nagisa Jul 9, 2021

nagisa commented Jul 9, 2021 •

edited

Loading

nikic commented Jul 9, 2021

nikic commented Jul 9, 2021

rust-timer commented Jul 9, 2021

bors commented Jul 9, 2021

bors commented Jul 9, 2021

rust-timer commented Jul 9, 2021

rust-timer commented Jul 10, 2021

nagisa commented Jul 10, 2021

bors commented Jul 10, 2021

bors commented Jul 10, 2021

rust-log-analyzer commented Jul 10, 2021

bors commented Jul 10, 2021

nagisa commented Jul 10, 2021

bors commented Jul 10, 2021

bors commented Jul 10, 2021

		@@ -403,15 +403,7 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> {
		}

		pub fn project_deref<Bx: BuilderMethods<'a, 'tcx, Value = V>>(&self, bx: &mut Bx) -> Self {

Improve opaque pointers support #86873

Improve opaque pointers support #86873

Conversation

nikic commented Jul 4, 2021 • edited Loading

rust-highfive commented Jul 4, 2021

nikic commented Jul 4, 2021

nagisa commented Jul 9, 2021

rust-timer commented Jul 9, 2021

bors commented Jul 9, 2021

nagisa Jul 9, 2021 • edited Loading

Choose a reason for hiding this comment

wesleywiser Jul 9, 2021

Choose a reason for hiding this comment

nagisa Jul 9, 2021

Choose a reason for hiding this comment

bors commented Jul 9, 2021

This comment has been minimized.

nagisa Jul 9, 2021

Choose a reason for hiding this comment

nagisa Jul 9, 2021 • edited Loading

Choose a reason for hiding this comment

nagisa Jul 9, 2021

Choose a reason for hiding this comment

nagisa commented Jul 9, 2021 • edited Loading

nikic commented Jul 9, 2021

nikic commented Jul 9, 2021

rust-timer commented Jul 9, 2021

bors commented Jul 9, 2021

bors commented Jul 9, 2021

rust-timer commented Jul 9, 2021

rust-timer commented Jul 10, 2021

nagisa commented Jul 10, 2021

bors commented Jul 10, 2021

bors commented Jul 10, 2021

rust-log-analyzer commented Jul 10, 2021

bors commented Jul 10, 2021

nagisa commented Jul 10, 2021

bors commented Jul 10, 2021

bors commented Jul 10, 2021

nikic commented Jul 4, 2021 •

edited

Loading

nagisa Jul 9, 2021 •

edited

Loading

nagisa Jul 9, 2021 •

edited

Loading

nagisa commented Jul 9, 2021 •

edited

Loading