vsariola / pakettic

TIC-80 cartridge packer
MIT License
19 stars 4 forks source link

Add a way to prevent pakettic from reordering certain expressions (was: Lua issue with table access expression reordering) #15

Open AMcBain opened 5 months ago

AMcBain commented 5 months ago

I have some code that, at least in my original TIC-80 program works perfectly but after minification (with default settings) it fails with invalid order function for sorting.

tl;dr: The problem isn't the minification per se, it's that certain table accesses if not done in one of a few specific orders results in different behavior/output.


Original code (works):

table.sort(b0,function (a,b)
  ax=(a[1].x+a[2].x+a[3].x)/3
  ay=(a[1].y+a[2].y+a[3].y)/3
  az=(a[1].z+a[2].z+a[3].z)/3
  bx=(b[1].x+b[2].x+b[3].x)/3  
  by=(b[1].y+b[2].y+b[3].y)/3
  bz=(b[1].z+b[2].z+b[3].z)/3
  if az~=bz then
   return az>bz
  end
  if ax~=bx then
   return ax<bx
  end
  return ay>by
 end)

b0 is a table that contains triangles ({{x=1.0,y=1.0,z=1.0}}) that have doubles as property values.

Minified code (fails):

table.sort(b0,function(a,b)
 ax=(a[3].x+a[2].x+a[1].x)/3
 ay=(a[3].y+a[2].y+a[1].y)/3
 az=(a[3].z+a[2].z+a[1].z)/3
 bx=(b[2].x+b[3].x+b[1].x)/3
 by=(b[2].y+b[3].y+b[1].y)/3
 bz=(b[2].z+b[1].z+b[3].z)/3
 if az~=bz then return bz<az end
 if bx~=ax then return ax<bx end
 return by<ay end
)

This has been reformatted for clarity and the variables renamed to match original, but I've tested to ensure the error still occurs. I also verified the comparison "flip" is not the cause either since it maintains the original meaning.


The Fix

 bz=(b[2].z+b[3].z+b[1].z)/3

This reorders the table access for line 6 of the sort function to match the two above it. Then it doesn't fail.

Buuuut I still don't have any d-mn idea why that fixes it because if go grab my original code and pair it with the minified code:

 ax=(a[3].x+a[2].x+a[1].x)/3
 ay=(a[3].y+a[2].y+a[1].y)/3
 az=(a[3].z+a[2].z+a[1].z)/3
 bx=(b[1].x+b[2].x+b[3].x)/3
 by=(b[1].y+b[2].y+b[3].y)/3
 bz=(b[1].z+b[2].z+b[3].z)/3

It now fails!

But this works!

 bx=(b[3].x+b[2].x+b[1].x)/3
 by=(b[3].y+b[2].y+b[1].y)/3
 bz=(b[2].z+b[3].z+b[1].z)/3

I am so confused.

This issue isn't really the minifiers fault, since I can trigger it in unminified code, but there's two-three issues that may be:

  1. The minifier doesn't seem to care if I put --{! and --} around entire table.sort block, and reorders stuff inside anyway.
  2. Even if I reorder the non-minified accesses to match the order of the initial minified output, minifying will still reorder them.
  3. I had a working minified version before moving a couple unrelated statements elsewhere in the file, so it only messes with these arbitrarily.

-- Starchaser

AMcBain commented 5 months ago

As an update, if it helps any, pakettic file.lua.tic -aanneal does appear to generate the smallest output and leaves the ordering of those alone (or at least picks an ordering that doesn't make Lua complain.

vsariola commented 5 months ago

-aanneal is probably just lucky and does not produce invalid code. I agree, this is super weird. I'll investigate. Meanwhile, can you send the whole offending code? This might have something to do with the specific tables you have. You can DM me on discord if you want more real-time debugging session.

Oh, are you on 1.3.1?

vsariola commented 5 months ago

Regarding --{!: it does not prevent expression reordering. It only prevents reordering statements, which are not normally reordered, but are only reordered within --{ block. So, --{! has no use unless used within a --{ block. There is no way preventing expression reordering; maybe there should be because one day a case will arise where reordering expressions breaks the code, but I am not convinced yet that yours is one: it makes no sense for that reordering to break the code.

AMcBain commented 5 months ago

-aanneal is probably just lucky and does not produce invalid code. I agree, this is super weird. I'll investigate. Meanwhile, can you send the whole offending code? This might have something to do with the specific tables you have. You can DM me on discord if you want more real-time debugging session.

Oh, are you on 1.3.1?

I look to be, based on my env's site packages. I only installed it a couple days ago. As to aanneal, I think so too since I also got it to bug once. Just lucky it decided that wasn't a better optimization.

Regarding --{!: it does not prevent expression reordering. It only prevents reordering statements, which are not normally reordered, but are only reordered within --{ block. So, --{! has not use unless used within a --{ block. There is no way preventing expression reordering; maybe there should be because I one day a case will arise where reordering expressions breaks the code, but I am not convinced yet that yours is one: it makes no sense for that reordering to break the code.

I did actually have a --{ block surrounding the code I tried putting --{! around, as I grouped two large sets of code separately. (Though later determined I don't need to, but strangely things packs better if I keep that arrangement).


I did later determine the the /3 isn't necessary and can be removed while still being bugged. Someone did point out this might just be a a "weird" floating point issue, though I would hope that simple addition is stable, such that a + b + c is the same as b + a + c every time it's run. Because while floating point can be a PITA (see 0.2 + 0.1), the math should always be the same.

I was able to work around this problem by a suggestion of just looping through and precaulating the added values before the sort so it's only ever run once per sort.

I'll sort out sending you a copy since I still hold out a little hope I can figure out how to get my code ≤ 1kb for something upcoming. 🙂

vsariola commented 5 months ago

a+b+c != a+c+b in general. For example: a and b can be both so small that when added to a big number c, it rounds back to c. But when a & b are first added together, it becomes just big enough number to notch c up by tiny amount. Therefore, if ax, ay, az are calculated using one set of equations and bx, by and bz calculated using another set of equations, the sorting result might depend on which element is a and which element is b. And thus, LUA correctly complains that the sorting function is invalid, because it is :) The sorting function should not depend which element is which.

One ugly idea: make one function to return ax, ay, az that is called for a and b separately. This guarantees that the values are calculated identically.

True sizecoding way: just sort by the zs of the first vertices and be done with it :D

vsariola commented 5 months ago

Well, this is the first example where pakettic reordering broke the code. Granted, we were able to find ways around it, but I'll change this into a feature request: add a way to prevent pakettic from reordering certain expressions. More magic comments are needed; I'll spend some time to think what's actually the good notation for it.