Improve compilation of short-circuit operators #1215

Merged
merged 4 commits into from Aug 15, 2017

Conversation

Projects
None yet
5 participants
Contributor

lpw25 commented Jun 28, 2017 • edited

 The compiler optimises short-circuit operations in if conditions. For example: ```let foo x y = if (x > 4 && not (x < 10)) || (y > 3 && not (y < 9)) then x else 0``` gets compiled to the following code (from `-dlinear`): ``````camlFoo__foo_1002: if x/29[%rax] <=s 9 goto L101 if x/29[%rax] >=s 21 goto L100 L101: if y/30[%rbx] <=s 7 goto L102 if y/30[%rbx] >=s 19 goto L100 L102: I/31[%rax] := 1 return R/0[%rax] L100: return R/0[%rax] `````` However, these optimisations are not applied for short-circuit operators outside of if conditions. For example: ```let bar x y = (x > 4 && not (x < 10)) || (y > 3 && not (y < 9))``` gets compiled to: ``````camlFoo__bar_1005: if x/29[%rax] <=s 9 goto L107 I/31[%rax] := x/29[%rax] =s 21 goto L104 L105: if y/30[%rbx] <=s 7 goto L106 I/31[%rax] := y/30[%rbx] >=s 19 I/32[%rax] := I/31[%rax] * 2 + 1 return R/0[%rax] L106: V/33[%rax] := 1 return R/0[%rax] L104: V/34[%rax] := 3 return R/0[%rax] `````` With flambda turned on these optimisations were broken both inside and outside of if conditions. For example, the `foo` function above would produce: ``````camlFoo__foo_7: x_10/29[%rdi] := R/0[%rax] if x_10/29[%rdi] <=s 9 goto L103 I/31[%rax] := x_10/29[%rdi] =s 19 goto L100 I/37[%rax] := 1 return R/0[%rax] L101: I/36[%rax] := 1 return R/0[%rax] L100: R/0[%rax] := x_10/29[%rdi] return R/0[%rax] `````` with this PR it produces the same code as the non-flambda compiler.

Merged

mshinwell reviewed Aug 8, 2017

 let if_then_else (cond, ifso, ifnot) = (* Approximation of the "then" and "else" continuations in [transl_if] *) type then_else_approx = | Cond (* then => true, else => false *)

mshinwell Aug 8, 2017

Contributor

I think we could just call this `Then_true_else_false`. It's not used very much and will save the comment.

mshinwell Aug 10, 2017

Contributor

Actually, I don't find the use of the word "approximation" very helpful here. Describing this directly in terms of how it affects the result of `transl_if` would probably be clearer.

mshinwell requested changes Aug 10, 2017

 let if_then_else (cond, ifso, ifnot) = (* Approximation of the "then" and "else" continuations in [transl_if] *) type then_else_approx = | Cond (* then => true, else => false *)

mshinwell Aug 10, 2017

Contributor

Actually, I don't find the use of the word "approximation" very helpful here. Describing this directly in terms of how it affects the result of `transl_if` would probably be clearer.

Contributor

mshinwell commented Aug 10, 2017

 OK after fixing the approx type and adding a Changes entry. (I don't know how it passed Travis without a Changes entry.) I will keep an eye on benchmarks once this has been merged.
Member

yallop commented Aug 10, 2017

 I don't know how it passed Travis without a Changes entry It's due to the problem described by (and fixed in) #1222.

lpw25 added some commits Jun 26, 2017

``` Improve compilation of short-circuit operators ```
``` 222d227 ```
``` Improve debug locations in conditionals ```
``` 7abb15f ```
``` Rename then_else_approx type ```
``` d8208e7 ```
``` Add Changes entry ```
``` 79d6886 ```

lpw25force-pushed the lpw25:better-short-circuit branch from `46f96bd` to `79d6886`Aug 11, 2017

Contributor Author

lpw25 commented Aug 11, 2017

 Rebasing brought in a comment from Pierre about debug info on short-circuit operators, which inspired me to make them a bit more precise. I've also renamed the approx type and added a Changes entry.
Contributor

mshinwell commented Aug 11, 2017

 @let-def Please read the diff of the most recent changes again if you could (rev 222d227 to the tip).

Contributor

mshinwell commented Aug 14, 2017

 The whole looks ok to me, but waiting for the completed second review from @let-def .
Contributor

let-def commented Aug 14, 2017

 I reviewed again. It still looks good to me.
Contributor

mshinwell commented Aug 14, 2017

 @xavierleroy We've looked at this carefully, are you ok with this being merged?
Contributor

xavierleroy commented Aug 14, 2017

 I haven't had time to review the code in details, but this sounds good, and doubly so if it also fixes a missed optimization in flambda. The one point where I need to be reassured is that simple Boolean-valued expressions such as `````` let f x = x < 3 `````` are still compiled to branchless instructions (e.g. `cmp / set / movzb` on x86) and don't get compiled by accident like `````` let f x = if x < 3 then true else false ``````
Contributor Author

lpw25 commented Aug 15, 2017

 The one point where I need to be reassured is that... Yeah, the patch keeps track of that case explicitly with the `then_else` type: https://github.com/ocaml/ocaml/pull/1215/files#diff-c3e630ae63d7483207225e1ecb5a232eR265. So that should always be handled correctly.
Contributor

mshinwell commented Aug 15, 2017

 Merging, following @lpw25 's reply to @xavierleroy .

mshinwell merged commit `fde2001` into ocaml:trunk Aug 15, 2017 2 checks passed

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Merged