read_target_uint => read_target_ptr outside rustc_middle (nfc) #84826

workingjubilee · 2021-05-02T16:24:14Z

Uses a more narrow function for the actual purposes the callers have put it to, and locks down some previously public functions into private ones. A small refactoring with no functional changes, but hopefully makes future ones easier down the line.

I want to be sure the refactoring in cg_clif is correct, and if any further improvement can be made there, or if there's an obvious better place for read_target_ptr to live, so

r? @bjorn3

This function has no external users at the moment, so lock it down. This logic can be derived from std methods, so it's no real loss.

rust-highfive · 2021-05-02T16:24:16Z

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Most public callers of read_target_uint are using it for the purpose of acquiring a pointer-sized uint. Using read_target_ptr instead makes this usage more clear, and exploits the knowledge of the caller to make the function into a clear "table lookup". This enables making read_target_uint private, and is intended to help simplify refactoring the interpreter in rustc_middle. In cg_clif, one usage of read_target_uint was to obtain a u32, which has been specialized to match the exact numeric width.

bjorn3

The changes to cg_clif seem correct. I don't personally see the point of this, but I will leave it to @oli-obk to decide.

r? @oli-obk

compiler/rustc_codegen_cranelift/src/intrinsics/simd.rs

bjorn3 · 2021-05-02T17:22:50Z

compiler/rustc_middle/src/mir/interpret/mod.rs

+/// as the modules using this don't support wider pointer values
+/// FIXME(jubilee): Move this out of here and closer to the things using it!
+#[inline]
+pub fn read_target_ptr(endian: Endian, ptr_size: usize, bytes: &[u8]) -> u64 {


This should probably take Size instead of usize.

Size is a u64, so on a 32-bit host that is over-weighting this input (even tho' the output should be u64 regardless). Might not matter after inlining, I guess.

I thought about adding an enum to rustc_target so that can be matched on directly for more type-strictness, though.

It's just standard to use Size to describe such a property of the target, and the consistency helps avoid bugs. Also 32-bit host performance shouldn't be a significant concern (even tho memory usage in data structures may be).

@workingjubilee, this code doesn't seem terribly idiomatic, and if your concern is input overhead, why not just call bytes.len()? Besides, it's definitely going to be optimized away during inlining, and if you call bytes.len() then it's easier for the optimizer to see what the variable means.

@workingjubilee Oh, you're probably right! Even taking an input seems a bit silly, now that I think of it. I must be reading too many C codebases lately. 😅

bjorn3 · 2021-05-02T17:24:20Z

compiler/rustc_codegen_cranelift/src/constant.rs

                );
-                read_target_uint(endianness, bytes).unwrap()
+                read_target_ptr(endianness, ptr_size, bytes)


This name makes it seem like it reads the whole pointer instead of just the addend, leaving the target to which the addend is relative out.

read_target_usize perhaps?

Co-authored-by: bjorn3 <bjorn3@users.noreply.github.com>

bjorn3 · 2021-05-02T19:28:09Z

r? @oli-obk

(highfive ignored the previous re-assignment)

oli-obk · 2021-05-02T22:39:47Z

compiler/rustc_codegen_cranelift/src/intrinsics/simd.rs

+                        (Endian::Little, &[a, b, c, d]) => u32::from_le_bytes([a, b, c, d]),
+                        (Endian::Big, &[a, b, c, d]) => u32::from_be_bytes([a, b, c, d]),
+                        (_, _) => unreachable!(),
+                    };


This has become significantly more complex when the complexity was hidden behind a function before. Why was this change made?

oli-obk · 2021-05-02T22:45:58Z

I'm afraid I don't see where your future changes are going. To me this PR just makes things more complex without any (to me) visible gain.

While I do appreciate having more concrete functions for usize reads, that can be achieved by adding a method that calls the current one and trims the result.

Can you talk a bit more about your motivation for this change and the future plans? As it is I would prefer not to accept this PR

eddyb · 2021-05-03T10:38:59Z

compiler/rustc_middle/src/mir/interpret/mod.rs

+        (2, Endian::Little) => u16::from_le_bytes(bytes.try_into().unwrap()) as u64,
+        (2, Endian::Big) => u16::from_be_bytes(bytes.try_into().unwrap()) as u64,
+        (4, Endian::Little) => u32::from_le_bytes(bytes.try_into().unwrap()) as u64,
+        (4, Endian::Big) => u32::from_be_bytes(bytes.try_into().unwrap()) as u64,
+        (8, Endian::Little) => u64::from_le_bytes(bytes.try_into().unwrap()),
+        (8, Endian::Big) => u64::from_be_bytes(bytes.try_into().unwrap()),


I don't like explicitly handling several possibilities using host integers, when it can be done by e.g. copying bytes into an [0u8; 8], reversing the ..bytes.len() slice of the array if Endian::Big, and then using u64::fom_le_bytes.

@oli does miri already have some uniform (over integer sizes) code for doing this?

Actually, why not just keep using read_target_uint? It seems like the correct abstraction, am I missing something? If you don't want it to part of the rustc_mir::mir::interpret interface, we just have to move it around a bit, right?

does miri already have some uniform (over integer sizes) code for doing this?

Yes, as you noted, read_target_uint is that abstraction. I do see the point in an abstraction returning u64 for usize purposes, but imo that should just be a wrapper around the existing functions.

As to where to place them... if rustc_middle doesn't need them they can go to rustc_mir, otherwise idk

Hmm, I definitely was overthinking this, in retrospect. Anyways, my main thought was that it should probably exist in, say, rustc_target or something like that.

workingjubilee

Yeah, this probably isn't acceptable without changes, on review. I'm not sure why this @workingjubilee person submitted it. Perhaps to start a conversation? Couldn't they have just poked you on Zulip or something? :^)

But if I had to guess, I would surmise that they were thinking about how to minimize enmeshment between the codegen backends and rustc_middle and had wound up tinkering with this for a bit as a straw example of what reducing that entanglement might look like. Though it's obvious to me, thinking about it now, that such an effort is probably doomed per se as of course the codegen backends have to depend on rustc_middle! However, probably some pieces of the miri interpreter and such could be factored out, or if that would expose too much, then alternatively pieces of rustc_middle like this that are just widely useful and could go into more "utility-oriented" crates, minimizing the weight of what is in the critical path.

workingjubilee · 2021-05-03T21:36:58Z

compiler/rustc_middle/src/mir/interpret/mod.rs

+/// as the modules using this don't support wider pointer values
+/// FIXME(jubilee): Move this out of here and closer to the things using it!
+#[inline]
+pub fn read_target_ptr(endian: Endian, ptr_size: usize, bytes: &[u8]) -> u64 {


@workingjubilee, this code doesn't seem terribly idiomatic, and if your concern is input overhead, why not just call bytes.len()? Besides, it's definitely going to be optimized away during inlining, and if you call bytes.len() then it's easier for the optimizer to see what the variable means.

workingjubilee · 2021-05-03T21:38:36Z

compiler/rustc_middle/src/mir/interpret/mod.rs

+/// as the modules using this don't support wider pointer values
+/// FIXME(jubilee): Move this out of here and closer to the things using it!
+#[inline]
+pub fn read_target_ptr(endian: Endian, ptr_size: usize, bytes: &[u8]) -> u64 {


@workingjubilee Oh, you're probably right! Even taking an input seems a bit silly, now that I think of it. I must be reading too many C codebases lately. 😅

workingjubilee · 2021-05-03T21:40:51Z

compiler/rustc_middle/src/mir/interpret/mod.rs

+        (2, Endian::Little) => u16::from_le_bytes(bytes.try_into().unwrap()) as u64,
+        (2, Endian::Big) => u16::from_be_bytes(bytes.try_into().unwrap()) as u64,
+        (4, Endian::Little) => u32::from_le_bytes(bytes.try_into().unwrap()) as u64,
+        (4, Endian::Big) => u32::from_be_bytes(bytes.try_into().unwrap()) as u64,
+        (8, Endian::Little) => u64::from_le_bytes(bytes.try_into().unwrap()),
+        (8, Endian::Big) => u64::from_be_bytes(bytes.try_into().unwrap()),


Hmm, I definitely was overthinking this, in retrospect. Anyways, my main thought was that it should probably exist in, say, rustc_target or something like that.

bors · 2021-05-19T13:15:43Z

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

crlf0710 · 2021-06-05T09:40:20Z

@workingjubilee Ping from triage, what's next steps here?

JohnCSimon · 2021-06-22T04:22:02Z

@workingjubilee Ping from triage. Still has a merge conflict

workingjubilee · 2021-07-05T20:07:30Z

Ah sorry. Missed pings! Going to close this and maybe return to a slightly sideways idea.

Lock down write_target_uint

e00196d

This function has no external users at the moment, so lock it down. This logic can be derived from std methods, so it's no real loss.

rust-highfive assigned bjorn3 May 2, 2021

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

workingjubilee force-pushed the refactor-target-uint branch from 3229bc7 to c04c0b9 Compare May 2, 2021 16:28

bjorn3 reviewed May 2, 2021

View reviewed changes

Update compiler/rustc_codegen_cranelift/src/intrinsics/simd.rs

c7d4c8b

Co-authored-by: bjorn3 <bjorn3@users.noreply.github.com>

rust-highfive assigned oli-obk and unassigned bjorn3 May 2, 2021

oli-obk reviewed May 2, 2021

View reviewed changes

eddyb reviewed May 3, 2021

View reviewed changes

workingjubilee commented May 3, 2021

View reviewed changes

crlf0710 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 Jun 5, 2021

workingjubilee closed this Jul 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

read_target_uint => read_target_ptr outside rustc_middle (nfc) #84826

read_target_uint => read_target_ptr outside rustc_middle (nfc) #84826

workingjubilee commented May 2, 2021

rust-highfive commented May 2, 2021

bjorn3 left a comment

bjorn3 May 2, 2021

workingjubilee May 2, 2021 •

edited

Loading

eddyb May 3, 2021

workingjubilee May 3, 2021

workingjubilee May 3, 2021

bjorn3 May 2, 2021

workingjubilee May 2, 2021

bjorn3 May 2, 2021

bjorn3 commented May 2, 2021

oli-obk May 2, 2021

oli-obk commented May 2, 2021

eddyb May 3, 2021

oli-obk May 3, 2021

workingjubilee May 3, 2021

workingjubilee left a comment

workingjubilee May 3, 2021

workingjubilee May 3, 2021

workingjubilee May 3, 2021

bors commented May 19, 2021

crlf0710 commented Jun 5, 2021

JohnCSimon commented Jun 22, 2021

workingjubilee commented Jul 5, 2021

read_target_uint => read_target_ptr outside rustc_middle (nfc) #84826

read_target_uint => read_target_ptr outside rustc_middle (nfc) #84826

Conversation

workingjubilee commented May 2, 2021

rust-highfive commented May 2, 2021

bjorn3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee May 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjorn3 commented May 2, 2021

Choose a reason for hiding this comment

oli-obk commented May 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented May 19, 2021

crlf0710 commented Jun 5, 2021

JohnCSimon commented Jun 22, 2021

workingjubilee commented Jul 5, 2021

workingjubilee May 2, 2021 •

edited

Loading