[PATCH] D28200: [BypassSlowDivision] Do not bypass division of hash-like values

[Nikolai Bozhenov via Phabricator via llvm-commits]

n.bozhenov added a comment.

In https://reviews.llvm.org/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.

>> I slightly updated a few tests for a newer heuristic, but generally the tests stay the same. It was intentional, yes.
> 
> I can't approve a patch if its tests don't cover its behavior, sorry.  If the behavior was worth changing between versions of the patch, it needs test coverage.

I have added one test case which is handled differently by the new and old versions of the patch.

>> As we stop traversal of the IR after finding first value that is neither PHINode nor MUL/XOR, the number of traversed instructions is small for any non-malicious IR. And the recursion depth is limited by the length of the longest chain of consecutive PHINodes.
> 
> It's a design goal of LLVM to handle even pathological IR.  I think this applies here.  I cannot approve this patch if it blows up on IR with many consecutive phi nodes, sorry.

I have limited the number of visited PHINodes for a single division. Now no more than 16 PHINodes are traversed before giving up.


https://reviews.llvm.org/D28200