1609876 - Instruction reordering fails to hoist some final uses

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine: JIT, defect)

Description

[Andy Wingo [:wingo]]

Bug 1195545 rom 4 years ago added an instruction reordering pass to Ion. However the version that was committed doesn't correspond to the last reviewed patch; in https://bugzilla.mozilla.org/page.cgi?id=splinter.html&ignore=&bug=1195545&attachment=8650477, line 119, the code says:

            // Don't try to move instructions which aren't the last use of any
            // of their inputs (we really ought to move these down instead).
            if (lastUsedInputs.empty()) {
                iter++;
                continue;
            }

However the committed code, https://hg.mozilla.org/integration/mozilla-inbound/rev/59d2f2e62420#l1.121, does:

           // Don't try to move instructions which aren't the last use of any
           // of their inputs (we really ought to move these down instead).
           if (lastUsedInputs.length() < 2) {
               iter++;
               continue;
           }

Note there is a corresponding check lower down in the loop as well.

This change obviously prevents this optimization from applying as broadly as it seems was intended.

Comment 1

[Nicolas B. Pierron [:nbp]]

This code is aggregating a list of inputs for which the current instruction is the last use of, and does not do anything unless it is the last use of 2 of its inputs.

This actually makes sense, considering that instruction are the last use of 1 instruction and produces 1 instruction. Therefore moving this instruction would be a no-op in register allocator terms (not killing more registers than register produced). This makes sense unless there is a chain of instructions flowing into one instruction with multiple Last-used-inputs. But in a world filled with resume point, this is a safe approach.

So apart for the different code pushed and different code reviewed, this changes make sense.

Comment 2

[Andy Wingo [:wingo]]

Thanks for looking into this, the explanation makes sense!