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 "zero divided by n" (part of this was MPR#7201) #954

Merged
merged 1 commit into from Dec 9, 2016

Conversation

Projects
None yet
6 participants
@mshinwell
Contributor

mshinwell commented Dec 8, 2016

The Changes entry is self-explanatory.

@mshinwell mshinwell added the bug label Dec 8, 2016

@mshinwell mshinwell added this to the 4.05.0 milestone Dec 8, 2016

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Dec 8, 2016

Contributor
Contributor

shindere commented Dec 8, 2016

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 8, 2016

Member

I am deeply troubled by the fact that we still have wrong-integer-constants-optimization after the rather embarrassing k mod n scare we had last year. These are really dumb mistakes. Something is going wrong in the quality analysis process for optimizations that enables these mistakes to creep in. What do you think we could change to the patch production or review process for backend optimizations to catch them before they are committed?

One idea for example: "each backend optimizations should come with an argument, as a comment, for why that optimization is correct in all cases".

(The notion of "backend optimizations" is too broad here, I don't mean to make all middle_end work harder. I would be fine with "in cmmgen.ml" for a start.)

At the very least we should carefully review the existing optimizations to make sure there is no other such error lurking (if a good idea for a process is suggested, we could apply it to existing optimizations). @mshinwell, how did you find this one, by review or by testing?

Member

gasche commented Dec 8, 2016

I am deeply troubled by the fact that we still have wrong-integer-constants-optimization after the rather embarrassing k mod n scare we had last year. These are really dumb mistakes. Something is going wrong in the quality analysis process for optimizations that enables these mistakes to creep in. What do you think we could change to the patch production or review process for backend optimizations to catch them before they are committed?

One idea for example: "each backend optimizations should come with an argument, as a comment, for why that optimization is correct in all cases".

(The notion of "backend optimizations" is too broad here, I don't mean to make all middle_end work harder. I would be fine with "in cmmgen.ml" for a start.)

At the very least we should carefully review the existing optimizations to make sure there is no other such error lurking (if a good idea for a process is suggested, we could apply it to existing optimizations). @mshinwell, how did you find this one, by review or by testing?

@gasche gasche added the approved label Dec 8, 2016

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 8, 2016

Member

I believe that the patch is correct. Removing an optimization should not break stuff; I reviewed the remaining cases after this one to check that none was making the assumption on a (Const_int n1, _) pattern that n1 would be non-0 -- assumption that the patch breaks.

Member

gasche commented Dec 8, 2016

I believe that the patch is correct. Removing an optimization should not break stuff; I reviewed the remaining cases after this one to check that none was making the assumption on a (Const_int n1, _) pattern that n1 would be non-0 -- assumption that the patch breaks.

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 8, 2016

Contributor

I found this one whilst trawling through Mantis. I don't know how it was originally detected.

@lpw25 looked at the patch and believes it to be correct too.

@shindere : The commit message can be re-specified upon the merge itself.

Contributor

mshinwell commented Dec 8, 2016

I found this one whilst trawling through Mantis. I don't know how it was originally detected.

@lpw25 looked at the patch and believes it to be correct too.

@shindere : The commit message can be re-specified upon the merge itself.

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 8, 2016

Contributor

(I'm not sure I have any immediate observations as to how to help catch this kind of thing, but we mentioned at the dev meeting that I think @gasche , @damiendoligez and myself would try to discuss quality control with a view to then presenting some well-formed ideas to a wider group.)

Contributor

mshinwell commented Dec 8, 2016

(I'm not sure I have any immediate observations as to how to help catch this kind of thing, but we mentioned at the dev meeting that I think @gasche , @damiendoligez and myself would try to discuss quality control with a view to then presenting some well-formed ideas to a wider group.)

@mshinwell mshinwell removed the approved label Dec 8, 2016

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 8, 2016

Contributor

Actually I do have one suggestion: let's get yet another set of eyes on this before approval.

Contributor

mshinwell commented Dec 8, 2016

Actually I do have one suggestion: let's get yet another set of eyes on this before approval.

@jberdine

This comment has been minimized.

Show comment
Hide comment
@jberdine

jberdine Dec 8, 2016

jberdine commented Dec 8, 2016

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 8, 2016

Contributor

@jberdine Interesting...

Contributor

mshinwell commented Dec 8, 2016

@jberdine Interesting...

@yallop

This comment has been minimized.

Show comment
Hide comment
@yallop

yallop Dec 8, 2016

Member

[@gasche:]

I am deeply troubled by the fact that we still have wrong-integer-constants-optimization after the rather embarrassing k mod n scare we had last year.

Here's another: #956.

Member

yallop commented Dec 8, 2016

[@gasche:]

I am deeply troubled by the fact that we still have wrong-integer-constants-optimization after the rather embarrassing k mod n scare we had last year.

Here's another: #956.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 8, 2016

Member

@jberdine I think that ALIVE is very LLVM-specific in both its understanding of the IR being optimized and the bit-set reasoning it performs on the various instructions. Porting Alive or writing such a tool for OCaml IRs would be possible, but I expect it to be a non-trivial amount of work, more of the kind that I would propose as a six-month internship topic for a motivated student¹ than something we can do easily on a weekend's hacking session.

¹: one of the reasons is that it requires defining semantics for the IR being optimized, which we don't quite have right now.

Member

gasche commented Dec 8, 2016

@jberdine I think that ALIVE is very LLVM-specific in both its understanding of the IR being optimized and the bit-set reasoning it performs on the various instructions. Porting Alive or writing such a tool for OCaml IRs would be possible, but I expect it to be a non-trivial amount of work, more of the kind that I would propose as a six-month internship topic for a motivated student¹ than something we can do easily on a weekend's hacking session.

¹: one of the reasons is that it requires defining semantics for the IR being optimized, which we don't quite have right now.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Dec 8, 2016

Member

cc: @backtracking, the original bug reporter, who may have an opinion or advice on the quality-control question.

Member

gasche commented Dec 8, 2016

cc: @backtracking, the original bug reporter, who may have an opinion or advice on the quality-control question.

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Dec 9, 2016

Contributor
Contributor

shindere commented Dec 9, 2016

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Dec 9, 2016

Contributor

@shindere This is getting off-topic, but if you use "squash and merge", I thought only one message was involved...

Contributor

mshinwell commented Dec 9, 2016

@shindere This is getting off-topic, but if you use "squash and merge", I thought only one message was involved...

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Dec 9, 2016

Contributor

Reviewed and approved.

Contributor

xavierleroy commented Dec 9, 2016

Reviewed and approved.

@xavierleroy xavierleroy added the approved label Dec 9, 2016

@mshinwell mshinwell merged commit 33ca87f into ocaml:trunk Dec 9, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment