[PATCH] D60656: [LVI][CVP] Calculate with.overflow result range

[Nikita Popov via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Analysis/LazyValueInfo.cpp:1083
+    ValueLatticeElement &BBLV, WithOverflowInst *WO, BasicBlock *BB) {
+  Optional<ConstantRange> LHSRes = getRangeForOperand(0, WO, BB);
+  Optional<ConstantRange> RHSRes = getRangeForOperand(1, WO, BB);
----------------
reames wrote:
> If I'm putting this together correctly, you don't need the filter clause from the actual binary operator version under the assumption that all of the overflowing binary operators are supported right?  If so, that probably warrants a comment.
The filtering in the binary operator code is just a performance optimization: It would still be correct without it, but of course it makes little sense to compute the input ranges, if the output is always going to be a full range due to lack of ConstantRange support. We can drop it once we support all binary operator in CR (urem in D60952, sdiv and srem are still missing).

In this case though all the possible binary operators (add, sub and mul) are supported.


================
Comment at: llvm/lib/Analysis/LazyValueInfo.cpp:1092
+  Instruction::BinaryOps BinOp = WO->getBinaryOp();
+  BBLV = ValueLatticeElement::getRange(LHSRange.binaryOp(BinOp, RHSRange));
+  return true;
----------------
reames wrote:
> This (correctly) only handles one of two outputs for the overflow inst.  Are you planning on adding support for the other?  Just curious.
I don't think that supporting the overflow result in LVI block value calculation makes a lot of sense: If the overflow value is known, then we'll want the `with.overflow` intrinsic to be optimized away entirely, which is something that CVP already does (at least for the no overflow case, it doesn't handle always overflow yet).


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

https://reviews.llvm.org/D60656