Seems that it has been just fixed by rL314424 :-)
Abandoning this review.

committed r310359

If the patch is accepted, it should also be merged into 5.0, I believe.

In D28200#710710, @jlebar wrote:

Actually, the comment at VALRNG_LONG declaration says that it is not a promise.

Ah, you're right. I thought that the code that did "no division is needed at all: The quotient is 0 and the remainder is equal to Dividend." checked VALRNG_LONG, but it doesn't.

Regardless of what the comments say, I think this reveals that it is very confusing that we have VALRNG_SHORT, which is a promise, and VALRNG_LONG, which is not.

Maybe we should rename them to VALRNG_KNOWN_SHORT and VALRNG_LIKELY_LONG?

Done.

In D28200#692591, @jlebar wrote:

I believe that the new version of the heuristic is much more robust than the previous one. However, a naive implementation didn't work initially. It turned out that hash values weren't recognized because of PHI nodes with undef incoming values generated by loop-unroll. I had to amend the patch to work around the problem.

It doesn't look like you added any "unit" tests to cover this changed behavior. Was that intentional?

And since the set of middle-end optimizations and their parameters are target-specific, I would like to have this test X86-specific. It is not an automatically generated test, it has only one CHECK statement, and therefore it is not a problem to have it as an llc test.

If we we really wanted to run the full pipeline, I don't see why we couldn't simply run opt -O3 | opt -codegenprepare. Can you think of a specific bug in this pass that would not be caught by this that would be caught by the lit test?

Changed the PHI heuristic as planned.

That wouldn't make much sense. The recognized patterns are already checked with simpler tests I have put into bypass-slow-div-special-cases.ll. The purpose of this particular file is to make sure that the patterns we recognize are indeed the patterns produced by the middle-end at the maximal optimization level (I have removed the explicit unroll-count option).

It sounds like there's an implicit "on this particular testcase" at the end of this sentence. "We are checking that we recognize the patterns produced by the middle-end at -O3 *on this particular testcase*."

Superseded by D29897.

Moved most of the tests into CodeGenPrepare/NVPTX and changed RUN line in bypass-slow-division-fnv.ll.

Ok. I see your point. Indeed, for example, there's no need to run the whole X86 code generator to check that a division bypassing is disabled if one of the operands doesn't fit into BypassType. So I have moved the tests introduced by this patch into Transforms/CodeGenPrepare/NVPTX/bypass-slow-div-special-cases.ll. However, I still believe that we should keep a few existing tests in CodeGen/X86/bypass-slow-division-64.ll to check that when bypassing fires, it produces a good X86 code.

That's indeed somewhat frustrating... As I understand, now you all believe it's better to drop all the tests from test/CodeGen/X86/bypass-slow-division-32.ll and test/CodeGen/X86/bypass-slow-division-64.ll and replace them with a few target-independent tests.

The patch has been rebased onto D29897.

Well, it might make sense to write opt tests here... But we already have got 3 files with 18 llc tests for division bypassing. Moreover, while working on this patchset, the tests were thoroughly reviewed, it was insisted that the test should be written this way, I had to make a number of commits specifically to modify the tests. So, I don't think it would be a good idea to throw away all the work that has been done and re-write the tests from scratch now.

As for tests, it was discussed in D28196 that it's better to have them that detailed. I agree with you that they are overly fragile and check for a lot of actually insignificant details. But on the other hand, this approach also has a number of advantages: tests indeed guarantee that the compiler generates correct code for the testcase, the tests are easy to create and update, diffs in tests induced by patches are explicit and easy to review.

In D29896#681722, @jlebar wrote:

lgtm with comments updated. \o/

In D29897#681767, @jlebar wrote:

Do you think it would be too conservative for us to say: We won't optimize a long div if you do a short div with the same operands anywhere in the same function?

In D29897#681222, @jlebar wrote:

When we see x/y, in addition to checking whether x and y are known "long", should we also check whether x | y is known-long? This way if we saw something like:

if (x | y & 0xffffffff00000000) return x / y;
else return static_cast<int32_t>(x) / static_cast<int32_t>(y);

we wouldn't re-optimize this code.

Yet another rebase.

Rebased onto a newer version of D29896.