[compiler-v2] Use livevar analysis to compute copy/move instructions

[wrwg]

Live variables is a standard compiler construction data flow analysis. It computes the set of variables which are alive (being used in subsequent reachable code) before and after each program point. The implementation from the prover uses our dataflow analysis framework and is here.

In this PR we create the first processing step in the bytecode pipeline for the V2 compiler with the new module pipeline/livevar_analysis_step.rs which forwards logical work to the existing analysis. This is then used primarily to compute copy/move information correctly which is a pre-requisite for ability and borrow safety analysis.

[brmataptos]

Also: Not sure what to do with this, but the BTreeMap initialization in livevar_analysis.rs (outside of this review) is pretty pessimal, as it is sequential and will rebalance the tree on every insert. Apparently if you create a BTreeMap directly from an iterator it saves the elements in a vector, sorts it (hopefully efficiently if it's already sorted), then creates the BTree more efficiently. I don't have numbers, but maybe be careful about anti-performance patterns like that.

[rahxephon89]

LGTM, thanks!

[brmataptos]

Why are you testing is_alive_before here?

Also, It seems that you only need to save a pinned local if it has been changed on the stack. But abstract_push_result() already flushes a pinned temp.

We may need some targeted test cases that make this situation easier to see.

[wrwg]

When abstract_flush_stack is called, we are interested in the pre state. That is because the flushing will be essentially inserted before the next instruction. This is different to when we use is_alive_after above: here we are interested in what is alive after this given program point. FWIW this has been discovered by testing; before I would use the after-set.

Regards your 2nd point: There are temps on the stack which may in fact have never been stored into any local (as this is a stack machine -- in the ideal case we need to store as little as possible). Those need to be saved here. You are right we could optimize the case if the temp on the stack is loaded from a non-pinned local. I suggest we keep this optimization for later. Not really need the most optimal code here -- doing the liveness here so we can do meaningful ability and borrow analysis, which require the right placement of copy and move as pre-requisite.

Testing: please note this is quite extensively tested via the transactional tests here, which you do not see as part of this change. In fact I debugged quite a bit based on those tests.

[wrwg]

Regards the abstract stack, there is now also a section in the design doc: https://www.notion.so/aptoslabs/Aptos-Move-Compiler-V2-High-Level-Design-6ab5e5ffcd574c0693795e6fdefc1331?pvs=4#fc7f5dc9114a4a6097eeb75cdc2d34ff

[brmataptos]

I think you should generate tight code based on the information you have available here, rather than being sloppy and assuming it will be fixed later. We have the liveness results, let's use them effectively.

I'm afraid that abstract_flush_stack() is saving temps from the stack into locals in too many cases. It might help to add comments for the pre/post conditions for a few of these shared operations, since the state of the function generator is a bit inconsistent as it is called in a few places at different points in generating code for a given input bytecode.

The function is defined here:

aptos-core/third_party/move/move-compiler-v2/src/file_format_generator/function_generator.rs

Line 672 in 47803cd

fn abstract_flush_stack(&mut self, ctx: &BytecodeContext, top: usize) {

.
and the overly-generous test is here:

aptos-core/third_party/move/move-compiler-v2/src/file_format_generator/function_generator.rs

Line 676 in 47803cd

if ctx.is_alive_before(temp) || ctx.is_alive_after(temp) || self.pinned.contains(&temp)

(1) is_alive_before/after: See function_generator.rs:152-156 (your branch) where we see:

                self.gen_bytecode(&bytecode_ctx, &bytecode[i], Some(next_bc));
                if !bc.is_branch() && matches!(next_bc, Bytecode::Label(..)) {
                    // At block boundaries without a preceding branch, need to flush stack
                    // TODO: to avoid this, we should use the CFG for code generation.
                    self.abstract_flush_stack(&bytecode_ctx, 0);

So in this particular case, after the bytecode is generated, we carefully store temps including those which are alive before the generated bytecode (even if not live after the bytecode). In other cases (e.g., in abstract_push_args()) the flush is presumably happening before the current bytecode. There's also a call in abstract_push_result() which also seems to happen after the bytecode is generated.

The current code test on line 676 (if ctx.is_alive_before(temp) || ctx.is_alive_after(temp) || self.pinned.contains(&temp)) is conservative, and safe, in that it saves a superset of the required temps that works for both cases. But it would be better to differentiate the 2 cases (before and after the bytecode) so that spurious temp saves are not done.

(2) is_pinned: The current code seems to ignore the lifetime of a pinned variable and consider it pinned for the rest of the method. Fine, though it would be good to mention it somewhere. Even with this conservative assumption, a pinned temp only needs to be saved if it has not already been copied to its home local.

There are only 2 locations where stack.push() is called. (i) In abstract_push_args(), the temp is saved to a local before being pushed on the stack. This case could presumably happen when pushing args for a Borrow operation, in which case the value would not yet have been saved, and should probably be saved to its home location immediately rather than waiting for abstract_flush_stack() to do it.

(ii) In abstract_push_result(), a temp is pushed, and if it is pinned, then it is saved right away, so there is no need to copy it later when abstract_flush_stack() is called and pops that temp.

So the is_pinned case is redundant in case (ii), and in case (i) it could be saved immediately.

(3) I don't see bytecode output for the transactional tests, so they aren't as useful to understand what is happening.

[wrwg]

I think you should generate tight code based on the information you have available here, rather than being sloppy and assuming it will be fixed later. We have the liveness results, let's use them effectively.

Thanks for calling my work sloppy :-) But I disagree. You and I have discussed multiple times that the goal is to not implement optimizations at this phase. Its also like this in the engineering plan. Optimizations are in milestone 4. There is a lot more optimizations you can imagine than this single one you are suggesting.

Technically, keeping track of what value on the stack is identical to a value in a local, may require us to do additional data flow analysis. Not 100% sure but perhaps in different paths, this information can be different. This would make this PR significantly larger and require a larger rewrite.

I'm afraid that abstract_flush_stack() is saving temps from the stack into locals in too many cases. It might help to add comments for the pre/post conditions for a few of these shared operations, since the state of the function generator is a bit inconsistent as it is called in a few places at different points in generating code for a given input bytecode.

I adopted this and introduced two different functions: flush_stack_after and flush_stack_before.

The function is defined here:

aptos-core/third_party/move/move-compiler-v2/src/file_format_generator/function_generator.rs

Line 672 in 47803cd

fn abstract_flush_stack(&mut self, ctx: &BytecodeContext, top: usize) {

.
and the overly-generous test is here:

aptos-core/third_party/move/move-compiler-v2/src/file_format_generator/function_generator.rs

Line 676 in 47803cd

if ctx.is_alive_before(temp) || ctx.is_alive_after(temp) || self.pinned.contains(&temp)

(1) is_alive_before/after: See function_generator.rs:152-156 (your branch) where we see:

                self.gen_bytecode(&bytecode_ctx, &bytecode[i], Some(next_bc));
                if !bc.is_branch() && matches!(next_bc, Bytecode::Label(..)) {
                    // At block boundaries without a preceding branch, need to flush stack
                    // TODO: to avoid this, we should use the CFG for code generation.
                    self.abstract_flush_stack(&bytecode_ctx, 0);

So in this particular case, after the bytecode is generated, we carefully store temps including those which are alive before the generated bytecode (even if not live after the bytecode). In other cases (e.g., in abstract_push_args()) the flush is presumably happening before the current bytecode. There's also a call in abstract_push_result() which also seems to happen after the bytecode is generated.

The current code test on line 676 (if ctx.is_alive_before(temp) || ctx.is_alive_after(temp) || self.pinned.contains(&temp)) is conservative, and safe, in that it saves a superset of the required temps that works for both cases. But it would be better to differentiate the 2 cases (before and after the bytecode) so that spurious temp saves are not done.

(2) is_pinned: The current code seems to ignore the lifetime of a pinned variable and consider it pinned for the rest of the method. Fine, though it would be good to mention it somewhere. Even with this conservative assumption, a pinned temp only needs to be saved if it has not already been copied to its home local.

One needs borrow analysis to reason about liveness of pinned variables. The borrow analysis maintains a borrow graph which has an edge from the pinned variable to the temp which contains the reference if it is still alive. But this happens in a future analysis which we have not written yet.

There are only 2 locations where stack.push() is called. (i) In abstract_push_args(), the temp is saved to a local before being pushed on the stack. This case could presumably happen when pushing args for a Borrow operation, in which case the value would not yet have been saved, and should probably be saved to its home location immediately rather than waiting for abstract_flush_stack() to do it.

(ii) In abstract_push_result(), a temp is pushed, and if it is pinned, then it is saved right away, so there is no need to copy it later when abstract_flush_stack() is called and pops that temp.

So the is_pinned case is redundant in case (ii), and in case (i) it could be saved immediately.

Can you please check the link to the design doc I provided that we are on the same page what the actual goals are here. For (i) the goal is to not save to local and postpone or avoid the need for that. That's the major motivation for the whole logic here. Regards the (ii) case, this is not redundant because of (i).

(3) I don't see bytecode output for the transactional tests, so they aren't as useful to understand what is happening.

These are transactional tests which compare execution results of the both compilers. There was a PR out with this which multiple folks reviewed. The other tests which show bytecode are also there, and you should see the results.

[brmataptos]

Btw, since the current code is safe, I'd be ok with accepting this PR if we add an issue to go back and clarify the "stack temp needs to be stored in local" property in a latter PR.

[brmataptos]

Ok.

[brmataptos]

Looks ok.

[github-actions]

✅ Forge suite compat success on aptos-node-v1.5.1 ==> d9162abb363059693f7a1ad00935c4bb94591b9a

Compatibility test results for aptos-node-v1.5.1 ==> d9162abb363059693f7a1ad00935c4bb94591b9a (PR)
1. Check liveness of validators at old version: aptos-node-v1.5.1
compatibility::simple-validator-upgrade::liveness-check : committed: 3591 txn/s, latency: 7082 ms, (p50: 7100 ms, p90: 9900 ms, p99: 10800 ms), latency samples: 168800
2. Upgrading first Validator to new version: d9162abb363059693f7a1ad00935c4bb94591b9a
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1785 txn/s, latency: 16363 ms, (p50: 18800 ms, p90: 22400 ms, p99: 22600 ms), latency samples: 92820
3. Upgrading rest of first batch to new version: d9162abb363059693f7a1ad00935c4bb94591b9a
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1851 txn/s, latency: 15592 ms, (p50: 18600 ms, p90: 21900 ms, p99: 22200 ms), latency samples: 92560
4. upgrading second batch to new version: d9162abb363059693f7a1ad00935c4bb94591b9a
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3359 txn/s, latency: 9334 ms, (p50: 9900 ms, p90: 13500 ms, p99: 13900 ms), latency samples: 137740
5. check swarm health
Compatibility test for aptos-node-v1.5.1 ==> d9162abb363059693f7a1ad00935c4bb94591b9a passed
Test Ok

• Grafana dashboard
• Humio Logs
• Test runner output
• Test run is land-blocking

[github-actions]

✅ Forge suite realistic_env_max_load success on d9162abb363059693f7a1ad00935c4bb94591b9a

two traffics test: inner traffic : committed: 6651 txn/s, latency: 5889 ms, (p50: 5700 ms, p90: 7200 ms, p99: 10400 ms), latency samples: 2879920
two traffics test : committed: 100 txn/s, latency: 2951 ms, (p50: 2900 ms, p90: 3600 ms, p99: 4300 ms), latency samples: 1820
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.220, avg: 0.210", "QsPosToProposal: max: 0.156, avg: 0.139", "ConsensusProposalToOrdered: max: 0.568, avg: 0.549", "ConsensusOrderedToCommit: max: 0.515, avg: 0.498", "ConsensusProposalToCommit: max: 1.078, avg: 1.047"]
Max round gap was 1 [limit 4] at version 1511208. Max no progress secs was 3.728107 [limit 10] at version 1511208.
Test Ok

• Grafana dashboard
• Humio Logs
• Test runner output
• Test run is land-blocking

[github-actions]

✅ Forge suite framework_upgrade success on aptos-node-v1.5.1 ==> d9162abb363059693f7a1ad00935c4bb94591b9a

Compatibility test results for aptos-node-v1.5.1 ==> d9162abb363059693f7a1ad00935c4bb94591b9a (PR)
Upgrade the nodes to version: d9162abb363059693f7a1ad00935c4bb94591b9a
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 4497 txn/s, latency: 7014 ms, (p50: 6200 ms, p90: 9900 ms, p99: 18400 ms), latency samples: 175400
5. check swarm health
Compatibility test for aptos-node-v1.5.1 ==> d9162abb363059693f7a1ad00935c4bb94591b9a passed
Test Ok

• Grafana dashboard
• Humio Logs
• Test runner output
• Test run is land-blocking