Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Workaround for nasty ternary closure codegen for min/max on Lua target #18

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

shakesoda
Copy link

@shakesoda shakesoda commented Dec 17, 2022

This is pretty gross to do, so I don't know if you'll want to merge it, but the codegen for ternaries on Lua target is truly terrible and I thought I'd bring it up. I think this is also the only set of functions where switching it to a Lua builtin would be relevant, everything else would just need a plain if instead of ternary.

This also affects every other usage of ternaries such as in sign, step, normalize, faceforward and reflect, I just picked on min/max first because there's a standard function for it. I don't think other targets have this codegen problem, but spitting out this many closures constantly does have gc/perf consequences on Lua.

Before:

local min = Vec3Data.new((function() 
  local _hx_1
  if (b.x < p.x) then 
  _hx_1 = b.x; else 
  _hx_1 = p.x; end
  return _hx_1
end )(), (function() 
  local _hx_2
  if (b.y < p.y) then 
  _hx_2 = b.y; else 
  _hx_2 = p.y; end
  return _hx_2
end )(), (function() 
  local _hx_3
  if (b.z < p.z) then 
  _hx_3 = b.z; else 
  _hx_3 = p.z; end
  return _hx_3
end )());

After:

local min = Vec3Data.new(_G.math.min(b.x, p.x), _G.math.min(b.y, p.y), _G.math.min(b.z, p.z));

Alternatively without the special casing:

public extern overload inline function min(b: Vec3): Vec3 {
	var mx = x;
	var my = y;
	var mz = z;
	if (b.x < x) { mx = b.x; }
	if (b.y < y) { my = b.y; }
	if (b.z < z) { mz = b.z; }
	return new Vec3(mx, my, mz);
}

outputs:

local mx = p.x;
local my = p.y;
local mz = p.z;
if (b.x < p.x) then 
  mx = b.x;
end;
if (b.y < p.y) then 
  my = b.y;
end;
if (b.z < p.z) then 
  mz = b.z;
end;
local min = Vec3Data.new(mx, my, mz);

(all results from haxe 4.2.5 using -D lua-vanilla -D analyzer-optimize -dce full

@skial skial mentioned this pull request Jan 4, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant