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?

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.

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.

(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.)

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

@jberdine Interesting...

[@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.

@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.

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

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

Reviewed and approved.