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

Rust doesn't use niche in reference (or pointer) to slice #132235

Open
VorfeedCanal opened this issue Oct 27, 2024 · 14 comments
Open

Rust doesn't use niche in reference (or pointer) to slice #132235

VorfeedCanal opened this issue Oct 27, 2024 · 14 comments
Labels
A-layout Area: Memory layout of types C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such E-needs-design This issue needs exploration and design to see how and if we can fix/implement it T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@VorfeedCanal
Copy link

From reddit thread: why can't rust optimize the size of Option<Option<&str>>?

Indeed, in spite of the fact that &str doesn't all use more than half of possible bit sequences (top bit of 2nd word is always zero, plus first word can not be zero) only Option<&str> is optimized

Would it make sense to teach compiler about that niche or is it prevented by some deeper issue?

@VorfeedCanal VorfeedCanal added the C-bug Category: This is a bug. label Oct 27, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 27, 2024
@workingjubilee
Copy link
Member

Duplicate of #119507

@workingjubilee workingjubilee marked this as a duplicate of #119507 Oct 27, 2024
@workingjubilee workingjubilee closed this as not planned Won't fix, can't repro, duplicate, stale Oct 27, 2024
@the8472
Copy link
Member

the8472 commented Oct 27, 2024

I don't think that's correct. #119507 is about using niches across fields. This one is about having niches in slice lengths.

And I've attempted to improve that in #116542 but so far perf hasn't been looking good. Maybe because the IR gets more complicated.

@workingjubilee
Copy link
Member

@the8472 Isn't it a duplicate of #125363 then?

@workingjubilee
Copy link
Member

In any case, I don't really think having a zillion tiny issues regarding the layout code, which is a big ball of mud, is very helpful. It would be better to collect the various possible optimizations and reason about them in a central manner, rather than trying to make tiny adjustments to spot-fix the code.

@the8472
Copy link
Member

the8472 commented Oct 27, 2024

#125363 is similar to #119507 actually.

And having a "more niche optimizations" super issue would help. But some of the sub-issues are still useful because some specific cases can likely be fixed without difficult perf tradeoffs while others are less certain.

@workingjubilee
Copy link
Member

Honestly, I'm not sure why your PR #116542 is seemingly both trying to add metadata and alter the layout of slices.

@workingjubilee
Copy link
Member

The fundamental issue is that all of the issues we're discussing are trying to do the same thing: "There are two niches, one in each of two separate parts of this. Optimize the layout based on that."

In that regard, a struct and a slice should not be treated significantly differently.

@the8472
Copy link
Member

the8472 commented Oct 27, 2024

This issue isn't about jointly using two pre-existing niches. It's about adding a new set of niches to the length field in slice-refs, by exploiting the isize::MAX bytes upper limit, something we're currently not doing.

Honestly, I'm not sure why your PR #116542 is seemingly both trying to add metadata and alter the layout of slices.

They're based on the same thing, derived from the validity ranges of the fields.

@workingjubilee
Copy link
Member

They may be based on the same thing, but that doesn't mean the changes are functionally dependent in the implementation.

@workingjubilee
Copy link
Member

...Unless they are, in which case that segment of the layout code is possibly in even more dire need of being scrapped and rewritten than I thought.

@the8472
Copy link
Member

the8472 commented Oct 27, 2024

I mean if we define the slice length field validity range - which currently isn't explicitly set and so defaults to the same validity as usize - then both metadata and niches fall out of that automatically. Treating them differently for the purpose of llvmir metadata and niche calculation would take extra effort.

@workingjubilee
Copy link
Member

I suppose I'll reopen this then, but now I'm wondering if we can convince the layout code that &[T] is secretly a funny-looking struct.

@the8472
Copy link
Member

the8472 commented Oct 27, 2024

I think layout code should already be seeing an univariant ADT with two fields and scalarpair abi. My PR does result in layout changes.

@Vrtgs
Copy link

Vrtgs commented Oct 28, 2024

I suppose I'll reopen this then, but now I'm wondering if we can convince the layout code that &[T] is secretly a funny-looking struct.

The issue with this is that when T is a zst, it is now okay for a &[T] to have more than isize::MAX elements

Also isize::MAX is an even bigger range than the null optimization, so I think it should be favored instead of the null optimization if only one niche optimization can be applied

@lolbinarycat lolbinarycat added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-layout Area: Memory layout of types labels Oct 28, 2024
@jieyouxu jieyouxu added C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such E-needs-design This issue needs exploration and design to see how and if we can fix/implement it and removed C-bug Category: This is a bug. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. E-needs-design This issue needs exploration and design to see how and if we can fix/implement it labels Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-layout Area: Memory layout of types C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such E-needs-design This issue needs exploration and design to see how and if we can fix/implement it T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants