[PATCH] D35989: [SCEV][NFC] Introduces expression sizes estimation

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev marked 7 inline comments as done.
mkazantsev added inline comments.


================
Comment at: include/llvm/Analysis/ScalarEvolution.h:88
 protected:
+  // Estimated complexity of this node's expression tree size.
+  unsigned ExpressionSize;
----------------
reames wrote:
> Does this need to be a full unsigned?  From the threshold used in https://reviews.llvm.org/D35990, it looks like we could get away w/an uint16_t if we used saturating addition. 
> 
> Note: I'm not requiring this change.  I'm throwing out an idea for consideration on how to reduce memory usage at the cost of slightly higher computation cost.  I'm leaving it to the patch author to decide which approach is better. 
Saturating adds are OK as long as our threshold fits in 16 bits.


================
Comment at: include/llvm/Analysis/ScalarEvolutionExpressions.h:419
                 ScalarEvolution *se, SCEVUnknown *next) :
-      SCEV(ID, scUnknown), CallbackVH(V), SE(se), Next(next) {}
+      SCEV(ID, scUnknown, 0), CallbackVH(V), SE(se), Next(next) {}
 
----------------
reames wrote:
> sanjoy wrote:
> > Like `SCEVConstant` this should probably be `1` as well?  Same for `SCEVCouldNotCompute`?  They're both leaf nodes.
> > 
> > If it needs to be `0` then please add a comment why.
> I agree w/Sanjoy here.
There's no such reason, it was supposed to be 1. Thanks!


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

https://reviews.llvm.org/D35989