mlogjs icon indicating copy to clipboard operation
mlogjs copied to clipboard

Code optimization misses forward modification in loops

Open hsaturn opened this issue 1 year ago • 1 comments

Hello

(EDIT -> See smaller test case at the bottom)

This bug exists both in released and unreleased version:

byte is set to mem[idx + offset] at line 4 of the script in the while loop, the optimizer thinks that idx+offset is unchanged. But in the idx is changed inside the while loop, after the mem[idx] = mem[idx + offset]

(don't try to figure out the goal of this script, this is a reduced version of my big script)

var idx = 0;
var offset = 100;
const mem: Memory = new Memory(getBuilding("cell1"));
var byte = mem[idx + offset];
while (byte % 100) {
    var count = byte % 100;
    while (count--) {
        mem[idx] = mem[idx + offset];
        idx++;
    }
    byte = mem[idx + offset];
    idx++;
}

Generated code is

set idx:1:4 0
set offset:2:4 100
op add &t0 idx:1:4 offset:2:4    <--- HERE &t0  = idx + offset
read byte:4:4 cell1 &t0


// WHILE LOOP
  op mod &t1 byte:4:4 100
  jump 18 equal &t1 0
  set count:6:8 &t1
  set &t2 count:6:8
  op sub count:6:8 count:6:8 1
  jump 14 equal &t2 0
  read &t3 cell1 &t0                        <---- THERE THE BUG
  write &t3 cell1 idx:1:4
  op add idx:1:4 idx:1:4 1               <--------- Because here the update of idx
  jump 7 always
  op add &t3 idx:1:4 offset:2:4    <------- Idx has changed, index is re-computed
  read byte:4:4 cell1 &t3
  op add idx:1:4 idx:1:4 1
jump 4 always
end

Smaller test case

var idx = 10;
const mem: Memory = new Memory(getBuilding("cell1"));
var byte = mem[idx + 10];
while (idx < 20) {
    mem[idx] = mem[idx + 10];
    idx++;
}

Wrong at generated code for mem[idx] = mem[idx + 10] in the while loop

Best regards, Mlogjs rocks !

hsaturn avatar Mar 28 '24 01:03 hsaturn

const displayRes = getBuilding("display1").size * 32 - 16 //assume it's 176

let px = displayRes / 2 // px is cached so this means anything using "displayRes/2" will be replaced by px unless it updates
let accumulator = 0
for (let i = 0; i < displayRes / 2; i++) {
    accumulator += i + displayRes / 2 // but here it is still using px as cached! As it only checks linearly without checking if px is set elsewhere on the loop
    px = accumulator // which it is being set right here, meaning the optimisation will break.
}

this also happened with me, if it helps I'm providing another test case

YuriGen2423 avatar Aug 06 '24 23:08 YuriGen2423