[PATCH] D109821: [ScalarEvolution] Infer loop max trip count from array accesses

[Liren.Peng via Phabricator via llvm-commits]

Peakulorain added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:7312
+      auto *ElemType = dyn_cast<ArrayType>(Ty)->getElementType();
+      if (!ElemType || DL.getTypeAllocSize(ElemType).isScalable())
+        continue;
----------------
reames wrote:
> Its not clear to me why we care about the structure of the allocated type.  If we can get the allocated size, why do we care what the raw element type is?
> 
> OH, you've reintroduced the bug I pointed out before.  You seem to be expecting ElementType == GEPType, but you never actually check that.  Given that potential mismatch, you have a potential unit confusion.  
> 
> Please either fix the bug - if I'm correct - or add appropriate tests and asserts to make it obvious this is not possible.  
> Its not clear to me why we care about the structure of the allocated type.  If we can get the allocated size, why do we care what the raw element type is?
Not sure if I understand correctly, so for below alloca instructon:
%a = alloca [7 x i32], <ty> <NumElements>, align 4
The purpose of checks (!isa<ArrayType>(Ty) || !ArrSize || !ArrSize->isOne()) is to make sure it's allocating an array and the number (i.e, <NumElements>) of arrays is 1.  This check is restricted, other scenarios might be handled but not handled here.  

> OH, you've reintroduced the bug I pointed out before.  You seem to be expecting ElementType == GEPType, but you never actually check that.  Given that potential mismatch, you have a potential unit confusion.  

Hmm, not really if I understand your comment correctly. If we get the total size of array is 128bytes but ElementType size is 4bytes, NumElement is 32 ,  but  GEP in loop handle 8bytes in each iteration,  the max time is 16, not equal to NumElement. So, I think we do not need to care the relevance between ElementType and GEPType.




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

https://reviews.llvm.org/D109821