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

[Justin Lebar via Phabricator via llvm-commits]

jlebar requested changes to this revision.
jlebar added a comment.
This revision now requires changes to proceed.

> 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?

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

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



================
Comment at: lib/Transforms/Utils/BypassSlowDivision.cpp:230
+    // it means that we couldn't find any value that doesn't look hash-like.
+    if (Visited.find(I) != Visited.end())
+      return true;
----------------
if (!Visited.insert(I).second)


https://reviews.llvm.org/D28200