Refactoring after the PlaceValue addition

[scottmcm]

I added PlaceValue in #123775, but kept that one line-by-line simple because it touched so many places.

This goes through to add more helpers & docs, and change some PlaceRef to PlaceValue where the type didn't need to be included.

No behaviour changes -- the codegen is exactly the same.

[rustbot]

r? @compiler-errors

rustbot has assigned @compiler-errors.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[scottmcm]

I'm happy that #122053 landed since it means that it's reasonable to alloca a PlaceValue without a type.

[saethlin]

r? saethlin

[bors]

☔ The latest upstream changes (presumably #123886) made this pull request unmergeable. Please resolve the merge conflicts.

[aapoalas]

I don't exactly understand the entire context and do not know the code base, but reading through I found no technical issues that would've struck out. Seems like a mostly nice mechanical cleanup: The pointer_parts helper function seemed a little magical (or too broad?) as taking it into use removed some inline comments in one place but after seeing what it actually does it's probably my favourite part of this PR. Nice and simple thing that had been repeated manually in way too many places.

Thinking on it now, I wonder if the helper should be marked #[inline]? Then again, LLVM is probably smarter than we are here.

Another thought I had was if some of the fields on PlaceValue and/or PlaceRef would benefit from being less public, so as to avoid eg. PlaceRef { val: llval .. } type of inline creation popping up again after this refactor.

Anyway, from someone not fully capable of saying this: Looks good to me.

[saethlin]

Thinking on it now, I wonder if the helper should be marked #[inline]? Then again, LLVM is probably smarter than we are here.

For code in the compiler, that attribute has very little impact because the dist build use PGO+LTO+BOLT which tends to find all the obvious inlinings.

I agree that this is a nice improvement to a lot of this code. Thanks!

@bors r+

[bors]

📌 Commit 9be16eb has been approved by saethlin

It is now in the queue for this repository.

[scottmcm]

if some of the fields on PlaceValue and/or PlaceRef would benefit from being less public

I look forward to seeing restrictions get to the point where we'd be willing to use it in the compiler. Because I agree, but not enough to want to move everything to use accessor methods for things.

If the fields being read-only was a possibility -- so you could still match the types, etc -- then I agree that'd be great.

[bors]

⌛ Testing commit 9be16eb with merge 8b64adc...

[bors]

☀️ Test successful - checks-actions
Approved by: saethlin
Pushing 8b64adc to master...

[rust-timer]

Finished benchmarking commit (8b64adc): 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)	-	-	0
Improvements ✅ (primary)	-3.3%	[-3.3%, -3.3%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.3%	[-3.3%, -3.3%]	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)	1.1%	[1.1%, 1.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 673.458s -> 676.399s (0.44%)
Artifact size: 315.85 MiB -> 315.85 MiB (-0.00%)