This is an archive of the discontinued LLVM Phabricator instance.

[BypassSlowDivision] Handle division by constant numerators properly.
ClosedPublic

Authored by jlebar on Nov 15 2016, 2:04 PM.

Details

Summary

We don't do BypassSlowDivision when the denominator is a constant, but
we do do it when the numerator is a constant.

This patch makes two related changes to BypassSlowDivision when the
numerator is a constant:

  • If the numerator is too large to fit into the bypass width, don't bypass slow division (because we'll never run the smaller-width code).
  • If we bypass slow division where the numerator is a constant, don't OR together the numerator and denominator when determining whether both operands fit within the bypass width. We need to check only the denominator.

Event Timeline

jlebar updated this revision to Diff 78074.Nov 15 2016, 2:04 PM
jlebar retitled this revision from to [BypassSlowDivision] Handle division by constant numerators properly..
jlebar updated this object.
jlebar added a reviewer: tra.
jlebar added a subscriber: llvm-commits.
tra added inline comments.Nov 15 2016, 2:23 PM
llvm/lib/Transforms/Utils/BypassSlowDivision.cpp
86–87

This condition appears to be tautological.
AFAICT, it's equivalent to just if(isa<ConstantInt>(Divisor)).

160

This does not parse.
Perhaps it was meant to be "We ensured above" or "We bailed out above if divisor *was* constant".

163

Given that we do bail out on all constant divisors, this assert seems to be a bit redundant.

jlebar added inline comments.Nov 15 2016, 2:30 PM
llvm/lib/Transforms/Utils/BypassSlowDivision.cpp
86–87

lol, if (A || (A && B)).

I will fix this in a separate patch.

160

Fixed, thanks.

163

Well, yes, that's why the assertion is safe? :) The intent is to ensure that the behavior above doesn't change without the logic down here also changing.

jlebar updated this revision to Diff 78077.Nov 15 2016, 2:31 PM

Fix comment

tra accepted this revision.Nov 15 2016, 2:50 PM
tra edited edge metadata.

Comment nit. OK otherwise.

llvm/lib/Transforms/Utils/BypassSlowDivision.cpp
160

Now it parses, but still does not seem to match the code.
We bail out if divisor *is* constant.

This revision is now accepted and ready to land.Nov 15 2016, 2:50 PM
This revision was automatically updated to reflect the committed changes.