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

Use dangling for default slices (like vec and box do) #62487

Closed
wants to merge 1 commit into from

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented Jul 8, 2019

More consistent and means LLVM & linking don't need to faff about with useless private constants.

(I suspect the previous way might also have meant it could return different things in different translation units, but I haven't actually checked whether that was happening.)

More consistent and means LLVM doesn't need to faff about with useless private constants.
@rust-highfive
Copy link
Collaborator

r? @aidanhs

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 8, 2019
@cramertj
Copy link
Member

cramertj commented Jul 8, 2019

I frequently use &[] and &mut [] literals in code, so if this is actually a perf / codegen improvement it'd be great to see it on all uses of &[] and &mut [].

@Aaron1011
Copy link
Member

Aaron1011 commented Jul 8, 2019

I would be somewhat concerned if from_raw_parts(ptr::NonNull::dangling().as_ptr(), 0) represented any perf/codegen improvement over &[] - that would mean that an obscure, non-obvious way of constructing an empty slice should actually be preferred over the obvious, easy-to-read way.

@scottmcm
Copy link
Member Author

scottmcm commented Jul 8, 2019

There is a slight codegen difference, but I wouldn't expect any perf difference from it.

I just wanted this check to pass so my debugging printfs of .as_ptr()s were more readable:

std::ptr::eq(<&[u32]>::default(), <Vec<u32>>::default().as_slice())

@totsteps
Copy link
Member

ping from wg-triage, @aidanhs any updates on this?

@fmckeogh
Copy link
Member

fmckeogh commented Jul 31, 2019

Second ping from wg-triage, pinging reviewer from T-compiler per procedure.

@nagisa

@hdhoang
Copy link
Contributor

hdhoang commented Aug 9, 2019

Third ping from triage, pinging reviewer in T-compiler @estebank

@estebank estebank added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 9, 2019
Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

The code change looks reasonable but I share the concerns about codegen differences with &[]. I would prefer other reviewers in @rust-lang/compiler took a second look to verify we're not missing something obvious.

@scottmcm
Copy link
Member Author

scottmcm commented Aug 9, 2019

Now that I'm back from vacation, I'd also be willing to take a look at changing something in mir transforms to generate the simpler version of this, if someone could point me to somewhere appropriate. (Presumably in the promotion code somewhere? Or maybe I could change &_N in MIR to just be a constant when typeof(_N) is a ZST? But I don't know how to check generic ZSTs...)

@eddyb
Copy link
Member

eddyb commented Aug 9, 2019

@scottmcm You don't want to do this in MIR, but rather rustc_codegen_llvm::consts.
That is, whenever a constant allocation of size 0 is needed, instead of adding a LLVM global, you can constant-inttoptr-cast the allocation's alignment to the right pointer type.
@oli-obk might be able to help with this.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 9, 2019

Oh, I never thought about ZSTs when messing with llvm and constants. You should be able to just add an if alloc.size() != 0 around

let init = const_alloc_to_llvm(self, alloc);
if alloc.mutability == Mutability::Mutable {
self.static_addr_of_mut(init, alloc.align, None)
} else {
self.static_addr_of(init, alloc.align, None)
}
and generate a properly aligned pointer in the else branch.

Though such a solution will miss &(&(42, [])).1 (or something similar), because the allocation is large while the pointer doesn't need to point to something large. It's probably irrelevant (I mean we miss this right now, too), and it's probably rare for just the inner reference to be publicly reachable.

@nikomatsakis
Copy link
Contributor

Compiler team meeting:

Assigning to @eddyb to either help with a more general solution or perhaps "just do it".

r? @eddyb

@scottmcm
Copy link
Member Author

Closing in favour of #63635

@scottmcm scottmcm closed this Aug 17, 2019
Centril added a commit to Centril/rust that referenced this pull request Aug 18, 2019
Do not generate allocations for zero sized allocations

Alternative to rust-lang#62487

r? @eddyb

There are other places where we could do this, too, but that would cause `static FOO: () = ();` to not have a unique address
bors added a commit that referenced this pull request Aug 18, 2019
Do not generate allocations for zero sized allocations

Alternative to #62487

r? @eddyb

There are other places where we could do this, too, but that would cause `static FOO: () = ();` to not have a unique address
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.