-
Notifications
You must be signed in to change notification settings - Fork 29
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
if+return optimizations #209
Conversation
@@ -249,7 +249,7 @@ let private simplifyExpr (didInline: bool ref) env = function | |||
let sub1 = FunCall(Op "-", [x; a]) | |||
let sub2 = FunCall(Op "-", [b; a]) | |||
let div = FunCall(Op "/", [sub1; sub2]) |> mapExpr env | |||
FunCall(Var (Ident "smoothstep"), [Float (0.M,""); Float (1.M,""); div]) | |||
FunCall(Var (Ident "smoothstep"), [Float (0.M,""); Float (1.M,""); div]) |
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.
revert :)
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.
❔
src/rewriter.fs
Outdated
| Block b when hasNoDecl b -> b // inline inner empty blocks without variable | ||
| Decl _ as d -> [Block [d]] // a decl must stay isolated in a block, for the same reason | ||
| s -> [s] | ||
List.Cons (If (cond, bodyT, None), bodyF) |
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.
If (cond, bodyT, None) :: bodyF
| If (cond, bodyT, Some bodyF) when endsWithReturn bodyT -> | ||
let bodyF = match bodyF with | ||
| Block b when hasNoDecl b -> b // inline inner empty blocks without variable | ||
| Decl _ as d -> [Block [d]] // a decl must stay isolated in a block, for the same reason |
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.
This shouldn't happen, the variable would be unused.
(I would just remove the line)
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.
Not if its initialization has side effects
// if(a)return b;return c; -> return a?b:c; | ||
let rec replaceIfReturnsByReturnTernary = function | ||
| If (cond, Jump(JumpKeyword.Return, Some retT), None) :: Jump(JumpKeyword.Return, Some retF) :: c -> | ||
[Jump(JumpKeyword.Return, Some (FunCall(Op "?:", [cond; retT; retF])))] |
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.
Optimization idea:
if (c) return a;
x=y;
return z;
could be turned into:
return c?a:x=y,z;
Note that the transformation should work better after calling groupDeclarations
.
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.
Ah, yes, looks like this isn't already working, because we are so conservative on when to use commas, because of compressed size again I suppose.
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 mean, if we were using squeezeBlockWithComma everywhere, rather than only when it saves some braces, it would already work.
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.
You'd need to run squeezeBlockWithComma
on a subset of the block. You can do it, but it will make the minified output even less readable (unless you also update the Printer).
# Conflicts: # tests/compression_results.log # tests/unit/inline-aggro.aggro.expected # tests/unit/inline-aggro.expected
let b = removeUselessElseAfterReturn b | ||
|
||
// if(a)return b;return c; -> return a?b:c; | ||
let rec replaceIfReturnsByReturnTernary = function |
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.
replaceIfReturnsWithReturnTernary
replace... with
Needs tests, README update, and preferably a better solution for subtle optimization ordering issues.