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

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

Detailed code review comments below.

FTR, I talked to Sanjoy offline.  I double checked w/him because I'm not familiar enough w/SCEVs design to be confident in judging the approach itself.  He okayed the general direction, so I'm going to drive the review from a code style/implementation perspective.   Once done, I'll approve the patch.



================
Comment at: include/llvm/Analysis/ScalarEvolution.h:82
   /// ScalarEvolution's BumpPtrAllocator holds the data.
   FoldingSetNodeIDRef FastID;
 
----------------
Field alignment nit pick: If you keep it as a full 32 bit field, move it above the two 16 bit fields.  With the current layout, you end up as:
128 bit FastId
16 bit SCEVType
16 bit alignment padding
your 32 bit field
16 bit SubclassData
tail padding

i.e. you're burning 32 bits of alignment padding.


================
Comment at: include/llvm/Analysis/ScalarEvolution.h:88
 protected:
+  // Estimated complexity of this node's expression tree size.
+  unsigned ExpressionSize;
----------------
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. 


================
Comment at: include/llvm/Analysis/ScalarEvolution.h:122
 
-  explicit SCEV(const FoldingSetNodeIDRef ID, unsigned SCEVTy)
-      : FastID(ID), SCEVType(SCEVTy) {}
+  explicit SCEV(const FoldingSetNodeIDRef ID, unsigned SCEVTy, unsigned Size)
+      : FastID(ID), SCEVType(SCEVTy), ExpressionSize(Size) {}
----------------
nitpick: call the arg ExprSize or something


================
Comment at: include/llvm/Analysis/ScalarEvolution.h:153
+  // compilation time.
+  unsigned getExpressionSize() const;
+
----------------
I'd just inline the body here.


================
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) {}
 
----------------
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.


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

https://reviews.llvm.org/D35989