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 optimisation of switches to lookup tables #1069

Merged
merged 5 commits into from Mar 7, 2017

Conversation

Projects
None yet
3 participants
@stedolan
Contributor

stedolan commented Feb 27, 2017

This pull request is remarkably similar to #863, but:

  • removes misguided buggy bit-twiddling
  • adds tests

@gasche gasche referenced this pull request Feb 27, 2017

Closed

Fix for GPR#863 #1068

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Feb 27, 2017

Member

Would you consider splitting the commit in two, with first the constant_closures -> cmm_constants move (which should be semantics-preserving and easier to review) and the tests, and second the optimization? (We could even be very fine, oh so very fine-grained, and have a first commit with only the ConstClosure constructor, and ConstBlock coming as a second commit.)

(It's not only about the review, which is doable from just the current patch, but also about isolating the optimization, the part that we suspect we may want to temporally revert for debugging or impact study in the future, from the generalization on which even more cool things may be built from in the future.)

Nitpick: CamlCase is not idiomatic for constructor names in this part of the codebase, we rather use separate_words.

Member

gasche commented Feb 27, 2017

Would you consider splitting the commit in two, with first the constant_closures -> cmm_constants move (which should be semantics-preserving and easier to review) and the tests, and second the optimization? (We could even be very fine, oh so very fine-grained, and have a first commit with only the ConstClosure constructor, and ConstBlock coming as a second commit.)

(It's not only about the review, which is doable from just the current patch, but also about isolating the optimization, the part that we suspect we may want to temporally revert for debugging or impact study in the future, from the generalization on which even more cool things may be built from in the future.)

Nitpick: CamlCase is not idiomatic for constructor names in this part of the codebase, we rather use separate_words.

@stedolan

This comment has been minimized.

Show comment
Hide comment
@stedolan

stedolan Feb 27, 2017

Contributor

Now split in two.

Contributor

stedolan commented Feb 27, 2017

Now split in two.

@stedolan

This comment has been minimized.

Show comment
Hide comment
@stedolan

stedolan Feb 27, 2017

Contributor

Hmmm, now that I read your middle paragraph I see you had something slightly different in mind (I thought the split was just to make code review easier).

It's now in three commits: refactor; delete the broken optimisation and add tests; add the fixed optimisation

Contributor

stedolan commented Feb 27, 2017

Hmmm, now that I read your middle paragraph I see you had something slightly different in mind (I thought the split was just to make code review easier).

It's now in three commits: refactor; delete the broken optimisation and add tests; add the fixed optimisation

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Feb 27, 2017

Member

As far as I am concerned, this is very nice, thanks. We need a review from someone that understand the backend better than I do, but then I would support merging. Could you edit the relevant Changes entry to also indicate this PR, and 1068 as well?

Member

gasche commented Feb 27, 2017

As far as I am concerned, this is very nice, thanks. We need a review from someone that understand the backend better than I do, but then I would support merging. Could you edit the relevant Changes entry to also indicate this PR, and 1068 as well?

| Cconst_natint _
| Cconst_float _
| Cconst_natint n
| Cconst_natpointer n -> (Nativeint.(to_int (logand n one) = 1))

This comment has been minimized.

@mshinwell

mshinwell Feb 28, 2017

Contributor

This was present in the previous version of this patch too, but anyway: what is the reason for these two tests on the tag bit? Even if that bit was clear, the values are still constant, so I'm not sure why they can't go in the array.

@mshinwell

mshinwell Feb 28, 2017

Contributor

This was present in the previous version of this patch too, but anyway: what is the reason for these two tests on the tag bit? Even if that bit was clear, the values are still constant, so I'm not sure why they can't go in the array.

This comment has been minimized.

@stedolan

stedolan Feb 28, 2017

Contributor

If the low bit is clear, then the array is not a valid OCaml value. I suppose it doesn't really need to be: I think we can be sure that the GC never touches it, since the address of the array never escapes.

Would it be a good idea to allow the low bit to be zero and delete the block header? Then it would be a plain lookup table, not pretending to be an OCaml heap value.

@stedolan

stedolan Feb 28, 2017

Contributor

If the low bit is clear, then the array is not a valid OCaml value. I suppose it doesn't really need to be: I think we can be sure that the GC never touches it, since the address of the array never escapes.

Would it be a good idea to allow the low bit to be zero and delete the block header? Then it would be a plain lookup table, not pretending to be an OCaml heap value.

This comment has been minimized.

@mshinwell

mshinwell Mar 2, 2017

Contributor

Allowing the bit to be zero and removing the header sounds fine to me. Or maybe change the tag to Abstract_tag instead of removing the header (might be easier)?

@mshinwell

mshinwell Mar 2, 2017

Contributor

Allowing the bit to be zero and removing the header sounds fine to me. Or maybe change the tag to Abstract_tag instead of removing the header (might be easier)?

This comment has been minimized.

@stedolan

stedolan Mar 6, 2017

Contributor

I've removed the header.

I left in the low-bit check: I'm not convinced that it's correct to allow Cload to return an int with a zero in the low bit.

@stedolan

stedolan Mar 6, 2017

Contributor

I've removed the header.

I left in the low-bit check: I'm not convinced that it's correct to allow Cload to return an int with a zero in the low bit.

This comment has been minimized.

@gasche

gasche Mar 6, 2017

Member

I left in the low-bit check: I'm not convinced that it's correct to allow Cload to return an int with a zero in the low bit.

Note that this is probably what happens in strmatch.ml, which comes from MPR#6269. But I am completely fine with no trying it here -- I'm not sure why it would or wouldn't be correct either.

@gasche

gasche Mar 6, 2017

Member

I left in the low-bit check: I'm not convinced that it's correct to allow Cload to return an int with a zero in the low bit.

Note that this is probably what happens in strmatch.ml, which comes from MPR#6269. But I am completely fine with no trying it here -- I'm not sure why it would or wouldn't be correct either.

Show outdated Hide outdated testsuite/tests/basic/switch_opts.ml
Test (3, min_int, function 1 -> 1 | 2 -> 2 | 3 -> min_int | _ -> 0);
Test (3, max_int, function 1 -> 1 | 2 -> 2 | 3 -> max_int | _ -> 0);
Test (3, 3., function 1 -> 1. | 2 -> 2. | 3 -> 3. | _ -> 0.);
Test (3, ""^"c"^"", function 1 -> "a" | 2 -> "b" | 3 -> "c" | _ -> "");

This comment has been minimized.

@mshinwell

mshinwell Feb 28, 2017

Contributor

Was the reason for avoiding the constant sharing here just one of keeping the test case self contained? I think Sys.opaque_identity should probably be used.

@mshinwell

mshinwell Feb 28, 2017

Contributor

Was the reason for avoiding the constant sharing here just one of keeping the test case self contained? I think Sys.opaque_identity should probably be used.

This comment has been minimized.

@stedolan

stedolan Feb 28, 2017

Contributor

I don't think that Sys.opaque_identity would prevent constant sharing here.

@stedolan

stedolan Feb 28, 2017

Contributor

I don't think that Sys.opaque_identity would prevent constant sharing here.

This comment has been minimized.

@mshinwell

mshinwell Mar 2, 2017

Contributor

Hmm, maybe not---although it could be used to stop Flambda evaluating the string concatenation (which it might do in the future). I'm not quite sure what to do here. How important is it that the constants aren't shared? That isn't clear to me.

@mshinwell

mshinwell Mar 2, 2017

Contributor

Hmm, maybe not---although it could be used to stop Flambda evaluating the string concatenation (which it might do in the future). I'm not quite sure what to do here. How important is it that the constants aren't shared? That isn't clear to me.

This comment has been minimized.

@stedolan

stedolan Mar 6, 2017

Contributor

It's not terribly important. I was imagining a bizarre failure mode where the table is generated but the constants are screwed up, and the test happens to pass because it doesn't actually examine the constants because they're the same object.

I've changed it to use Sys.opaque_identity

@stedolan

stedolan Mar 6, 2017

Contributor

It's not terribly important. I was imagining a bizarre failure mode where the table is generated but the constants are screwed up, and the test happens to pass because it doesn't actually examine the constants because they're the same object.

I've changed it to use Sys.opaque_identity

Show outdated Hide outdated asmcomp/cmmgen.ml
Array.to_list (Array.map (fun act ->
const_actions.(act)) cases))) in
const_actions.(act)) cases)));
addr_array_ref (Cconst_symbol table) (tag_int arg dbg) dbg

This comment has been minimized.

@mshinwell

mshinwell Feb 28, 2017

Contributor

Why is this tag_int needed?

@mshinwell

mshinwell Feb 28, 2017

Contributor

Why is this tag_int needed?

This comment has been minimized.

@stedolan

stedolan Mar 1, 2017

Contributor

The array indexing logic (Cmmgen.array_indexing) expects the index to be a tagged integer, but is good at optimising away the tagging. In practice, this generates good code.

@stedolan

stedolan Mar 1, 2017

Contributor

The array indexing logic (Cmmgen.array_indexing) expects the index to be a tagged integer, but is good at optimising away the tagging. In practice, this generates good code.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Mar 4, 2017

Member

Where are we on this? The patch as it is looks fine to me (I'm not against playing tricks with outside-the-heap values, but I'm not particularly excited either), but in any case I would like the miscompilation currently in trunk to be fixed as soon as possible, which probably means reverting the previous PR if we cannot merge this one.

Member

gasche commented Mar 4, 2017

Where are we on this? The patch as it is looks fine to me (I'm not against playing tricks with outside-the-heap values, but I'm not particularly excited either), but in any case I would like the miscompilation currently in trunk to be fixed as soon as possible, which probably means reverting the previous PR if we cannot merge this one.

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Mar 6, 2017

Contributor

@gasche This is in hand, and will be ready shortly.

Contributor

mshinwell commented Mar 6, 2017

@gasche This is in hand, and will be ready shortly.

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Mar 6, 2017

Contributor

Sorry to pick nits, but two final things:

  1. Please add the note about Cload as a comment, because it isn't obvious.
  2. How about renaming the "const block" additions in this patch to "const table" or similar? "Block" sounds like it should have a header.

OK with those changes.

Contributor

mshinwell commented Mar 6, 2017

Sorry to pick nits, but two final things:

  1. Please add the note about Cload as a comment, because it isn't obvious.
  2. How about renaming the "const block" additions in this patch to "const table" or similar? "Block" sounds like it should have a header.

OK with those changes.

@stedolan

This comment has been minimized.

Show comment
Hide comment
@stedolan

stedolan Mar 6, 2017

Contributor

Nits picked

Contributor

stedolan commented Mar 6, 2017

Nits picked

@mshinwell mshinwell added the approved label Mar 7, 2017

@mshinwell mshinwell merged commit 0d12a74 into ocaml:trunk Mar 7, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

mshinwell added a commit that referenced this pull request Mar 7, 2017

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Mar 7, 2017

Contributor

This has been cherry-picked to 4.05.

Contributor

mshinwell commented Mar 7, 2017

This has been cherry-picked to 4.05.

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

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