-
-
Notifications
You must be signed in to change notification settings - Fork 670
Fix local confusion in builtin min/max/rotl/rotr #1540
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
Conversation
@@ -1484,7 +1485,7 @@ function builtin_max(ctx: BuiltinContext): ExpressionRef { | |||
if (op != -1) { | |||
let flow = compiler.currentFlow; | |||
let nativeType = type.toNativeType(); | |||
let temp1 = flow.getTempLocal(type); | |||
let temp1 = flow.getTempLocal(type, findUsedLocals(arg1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about make getUnusedLocal
which try to find unused local in already allocated locals or allocate new one otherwise? And use flow.getUnusedLocal(type)
here and in other places?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
findUsedLocals
is what other code typically uses to keep the number of necessary locals low. Otherwise we'd always get a new local, never a reused one, I guess?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also builtin_abs
, builtin_rotl
/ builtin_rotr
I guess also required this fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, at some point I expect us to move away from free'ing temporary locals, but we'll have to get rid of any kind of recompiling expressions first. There are still a few places doing this, and getUnusedLocal
would return a new local upon recompile, yielding different code, while findUsedLocals
doesn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also builtin_abs, builtin_rotl / builtin_rotr I guess also required this fix
Abs is a unary expression not executing input code in between assignment and usage of the temporary locals. Should be safe. Going to take a look at rotl/rotr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
findUsedLocals is what other code typically uses to keep the number of necessary locals low. Otherwise we'd always get a new local, never a reused one, I guess?
I guess better always alloc new loacal and let binaryen optimize all of this afterwards
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expect us to move into that direction, yeah, but we cannot do that as long as we are recompiling code (and not resetting the local pool with a new mechanism) because it would confuse local flags (adding new locals on each recompile, leaving earlier ones unused), then for example leading to infinite loops when attempting to unify local flags in loops.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems that rotl
for some reason does not exhibit the behavior on rotl(a, rotl(b, c))
, but should be susceptible just as rotr
is, which trivially breaks without the fix. Probably coincidence. Applied the same fix for both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then for example leading to infinite loops when attempting to unify local flags in loops.
I see. Ok, in this case PR LGTM
The emulated min/max builtins for integer types compile to a
select
expression with two temporary locals(select (t1 = arg1) (t2 = arg2) (t1 > t2) )
where
arg2
may itself utilize a temporary local, and sincearg2
is compiled before the temporary locals for theselect
are obtained, it may have used the same then-free'd local, in turn overwriting the value oft1
when executingarg2
, ultimately breaking the condition executed last as reported.This PR makes sure that
t1
is not a local used inarg2
.Fixes #1537