Page MenuHomePhabricator

[POC][LoopVectorizer] Propagate ElementCount to interfaces in preparation for scalable auto-vec.
AbandonedPublic

Authored by sdesmalen on Oct 28 2020, 2:09 PM.

Details

Reviewers
rengolin
ctetreau
Summary

This patch is part of a proof of concept for vectorising a loop using
scalable vectors. The patch is shared for reference and there is no
expectation for this patch to land in the current form.

Some of the changes include:

  • Change VFRange to use ElementCount for Start, End.
  • Change buildVPlans to take an ElementCount
  • Change buildVPlansWithVPRecipes to take an ElementCount
  • Change LoopVectorizationCostModel::computeMaxVF to take an ElementCount.
  • Change LoopVectorizationCostModel::computeFeasibleMaxVF to return an ElementCount.

This patch is NFC for fixed-width vectors.

Diff Detail

Event Timeline

sdesmalen created this revision.Oct 28 2020, 2:09 PM
Herald added a project: Restricted Project. · View Herald Transcript
sdesmalen requested review of this revision.Oct 28 2020, 2:09 PM
khchen added a subscriber: khchen.Oct 28 2020, 5:40 PM
vkmr added a comment.EditedOct 29 2020, 6:13 AM

Overall, definitely a good starting point. I just have a couple of minor comments.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
5401

Missing words? Typo? (... as we this...)

7846

Do we need this assert here? Wouldn't it be redundant with the assert in the VFRange constructor?

This seems reasonable to me.

llvm/include/llvm/Support/TypeSize.h
352

I feel like a function called "inc" should mutate the thing being incremented. Morally, this is behaves like operator+=.

All uses currently of this function would be fine with these semantics with some small changes (that should probably be done anyways)

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
7202–7209

MaxVF.inc(1) can be moved outside the loop

7661–7662

I'm pretty sure this inc can be moved outside also

dancgr added a subscriber: dancgr.Nov 3 2020, 9:56 AM
sdesmalen marked 4 inline comments as done.Nov 3 2020, 1:42 PM
sdesmalen added inline comments.
llvm/include/llvm/Support/TypeSize.h
352

You're right that inc suggests the object itself is modified. However, to my opinion that results in some odd looking code, like:

MaxVF.inc(1); // Implicitly modifies MaxVF
for (ElementCount VF = MinVF; ElementCount::isKnownLT(VF, MaxVF); ) { ... }

I've pulled this change out into a separate patch, D90713, where I've added add and sub methods that return a new object.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
7846

We don't, good spot!

hiraditya added inline comments.Nov 3 2020, 2:07 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
2769

?

7006

formating...

hiraditya added a subscriber: Ayal.Nov 3 2020, 2:08 PM
bmahjour removed a subscriber: bmahjour.Nov 3 2020, 2:09 PM
ctetreau added inline comments.Nov 5 2020, 2:16 PM
llvm/include/llvm/Support/TypeSize.h
352

your other patch has been approved

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
7202–7209

I still think this change is worth making, even given the fact that inc() has not been made to mutate the object.

7661–7662

still worth doing IMO

sdesmalen marked an inline comment as done.Nov 5 2020, 3:15 PM

FYI - This patch has been split up into D90713, D90715, D90879 and D90880.

llvm/include/llvm/Support/TypeSize.h
352

Thanks!

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
7006

Thanks, this is addressed in D90879.

7202–7209

This has been fixed in D90715. Sorry, I just realised that I forgot you as a reviewer to that patch.

7661–7662

Also fixed in D90715.

It looks like all the split-out patches have been merged. Is there anything in this patch that is still outstanding?

sdesmalen abandoned this revision.Dec 1 2020, 4:33 AM

It looks like all the split-out patches have been merged. Is there anything in this patch that is still outstanding?

No, you're right, this patch has been covered by all other patches. They haven't all landed yet because of some dependences, but this POC patch can be closed. Thanks!