This is an archive of the discontinued LLVM Phabricator instance.

[LV] Support vectorizing 'select index of minimum element' idiom. (WIP)
AbandonedPublic

Authored by fhahn on Aug 17 2022, 12:39 PM.

Details

Reviewers
ABataev
Mel-Chen

Diff Detail

Event Timeline

fhahn created this revision.Aug 17 2022, 12:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2022, 12:39 PM
fhahn requested review of this revision.Aug 17 2022, 12:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2022, 12:39 PM
fhahn updated this revision to Diff 495077.Feb 6 2023, 4:14 AM

Rebase and fix crash with VF=1 IC>1 (test added in 40621ff4b8a6214924)

ABataev added inline comments.Feb 6 2023, 6:25 AM
llvm/include/llvm/Analysis/IVDescriptors.h
130

Pass by reference?

llvm/lib/Analysis/IVDescriptors.cpp
835

Better to name it something like get..., since it returns instruction?

843–845

Just return Phi->getIncomingValueForBlock(TheLoop->getLoopLatch()) == UMinU;

861

get?

883–894
auto *P0 = dyn_cast<PHINode>(CmpOp0);
auto *P1 = dyn_cast<PHINode>(CmpOp1);
auto *P = P0 ? P0 : P1;
if (!P)
  return nullptr;
if ((UMin = isMinIdxReductionMinVal(P, TheLoop, SE))) {
   MinPhi = P;
   CurVal = P == P0 ? CmpOp1 : CmpOp0;
}
1029

The condition is same as in previous if. Is this ok?

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

Remove the new line

4000–4001

State.Plan?

4011–4012

break?

8678

Delete new line

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
645–647

No need for braces

llvm/test/Transforms/LoopVectorize/select-min-index.ll
8

Generate the checks by the script?

Mel-Chen added inline comments.Feb 6 2023, 11:53 PM
llvm/include/llvm/Analysis/IVDescriptors.h
55–57

Interesting. I have different point. In my opinion, we should be able to reuse min/max recurrence. This prevents adding Select[Min|Max]Idx for each kind of min/max. I will share my current implementation later.

130

I use pointer too. This is because function isReductionPHI may not pass argument SE.

llvm/lib/Analysis/IVDescriptors.cpp
912–918

Canonical induction variable? It seems that there is a existing function that can be called directly, but I forgot the function name...

1224–1225

I also encountered the same problem here. I think MMI should base on full SelectCmp support. Using -1 may cause problems with unsigned index. But full support for SelectCmp is another long long story.

llvm/test/Transforms/LoopVectorize/select-min-index.ll
26–28

Your test case is single-exit. This is TODO for me. I will add a case that returns both umin and index.

https://reviews.llvm.org/D143465
Our views are a bit different, so I share our implementation with you. At present, the comments and commit log are not perfect, but I will make up.

fhahn abandoned this revision.Aug 3 2023, 12:52 AM

Abandoning since there are more recent patches making great progress in that direction and I never had time to wrap this up