[PATCH] D60483: [InstCombine] Handle usubo always overflow

[Nikita Popov via Phabricator via llvm-commits]

nikic marked 2 inline comments as done.
nikic added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3940
 
 bool InstCombiner::OptimizeOverflowCheck(OverflowCheckFlavor OCF, Value *LHS,
                                          Value *RHS, Instruction &OrigI,
----------------
lebedev.ri wrote:
> Hm, i should check if this can be used (or is already used?) for some of the implicit-conversion checks that aren't using overflow intrinsics, but plain ir..
Apart from `with.overflow` handling this function is used in one more place: https://github.com/llvm-mirror/llvm/blob/29ea2070bd62816674f48ac392a1ee24e5c915cf/lib/Transforms/InstCombine/InstCombineCompares.cpp#L5049-L5060 Not sure if that's the usage you have in mind.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3993
     if (OCF == OCF_UNSIGNED_SUB) {
-      if (willNotOverflowUnsignedSub(LHS, RHS, OrigI))
         return SetResult(Builder.CreateNUWSub(LHS, RHS), Builder.getFalse(),
----------------
lebedev.ri wrote:
> And `willNotOverflowUnsignedSub()` is: (unless i'm looking at the wrong func?)
> ```
>    bool willNotOverflowUnsignedSub(const Value *LHS, const Value *RHS,
>                                    const Instruction &CxtI) const {
>      return computeOverflowForUnsignedSub(LHS, RHS, &CxtI) ==
>             OverflowResult::NeverOverflows;
>    }
> ```
> So this should ideally be:
> patch 0 (nfc)
> ```
> - if (willNotOverflowUnsignedSub(LHS, RHS, OrigI))
> + OverflowResult OR = computeOverflowForUnsignedSub(LHS, RHS, &OrigI);
> + if (OR == OverflowResult::NeverOverflows)
> ```
> patch 1 - 
> ```
> + if (OR == OverflowResult::AlwaysOverflows)
> +   return SetResult(Builder.CreateSub(LHS, RHS), Builder.getTrue(), true);
> ```
I've split out the willNot -> computeOverflow change (for all cases) into rL358051.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60483/new/

https://reviews.llvm.org/D60483