[PATCH] D28200: [BypassSlowDivision] Do not bypass division of hash-like values
Nikolai Bozhenov via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 24 14:14:32 PDT 2017
n.bozhenov added a comment.
In https://reviews.llvm.org/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?
I slightly updated a few tests for a newer heuristic, but generally the tests stay the same. It was intentional, yes.
> Ultimately I'm not comfortable LGTM'ing this patch so long as it contains this -O3 test. This is very different than how I understood we write tests. But that might just be my ignorance speaking; what you're trying to do may in fact be perfectly fine; I really don't know enough to say for sure. Let's get a second opinion? If @hfinkel or someone thinks this is the right approach, I'm happy.
Ok. I have removed the test in question. Probably I kind of abused the lit testing infrastructure.
More information about the llvm-commits