Skip to content

Commit

Permalink
Remove an optimisation that's invalid since OCaml 4.14
Browse files Browse the repository at this point in the history
Since ocaml/ocaml#10681 the Lifthenelse
constructor accepts only integers as its condition. (Previously,
blocks were accepted as well and considered true)

Allowing the optimiser to assume that the argument to Lifthenelse
is an integer improves code generation in some cases. However,
generation of Lambda must change (e.g. Lifthenelse(Pisint(x),...)
instead of Lifthenelse(x, ...)).

Malfunction contained an optimisation that generated Lifthenelse
of values that may be blocks in some cases, copied from a similar
optimisation in OCaml 4.13 and below. That optimisation was removed
but the copy in Malfunction was not, and is now invalid.

This causes a miscompilation, but only in very specific circumstances
(because the assumption that Lifthenelse takes an integer is not widely
relied upon).

The fix is to delete the now-invalid optimisation, as it wasn't very
important and is now wrong.

Fixes #36.
  • Loading branch information
stedolan committed Nov 6, 2023
1 parent 3d27b33 commit cc061f9
Showing 1 changed file with 1 addition and 6 deletions.
7 changes: 1 addition & 6 deletions src/malfunction_compiler.ml
Original file line number Diff line number Diff line change
Expand Up @@ -433,12 +433,7 @@ let rec to_lambda env = function
| None, Some e | Some e, None -> e
| Some eint, Some etag ->
Lifthenelse (lprim Pisint [scr], eint, etag)) in
(match cases with
| [[`Intrange (0, 0)], ezero; _, enonzero]
| [_, enonzero; [`Intrange (0, 0)], ezero] ->
(* special case comparisons with zero *)
Lifthenelse(scr, to_lambda env enonzero, to_lambda env ezero)
| cases -> flatten [] cases)
flatten [] cases
| Mnumop1 (op, ty, e) ->
let e = to_lambda env e in
let ones32 = Const_base (Asttypes.Const_int32 (Int32.of_int (-1))) in
Expand Down

0 comments on commit cc061f9

Please sign in to comment.