Fix wrong register typing in float (and boxed int) unboxing by trefis · Pull Request #2083 · ocaml/ocaml

trefis · 2018-10-01T16:05:39Z

$ cat unbox_under_switch.ml
type _ t =
  | IO : int option t
  | F : float t

let bar : type a. a t -> float -> int -> a =
  fun t f i ->
    match t with
    | IO -> Some i
    | F -> f
[@@inline always]

let foo (t : float t) f i =
  let r = ref 0. in
  r := bar t f i

$ ocamlopt -c ./unbox_under_switch.ml
Fatal error: exception File "asmcomp/cmm.ml", line 83, characters 4-10: Assertion failed
Raised at file "asmcomp/cmm.ml", line 83, characters 4-16
Called from file "asmcomp/selectgen.ml", line 144, characters 13-51
Called from file "asmcomp/selectgen.ml", line 769, characters 18-42
Called from file "asmcomp/selectgen.ml", line 671, characters 18-32
Called from file "asmcomp/selectgen.ml", line 1102, characters 18-32
Called from file "asmcomp/selectgen.ml", line 1207, characters 2-16
Called from file "utils/misc.ml", line 28, characters 20-27
Re-raised at file "utils/misc.ml", line 28, characters 50-57
Called from file "asmcomp/asmgen.ml" (inlined), line 102, characters 15-18
Called from file "asmcomp/asmgen.ml", line 107, characters 2-75
Called from file "list.ml", line 106, characters 12-15
Called from file "utils/misc.ml", line 28, characters 20-27
Re-raised at file "utils/misc.ml", line 28, characters 50-57
Called from file "asmcomp/asmgen.ml" (inlined), line 102, characters 15-18
Called from file "asmcomp/asmgen.ml", line 180, characters 2-122
Called from file "asmcomp/asmgen.ml", line 155, characters 6-12
Re-raised at file "asmcomp/asmgen.ml", line 160, characters 6-15
Re-raised at file "asmcomp/asmgen.ml", line 171, characters 4-13
Called from file "driver/optcompile.ml" (inlined), line 67, characters 15-18
Called from file "driver/optcompile.ml", line 127, characters 16-303
Called from file "utils/misc.ml", line 28, characters 20-27
Re-raised at file "utils/misc.ml", line 28, characters 50-57
Called from file "driver/optcompile.ml" (inlined), line 67, characters 15-18
Called from file "driver/optcompile.ml", line 121, characters 10-626
Called from file "driver/optcompile.ml", line 139, characters 8-68
Re-raised at file "driver/optcompile.ml", line 144, characters 6-13
Called from file "utils/misc.ml", line 28, characters 20-27
Re-raised at file "utils/misc.ml", line 28, characters 50-57
Called from file "driver/compenv.ml", line 578, characters 6-35
Called from file "list.ml", line 106, characters 12-15
Called from file "driver/compenv.ml", line 654, characters 2-61
Called from file "driver/optmain.ml", line 256, characters 6-163
Re-raised at file "parsing/location.ml", line 494, characters 14-25
Re-raised at file "parsing/location.ml", line 494, characters 14-25
Re-raised at file "parsing/location.ml", line 494, characters 14-25
Re-raised at file "parsing/location.ml", line 494, characters 14-25
Re-raised at file "parsing/location.ml", line 494, characters 14-25
Re-raised at file "parsing/location.ml", line 494, characters 14-25
Called from file "parsing/location.ml" (inlined), line 499, characters 31-61
Called from file "driver/optmain.ml", line 316, characters 6-37
Called from file "driver/optmain.ml", line 320, characters 2-9

This was initially noticed when building Owl with -inline 20, which caused that call to Ctypes.(!@) to be inlined.

The bug is also present (and was noticed) on 4.07.

The fix was suggested by @lpw25.

trefis · 2018-10-01T16:08:31Z

I'd be in favor of backporting to the 4.07 branch and make an rc2.

mshinwell · 2018-10-01T16:09:26Z

Suggested by @lpw25 and yours truly.

The patch looks correct to me.

trefis · 2018-10-01T16:12:15Z

Suggested by @lpw25 and yours truly.

Apologies :)

trefis · 2018-10-02T08:49:33Z

check_typo was unhappy, I pushed a fix as well as a changelog entry; all the other tests passed.
Whoever approves should feel free to squash and merge.

gasche · 2018-10-02T08:59:32Z

I see the same header-ignoring logic in the third case of the unbox_int function. Should it be fixed as well?

mshinwell · 2018-10-02T09:12:09Z

Well spotted. I think that code is wrong as well but won't actually cause a failure at present (because the types of registers used to hold unboxed integers and arbitrary values are compatible with respect to "join" in Selectgen). However some work exists (#1192) which tightens up the register typing specifically with a view to catching this kind of error. (I would still appreciate feedback on that work.) If such change to the typing were merged, then the code in unbox_int could cause a compile-time failure, because the type of a register holding an unboxed integer (which cannot be scanned by the GC) would be incompatible with the type of a register holding an arbitrary value (corresponding to the Some case in the test case above). So we should fix unbox_int, I think.

mshinwell · 2018-10-02T09:45:48Z

I've pushed a fix for unbox_int. @lpw25 has reviewed this diff (591718e).

mshinwell · 2018-10-02T09:52:31Z

@chambart Could you please look at this?

mshinwell · 2018-10-02T09:54:09Z

@damiendoligez Do you want to backport this to 4.07? It is needed to avoid a failure when compiling Owl, so I understand.

chambart · 2018-10-04T11:01:11Z

Looks good to me. You could be a bit more aggressive in that case: It would be sensible to produce some kind of trap (like the Uunreachable case).

mshinwell · 2018-10-04T14:55:27Z

@chambart Agreed. However, I think that would involve adding cases that explicitly match the ill-typed cases, in addition to the well-typed cases that are picked out at the moment. I'm not sure if it's worth doing this. In the back of my mind I have that the next version of Flambda should have eliminated these branches (or replaced them with traps) by the time we get here. How strongly do you feel about this?

mshinwell · 2018-10-16T08:46:45Z

@chambart Ping (we should make sure this goes into 4.08)

trefis · 2018-11-02T16:18:09Z

Note: unless @chambart speaks up, I will rebase and merge this on Tuesday.

mshinwell · 2018-11-02T16:53:19Z

@trefis Pierre is away for a while. I think it's ok to merge this in its current form, and if needs be it can be improved upon later.

$ ocamlrun ocamlopt -I %{root}/runtime -nostdlib -I %{root}/stdlib -o unbox_under_switch.opt unbox_under_switch.ml Fatal error: exception File "asmcomp/cmm.ml", line 83, characters 4-10: Assertion failed Raised at file "asmcomp/cmm.ml", line 83, characters 4-16 Called from file "asmcomp/selectgen.ml", line 152, characters 13-51 Called from file "asmcomp/selectgen.ml", line 777, characters 18-42 Called from file "asmcomp/selectgen.ml", line 679, characters 18-39 Called from file "asmcomp/selectgen.ml", line 1108, characters 18-39 Called from file "asmcomp/selectgen.ml", line 1214, characters 2-35 Called from file "utils/misc.ml", line 31, characters 8-15 Re-raised at file "utils/misc.ml", line 45, characters 40-48 Called from file "asmcomp/asmgen.ml", line 106, characters 2-75 Called from file "list.ml", line 107, characters 12-15 Called from file "utils/misc.ml", line 31, characters 8-15 Re-raised at file "utils/misc.ml", line 45, characters 40-48 Called from file "asmcomp/asmgen.ml", line 176, characters 2-140 Called from file "utils/misc.ml", line 31, characters 8-15 Re-raised at file "utils/misc.ml", line 45, characters 40-48 Called from file "asmcomp/asmgen.ml", line 155, characters 7-231 Called from file "utils/misc.ml", line 31, characters 8-15 Re-raised at file "utils/misc.ml", line 45, characters 40-48 Called from file "driver/optcompile.ml", line 68, characters 7-198 Called from file "utils/misc.ml", line 31, characters 8-15 Re-raised at file "utils/misc.ml", line 45, characters 40-48 Called from file "utils/misc.ml", line 31, characters 8-15 Re-raised at file "utils/misc.ml", line 45, characters 40-48 Called from file "driver/compile_common.ml", line 123, characters 6-68 Called from file "utils/misc.ml", line 31, characters 8-15 Re-raised at file "utils/misc.ml", line 45, characters 40-48 Called from file "utils/misc.ml", line 31, characters 8-15 Re-raised at file "utils/misc.ml", line 45, characters 40-48 Called from file "driver/compenv.ml", line 588, characters 6-57 Called from file "list.ml", line 107, characters 12-15 Called from file "driver/compenv.ml", line 664, characters 2-61 Called from file "driver/optmain.ml", line 270, characters 6-163 Re-raised at file "parsing/location.ml", line 633, characters 22-25 Called from file "driver/optmain.ml", line 336, characters 6-37 Called from file "driver/optmain.ml", line 340, characters 2-9

… boxed integer cases too

mshinwell changed the title ~~Fix excessively aggressive float unboxing~~ Fix excessively aggressive float (and boxed int) unboxing Oct 2, 2018

mshinwell changed the title ~~Fix excessively aggressive float (and boxed int) unboxing~~ Fix wrong register typing in float (and boxed int) unboxing Oct 2, 2018

mshinwell added this to the 4.08 milestone Oct 16, 2018

mshinwell added the bug label Oct 16, 2018

trefis and others added 9 commits November 5, 2018 12:58

unbox_float: fix

c89e465

check typo

f8bdc45

Changes

1832f31

Fix [unbox_int] too; update Changes

34d7670

Changes

97f6721

Enhance testsuite/tests/float-unboxing/unbox_under_switch.ml to cover…

bd327f5

… boxed integer cases too

Rename test case

1396fb3

Fix testcase

fe4c3da

trefis force-pushed the unbox_float branch from 6c5f43b to fe4c3da Compare November 5, 2018 13:01

lpw25 approved these changes Nov 5, 2018

View reviewed changes

trefis merged commit d85e98a into ocaml:trunk Nov 5, 2018

trefis deleted the unbox_float branch November 5, 2018 15:44

lthls mentioned this pull request Mar 4, 2019

Don't generate Clambda constants during Cmmgen, etc. #2280

Merged

gasche mentioned this pull request Feb 22, 2020

Performance regression in OCaml 4.08 #9326

Closed

Conversation

trefis commented Oct 1, 2018

Uh oh!

trefis commented Oct 1, 2018

Uh oh!

mshinwell commented Oct 1, 2018

Uh oh!

trefis commented Oct 1, 2018

Uh oh!

trefis commented Oct 2, 2018

Uh oh!

gasche commented Oct 2, 2018

Uh oh!

mshinwell commented Oct 2, 2018

Uh oh!

mshinwell commented Oct 2, 2018

Uh oh!

mshinwell commented Oct 2, 2018

Uh oh!

mshinwell commented Oct 2, 2018

Uh oh!

chambart commented Oct 4, 2018

Uh oh!

mshinwell commented Oct 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mshinwell commented Oct 16, 2018

Uh oh!

trefis commented Nov 2, 2018

Uh oh!

mshinwell commented Nov 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mshinwell commented Oct 4, 2018 •

edited

Loading