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

Fix for GPR#863 #1068

Closed
wants to merge 1 commit into
base: 4.05
from

Conversation

Projects
None yet
3 participants
@mshinwell
Contributor

mshinwell commented Feb 27, 2017

The code for compiling switches to array lookups in #863 is wrong when the constants on the right-hand side of the switch are negative "const int" or "const pointer". The top bit gets lost.

@mshinwell mshinwell added this to the 4.05.0 milestone Feb 27, 2017

@stedolan

This comment has been minimized.

Show comment
Hide comment
@stedolan

stedolan Feb 27, 2017

Contributor

Wow, that's awful. It's not specific to polymorphic variants, either:

let test2 = function
  | 1 -> 1
  | 2 -> 2
  | 3 -> -2
  | _ -> assert false

let () = Printf.printf "%d\n" (test2 3)

The fix looks OK to me but for obvious reasons you should find someone else to check it!

Contributor

stedolan commented Feb 27, 2017

Wow, that's awful. It's not specific to polymorphic variants, either:

let test2 = function
  | 1 -> 1
  | 2 -> 2
  | 3 -> -2
  | _ -> assert false

let () = Printf.printf "%d\n" (test2 3)

The fix looks OK to me but for obvious reasons you should find someone else to check it!

@stedolan

This comment has been minimized.

Show comment
Hide comment
@stedolan

stedolan Feb 27, 2017

Contributor

The fix looks OK to me but for obvious reasons you should find someone else to check it!

Revising that: this fix doesn't work either. This example:

let test2 = function
  | 1 -> 1
  | 2 -> 2
  | 3 -> min_int
  | _ -> assert false

gives nondeterministic (!) results for test2 3, because when min_int is passed, a boxed nativeint is generated.

Contributor

stedolan commented Feb 27, 2017

The fix looks OK to me but for obvious reasons you should find someone else to check it!

Revising that: this fix doesn't work either. This example:

let test2 = function
  | 1 -> 1
  | 2 -> 2
  | 3 -> min_int
  | _ -> assert false

gives nondeterministic (!) results for test2 3, because when min_int is passed, a boxed nativeint is generated.

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Feb 27, 2017

Contributor

I am looking at this again

Contributor

mshinwell commented Feb 27, 2017

I am looking at this again

@stedolan

This comment has been minimized.

Show comment
Hide comment
@stedolan

stedolan Feb 27, 2017

Contributor

I think the patch needs at least this:

diff --git a/asmcomp/cmmgen.ml b/asmcomp/cmmgen.ml
index d3763de..2d8d6cd 100644
--- a/asmcomp/cmmgen.ml
+++ b/asmcomp/cmmgen.ml
@@ -1358,7 +1358,7 @@ let make_switch arg cases actions dbg =
       | Cconst_int n -> Uconst_int (n asr 1)
       | Cconst_pointer n -> Uconst_ptr (n asr 1)
       | Cconst_symbol s -> Uconst_ref (s, None)
-      | Cconst_natint n -> const (Uconst_nativeint n)
+      | Cconst_natint n -> const (Uconst_nativeint (Nativeint.shift_right n 1))
       | Cconst_float f -> const (Uconst_float f)
       | _ -> assert false in
     let const_actions = Array.map to_uconst actions in

Turning a Cconst back into a Uconst is clearly trickier than I had imagined. I did it this way because Cmmgen.emit_structured_constant accepts a Clambda.ustructured_constant. It might be cleaner to introduce Cmm.structured_constant, and to separate compiling Clambda.ustructured_constant and emitting Cmm.structured_constant into two phases. That way, this optimisation can generate a Cmm.structured_constant directly, rather than trying to go against the flow of compilation like it currently does.

However, that's a larger patch, and fixing this issue shouldn't wait for it.

Contributor

stedolan commented Feb 27, 2017

I think the patch needs at least this:

diff --git a/asmcomp/cmmgen.ml b/asmcomp/cmmgen.ml
index d3763de..2d8d6cd 100644
--- a/asmcomp/cmmgen.ml
+++ b/asmcomp/cmmgen.ml
@@ -1358,7 +1358,7 @@ let make_switch arg cases actions dbg =
       | Cconst_int n -> Uconst_int (n asr 1)
       | Cconst_pointer n -> Uconst_ptr (n asr 1)
       | Cconst_symbol s -> Uconst_ref (s, None)
-      | Cconst_natint n -> const (Uconst_nativeint n)
+      | Cconst_natint n -> const (Uconst_nativeint (Nativeint.shift_right n 1))
       | Cconst_float f -> const (Uconst_float f)
       | _ -> assert false in
     let const_actions = Array.map to_uconst actions in

Turning a Cconst back into a Uconst is clearly trickier than I had imagined. I did it this way because Cmmgen.emit_structured_constant accepts a Clambda.ustructured_constant. It might be cleaner to introduce Cmm.structured_constant, and to separate compiling Clambda.ustructured_constant and emitting Cmm.structured_constant into two phases. That way, this optimisation can generate a Cmm.structured_constant directly, rather than trying to go against the flow of compilation like it currently does.

However, that's a larger patch, and fixing this issue shouldn't wait for it.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Feb 27, 2017

Member

Well, we could also revert the change from 4.05 (the fact that neither of us can easily propose a fix that we feel good about may be an indication that it is not release-ready yet), which gives more time to find a serendipitous fix for 4.06.

Member

gasche commented Feb 27, 2017

Well, we could also revert the change from 4.05 (the fact that neither of us can easily propose a fix that we feel good about may be an indication that it is not release-ready yet), which gives more time to find a serendipitous fix for 4.06.

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Feb 27, 2017

Contributor

@stedolan Actually, it looks like Cconst_natint is nothing to do with Uconst_nativeint. (There is a similar Cconst_natpointer as well...)

A way of doing this that should be safer is to collect in a global ref the constants that are generated during the Cmmgen translation itself and then emit them together with the constants brought through from Compilenv. This would mean the array could be described directly in Cmm. I think such a patch is OK for 4.05.

Contributor

mshinwell commented Feb 27, 2017

@stedolan Actually, it looks like Cconst_natint is nothing to do with Uconst_nativeint. (There is a similar Cconst_natpointer as well...)

A way of doing this that should be safer is to collect in a global ref the constants that are generated during the Cmmgen translation itself and then emit them together with the constants brought through from Compilenv. This would mean the array could be described directly in Cmm. I think such a patch is OK for 4.05.

@stedolan

This comment has been minimized.

Show comment
Hide comment
@stedolan

stedolan Feb 27, 2017

Contributor

A way of doing this that should be safer is to collect in a global ref the constants that are generated during the Cmmgen translation itself and then emit them together with the constants brought through from Compilenv. This would mean the array could be described directly in Cmm. I think such a patch is OK for 4.05.

Right, this optimisation should produce a Cmm.phrase. I think it was a mistake to attempt to make a Clambda.ustructured_constant out of Cmm data. I don't have any terribly strong opinion about whether it should be done for 4.05.

Contributor

stedolan commented Feb 27, 2017

A way of doing this that should be safer is to collect in a global ref the constants that are generated during the Cmmgen translation itself and then emit them together with the constants brought through from Compilenv. This would mean the array could be described directly in Cmm. I think such a patch is OK for 4.05.

Right, this optimisation should produce a Cmm.phrase. I think it was a mistake to attempt to make a Clambda.ustructured_constant out of Cmm data. I don't have any terribly strong opinion about whether it should be done for 4.05.

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Feb 27, 2017

Contributor

Let's try to do it for 4.05. There is some support for having this feature, and I don't think this fix should involve that much code. (However it should involve some test cases...)

Contributor

mshinwell commented Feb 27, 2017

Let's try to do it for 4.05. There is some support for having this feature, and I don't think this fix should involve that much code. (However it should involve some test cases...)

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Feb 27, 2017

Member

As far as I can tell, this is now subsumed by #1069.

Member

gasche commented Feb 27, 2017

As far as I can tell, this is now subsumed by #1069.

@gasche gasche closed this Feb 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment