This is an archive of the discontinued LLVM Phabricator instance.

[SCEV] Add support for GEPs over scalable vectors.
ClosedPublic

Authored by efriedma on Mar 9 2020, 6:48 PM.

Details

Summary

Because we have to use a ConstantExpr at some point, the canonical form isn't set in stone, but this seems reasonable.

The pretty sizeof(<vscale x 4 x i32>) dumping is a relic of ancient LLVM; I didn't have to touch that code. :)

Diff Detail

Event Timeline

efriedma created this revision.Mar 9 2020, 6:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2020, 6:48 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
sdesmalen added inline comments.Mar 10 2020, 2:41 AM
llvm/lib/Analysis/ScalarEvolution.cpp
3504

nit: this comment is now stale.

3522

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.

llvm/test/Analysis/ScalarEvolution/scalable-vector.ll
4

vscale is guaranteed to be >= 1, so the lower bound of the range should probably still be 48 rather than 0.

efriedma marked 2 inline comments as done.Mar 10 2020, 2:51 PM
efriedma added inline comments.
llvm/lib/Analysis/ScalarEvolution.cpp
3522

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?

llvm/test/Analysis/ScalarEvolution/scalable-vector.ll
4

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.

efriedma updated this revision to Diff 249505.Mar 10 2020, 3:07 PM

Address review comments

sdesmalen accepted this revision.Mar 11 2020, 2:57 PM

LGTM!

llvm/lib/Analysis/ScalarEvolution.cpp
3522

Yes, that's indeed more clear.

llvm/test/Analysis/ScalarEvolution/scalable-vector.ll
4

Okay, that's fine. Thanks for explaining.

This revision is now accepted and ready to land.Mar 11 2020, 2:57 PM
This revision was automatically updated to reflect the committed changes.