[llvm] 8d48766 - [CVP] Soften SDiv into a UDiv as long as we know domains of both of the operands.

[Roman Lebedev via llvm-commits]

Terribly sorry for that, fix pushed.

On Sat, Jul 18, 2020 at 7:04 PM Florian Hahn <florian_hahn at apple.com> wrote:
>
>
>
> > On Jul 18, 2020, at 16:00, Roman Lebedev via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> >
> >
> > Author: Roman Lebedev
> > Date: 2020-07-18T17:59:56+03:00
> > New Revision: 8d487668d09fb0e4e54f36207f07c1480ffabbfd
> >
> > URL: https://github.com/llvm/llvm-project/commit/8d487668d09fb0e4e54f36207f07c1480ffabbfd
> > DIFF: https://github.com/llvm/llvm-project/commit/8d487668d09fb0e4e54f36207f07c1480ffabbfd.diff
> >
> > LOG: [CVP] Soften SDiv into a UDiv as long as we know domains of both of the operands.
> >
> > Yes, if operands are non-positive this comes at the extra cost
> > of two extra negations. But  a. division is already just
> > ridiculously costly, two more subtractions can't hurt much :)
> > and  b. we have better/more analyzes/folds for an unsigned division,
> > we could end up narrowing it's bitwidth, converting it to lshr, etc.
> >
> > This is essentially a take two on 0fdcca07ad2c0bdc2cdd40ba638109926f4f513b,
> > which didn't fix the potential regression i was seeing,
> > because ValueTracking's computeKnownBits() doesn't make use
> > of dominating conditions in it's analysis.
> > While i could teach it that, this seems like the more general fix.
> >
> > This big hammer actually does catch said potential regression.
> >
> > Over vanilla test-suite + RawSpeed + darktable
> > (10M IR instrs, 1M IR BB, 1M X86 ASM instrs), this fires/converts 5 more
> > (+2%) SDiv's, the total instruction count at the end of middle-end pipeline
> > is only +6, so out of +10 extra negations, ~half are folded away,
> > and asm instr count is only +1, so practically speaking all extra
> > negations are folded away and are therefore free.
> > Sadly, all these new UDiv's remained, none folded away.
> > But there are two less basic blocks.
> >
> > https://rise4fun.com/Alive/VS6
> >
> > Name: v0
> > Pre: C0 >= 0 && C1 >= 0
> > %r = sdiv i8 C0, C1
> >  =>
> > %r = udiv i8 C0, C1
> >
> > Name: v1
> > Pre: C0 <= 0 && C1 >= 0
> > %r = sdiv i8 C0, C1
> >  =>
> > %t0 = udiv i8 -C0, C1
> > %r = sub i8 0, %t0
> >
> > Name: v2
> > Pre: C0 >= 0 && C1 <= 0
> > %r = sdiv i8 C0, C1
> >  =>
> > %t0 = udiv i8 C0, -C1
> > %r = sub i8 0, %t0
> >
> > Name: v3
> > Pre: C0 <= 0 && C1 <= 0
> > %r = sdiv i8 C0, C1
> >  =>
> > %r = udiv i8 -C0, -C1
> >
> > Added:
> >
> >
> > Modified:
> >    llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
> >    llvm/test/Transforms/CorrelatedValuePropagation/sdiv.ll
> >
> > Removed:
> >
> >
> >
> > ################################################################################
> > diff  --git a/llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp b/llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
> > index 284312eaf822..fb7a005708e5 100644
> > --- a/llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
> > +++ b/llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
> > @@ -607,6 +607,12 @@ static bool isNonNegative(Value *V, LazyValueInfo *LVI, Instruction *CxtI) {
> >   return Result == LazyValueInfo::True;
> > }
> >
> > +static bool isNonPositive(Value *V, LazyValueInfo *LVI, Instruction *CxtI) {
> > +  Constant *Zero = ConstantInt::get(V->getType(), 0);
> > +  auto Result = LVI->getPredicateAt(ICmpInst::ICMP_SLE, V, Zero, CxtI);
> > +  return Result == LazyValueInfo::True;
> > +}
> > +
> > static bool allOperandsAreNonNegative(BinaryOperator *SDI, LazyValueInfo *LVI) {
> >   return all_of(SDI->operands(),
> >                 [&](Value *Op) { return isNonNegative(Op, LVI, SDI); });
> > @@ -672,24 +678,65 @@ static bool processSRem(BinaryOperator *SDI, LazyValueInfo *LVI) {
> > }
> >
> > /// See if LazyValueInfo's ability to exploit edge conditions or range
> > -/// information is sufficient to prove the both operands of this SDiv are
> > -/// positive.  If this is the case, replace the SDiv with a UDiv. Even for local
> > +/// information is sufficient to prove the signs of both operands of this SDiv.
> > +/// If this is the case, replace the SDiv with a UDiv. Even for local
> > /// conditions, this can sometimes prove conditions instcombine can't by
> > /// exploiting range information.
> > static bool processSDiv(BinaryOperator *SDI, LazyValueInfo *LVI) {
> > -  if (SDI->getType()->isVectorTy() || !allOperandsAreNonNegative(SDI, LVI))
> > +  if (SDI->getType()->isVectorTy())
> >     return false;
> >
> > +  enum class Domain { NonNegative, NonPositive, Unknown };
> > +  auto getDomain = [&](Value *V) {
> > +    if (isNonNegative(V, LVI, SDI))
> > +      return Domain::NonNegative;
> > +    if (isNonPositive(V, LVI, SDI))
> > +      return Domain::NonPositive;
> > +    return Domain::Unknown;
> > +  };
> > +
> > +  struct Operand {
> > +    Value *V;
> > +    Domain Domain;
> > +  };
> > +  std::array<Operand, 2> Ops;
> > +  for (const auto &I : zip(Ops, SDI->operands())) {
>
> It looks like this is causing the following warning:
>
> lvm-project/llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp:703:20: warning: loop variable 'I' is always a copy because the range of type 'detail::zippy<detail::zip_shortest, array<Operand, 2> &, iterator_range<Use *> >' does not return a reference [-Wrange-loop-analysis]
>   for (const auto &I : zip(Ops, SDI->operands())) {
>