Replace return variable with its destination. by ptersilie · Pull Request #67 · ykjit/yk

ptersilie · 2020-05-28T21:25:06Z

This change replaces the default return variable $0 with the variable
in the outer context where the return value will end up after leaving
the function. This saves us an instruction when we compile the trace.
More importantly however, this guards us against a future optimisation
in rustc that allows SIR to assign to $0 multiple times and at the
beginning of a block, which could lead to another function overwriting
its value (see rust-lang/rust#72205).

While we are here, also fix a bug in the variable renaming code.
Currently, functions that are called consecutively (i.e. they are not
nested) use the same offset to rename their variables (the offset of
their outer context), which leads to them sharing the same variable
names. By using an accumulator which is continuously increased to
calculate the offset, we make sure consecutive functions have increasing
variable names, even when after leaving a function the offset is
temporarily reset to that of the outer context.

ptersilie · 2020-05-28T21:28:11Z

Last commit needs squashing.

ltratt · 2020-05-28T21:35:49Z

ykcompile/src/lib.rs


+    /// Returns the currently assigned register for a given `Local`. Similar to `local_to_reg` but
+    /// read-only, i.e. this function doesn't assign `Local`'s to registers.
+    fn get_reg(&self, local: &Local) -> Result<u8, CompileError> {


AFAICS this function is dead, so we should (later) squash d3394dd into this commit?

I see you already mentioned this.

Yes, I noticed shortly after pushing, but didn't know if force pushing is okay if I haven't assigned anyone yet.

You did the right thing.

vext01 · 2020-05-29T10:30:04Z

ykcompile/src/lib.rs

    /// In the long-run, when we have a proper register allocator, this won't be needed.
    OutOfRegisters,
+    /// We tried to retrieve the register for a local which doesn't have a register assigned to it.
+    UnknownLocal,


Shall we put the Local inside and report it when we crash?

I just realised this isn't even used anymore, so I removed it in
61e680e.

vext01 · 2020-05-29T10:32:37Z

ykcompile/src/lib.rs

    assigned_regs: HashMap<Local, u8>,
-    /// Stores the destination locals to which we copy RAX to after leaving an inlined call.
-    leaves: Vec<Option<Place>>,
+    /// Stores the destination local of the outer most function and moves its content into RAX at


s/outer most/outermost/

Fixed in bff4293.

vext01 · 2020-05-29T10:34:40Z

ykcompile/src/lib.rs

    fn free_register(&mut self, local: &Local) {
        if let Some(reg) = self.assigned_regs.remove(local) {
+            if local == &self.rtn_var.as_ref().unwrap().local {
+                // We currently assume that we only trace a single function which leaves its return


Gross hack, but OK to get us going ;)

Indeed. It'll be a while before we won't need this workaround anymore.

Ah! In fact I need this for my non-inline call work, which -- like for inlined calls -- puts the return value in the destination register, which may not be rax.

vext01 · 2020-05-29T10:38:20Z

yktrace/src/tir.rs

-            place.clone()
+            // Replace the default return variable $0 with the variable in the outer context where
+            // the return value will end up after leaving the function. This saves us an
+            // instruction when we compile the trace. More importantly however, this guards us


I'd kill the "More importantly however, this guards us against a future optimisation..." bit and just leave it for the commit message. The future will soon catch up with us and the comment will go stale.

Fixed in 35abcb1

vext01 · 2020-05-29T10:40:33Z

yktrace/src/tir.rs


+            // Initialise VarRenamer's accumulator (and thus also set the first offset) to the
+            // traces most outer number of locals.
+            rnm.init_acc(body.num_locals);


would this be better as an argument to VarRename::new()? It's only done once?

The problem is when we create VarRename we don't have access to body yet (at least not without jumping through some hoops).

I see. Never mind then.

vext01 · 2020-05-29T10:44:01Z

yktrace/src/tir.rs

    stack: Vec<u32>,
    /// Current offset used to rename variables.
    offset: u32,
+    /// Accumulator keeping track of total amount of variables used. Needed to use different


I think this would read better as "Accumulator keeping track of the total number of variables used"

I might be wrong, but "amount" feels like it implies an uncountable quantity?

Fixed in 61cadb7

vext01 · 2020-05-29T10:45:46Z

yktrace/src/tir.rs

-        // restore it once we leave the inlined function call again.
-        self.offset += num_locals as u32;
+        // When entering an inlined function call set the offset to the current accumulator. Then
+        // increase the accumulator with the number of locals in the current function. Also add the


Minor wording tweak:
"increase the accumulator with the number of locals" -> "increment the accumulator by the number of locals"

Fixed in 61cadb7

vext01 · 2020-05-29T10:46:36Z

Just a handful of small comments from me.

ptersilie · 2020-05-29T17:38:50Z

Think I have addressed all comments.

vext01 · 2020-05-30T08:31:36Z

So UnknownLocal was in fact unused?

ptersilie · 2020-06-01T08:47:57Z

It was used in get_reg which I removed since it wasn't needed anymore. I just missed that the error was only needed for that.

vext01 · 2020-06-01T08:48:19Z

OK. Please squash.

This change replaces the default return variable `$0` with the variable in the outer context where the return value will end up after leaving the function. This saves us an instruction when we compile the trace. More importantly however, this guards us against a future optimisation in rustc that allows SIR to assign to $0 multiple times and at the beginning of a block, which could lead to another function overwriting its value (see rust-lang/rust#72205).

Currently, functions that are called consecutively (i.e. they are not nested) use the same offset to rename their variables (the offset of their outer context), which leads to them sharing the same variable names. By using an accumulator which is continuously increased to calculate the offset, we make sure consecutive functions have increasing variable names, even when after leaving a function the offset is temporarily reset to that of the outer context.

ptersilie · 2020-06-01T08:57:29Z

Squashed.

vext01 · 2020-06-01T09:09:56Z

bors r+

bors · 2020-06-01T09:14:33Z

Build succeeded:

buildbot/ykrustc-buildbot-build-script-hwt

ltratt reviewed May 28, 2020

View reviewed changes

ptersilie assigned vext01 May 28, 2020

vext01 reviewed May 29, 2020

View reviewed changes

ptersilie added 2 commits June 1, 2020 09:56

ptersilie force-pushed the local_0 branch from 61cadb7 to 9d55618 Compare June 1, 2020 08:57

bors bot merged commit 31f9aec into ykjit:master Jun 1, 2020

ptersilie deleted the local_0 branch February 26, 2025 15:10

Comments

Conversation

ptersilie commented May 28, 2020

Uh oh!

ptersilie commented May 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vext01 commented May 29, 2020

Uh oh!

ptersilie commented May 29, 2020

Uh oh!

vext01 commented May 30, 2020

Uh oh!

ptersilie commented Jun 1, 2020

Uh oh!

vext01 commented Jun 1, 2020

Uh oh!

ptersilie commented Jun 1, 2020

Uh oh!

vext01 commented Jun 1, 2020

Uh oh!

bors bot commented Jun 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants