[PATCH] D28200: [BypassSlowDivision] Do not bypass division of hash-like values
Justin Lebar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sat Mar 25 13:14:58 PDT 2017
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;
More information about the llvm-commits