[PATCH] D155389: [ValueTracking][ScalarEvolution] improving llvm.assume's support for the argument value without context & reducing the result range of ScalarEvolution::getRange using computeConstantRange

[CaprYang via Phabricator via llvm-commits]

CaprYang added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:6868-6873
+    if (U->getType()->isIntOrIntVectorTy()) {
+      ConstantRange Range = computeConstantRange(
+          U->getValue(), SignHint == ScalarEvolution::HINT_RANGE_SIGNED, true,
+          &AC, nullptr, &DT);
+      ConservativeResult = ConservativeResult.intersectWith(Range, RangeType);
+    }
----------------
arsenm wrote:
> This is a separate change
okey, i will split it into a separate commit at later


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:103
+  const BasicBlock &B = A->getParent()->getEntryBlock();
+  return B.empty() ? nullptr : &B.front();
+}
----------------
arsenm wrote:
> An empty block isn't well formed IR, does it really need to handle that
improved


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:119
 
+  CxtI = getArgCxtI(dyn_cast<Argument>(V));
+  if (CxtI && CxtI->getParent())
----------------
arsenm wrote:
> Instead of pretending arguments are at the first instruction, could we just directly attach ranges to Arguments (by an attribute)
Use the attribute to set MD_range? Because the value range constraint may be influenced by other variable values, I think "assume" is more appropriate. Also, I think "assume" should be effective for all values, and the Arguments should not be an exception.
CtxI is set as the first instruction, because if it is effective for the first instruction, it will definitely be effective for subsequent instructions? maybe? And what would be the downside of doing this?



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

https://reviews.llvm.org/D155389