[Feature Request][compiler-v2] code improvement in reference_safety_processor (left over from PR#11511)

[brmataptos]

🚀 Feature Request

https://github.com/aptos-labs/aptos-core/pull/11511#discussion_r1435674650

[third_party/move/move-compiler-v2/src/pipeline/reference_safety_processor.rs](https://github.com/aptos-labs/aptos-core/pull/11511/files#diff-6418ac4ded15bbda344f23e45aacc0f0d858c0db6ef336204628f55d56e9a9ed)
        for released in alive.before.keys() {
            if !after_set.contains(released) {
                state.release_local(*released, &after_set)
        let mut release_cands = alive.before.keys().cloned().collect::<BTreeSet<_>>();

I think maybe this code can be simplified, but maybe we're still missing a case. The original loop can be simplified as follows, since the dests set should not overlap with the alive_before set:

for released in alive.before.keys().iter().chain(instr.dests().iter()) { But could the dests set overlap with the alive_before set?

Consider that if local x is both a parameter and a dest for a Bytecode, then it can be alive before, (optionally) alive after, and also assigned at this point. Is it possible that the same temporary can be both an operand and a destination of a Bytecode, such as either of the following 2 instructions:

      Assign(id, x, x, Inferred);
      Call(id, x, ..., [...x ]);

If x is alive after the instruction, then we may decide that the operand must be made a Copy. But then the instruction itself will overwrite the input. Are we handling (or forbidding) these cases?

Member Author @wrwg wrwg 19 hours ago Actually one can have <x is alive>; x := Call(x); <x is dead>, that is why we need the set union here. If x is alive after the call (or used twice as an argument) then copy inference will have introduced a copy. Its unfortunate that we can't have SSA because of the reference semantics, it would make things clearer, but its not necessarilly needed. Livevar analysis in general works fine with x := Call(x), which is the situation where x is both used and defined in the instruction. In this case, any usage of x after instruction is for the x we are defining here, whereas the argument to the call is the last use of the 'previous' x and thus will be moved into the call. So yes, I believe we are fine.

Member @brmataptos brmataptos 1 hour ago • Well, if you need to consider overlap, it will still be more efficient to avoid copying the alive.before.keys set:

for released in alive.before.keys().iter().chain(instr.dests().iter().filter(|x| !alive.before.contains_key(x)) {

or perhaps more readably, using iter_set::union:

let dests_set: BTreeSet<_> = instr.dests().into_iter().collect();
for released in iter_set::union(alive.before_keys().iter(), dests_set.iter()) {

which allocates more memory (but just for the dests set) but is more readable and might even be more efficient.