[PATCH] D75887: [SCEV] Add support for GEPs over scalable vectors.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen accepted this revision.
sdesmalen added a comment.
This revision is now accepted and ready to land.

LGTM!



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:3522
       // Update CurTy to its element type.
-      CurTy = cast<SequentialType>(CurTy)->getElementType();
+      if (CurTy == GEP->getType())
+        CurTy = GEP->getSourceElementType();
----------------
efriedma wrote:
> sdesmalen wrote:
> > Is this equivalent to:
> > ```
> >   auto *VecTy = dyn_cast<VectorType>(CurTy);
> >   if (VecTy && VecTy->getVectorIsScalable()) {
> >     assert (CurTy == GEP->getType());
> >     CurTy = GEP->getSourceElementType();
> >   }else
> >     CurTy = cast<SequentialType>(CurTy)->getElementType();
> > ```
> > ?
> > Not sure if that's necessarily better, but otherwise having a comment that describes the reason for handling the `CurTy == GEP->getType()` case differently would be useful.
> There isn't really anything specific to scalable vectors in this section of the patch; I just needed to rearrange the code to avoid calling ArrayType::get() on a scalable vector type.
> 
> GEP->getType() isn't a vector type at all; it's a pointer.  I guess I could use a boolean to indicate the first iteration instead, if that would be more clear?
Yes, that's indeed more clear.


================
Comment at: llvm/test/Analysis/ScalarEvolution/scalable-vector.ll:4
+; CHECK: %1 = getelementptr <vscale x 4 x i32>, <vscale x 4 x i32>* null, i32 3
+; CHECK: -->  (3 * sizeof(<vscale x 4 x i32>)) U: [0,-15) S: [-9223372036854775808,9223372036854775793)
+; CHECK: %2 = getelementptr <vscale x 1 x i64>, <vscale x 1 x i64>* %p, i32 1
----------------
efriedma wrote:
> sdesmalen wrote:
> > `vscale` is guaranteed to be >= 1, so the lower bound of the range should probably still be 48 rather than 0.
> The only data the SCEV value range algorithm is actually using as an input for SCEVUnknown values is that the bottom four bits are known zero (computed by the code in ValueTracking).  So yes, we could do better here, but that's orthogonal.
Okay, that's fine. Thanks for explaining.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75887