-
-
Notifications
You must be signed in to change notification settings - Fork 679
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
| 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
selectexpression with two temporary locals(select (t1 = arg1) (t2 = arg2) (t1 > t2) )where
arg2may itself utilize a temporary local, and sincearg2is compiled before the temporary locals for theselectare obtained, it may have used the same then-free'd local, in turn overwriting the value oft1when executingarg2, ultimately breaking the condition executed last as reported.This PR makes sure that
t1is not a local used inarg2.Fixes #1537