This is an archive of the discontinued LLVM Phabricator instance.

[SLP][NFC] Cleanup: Separate vectorization of Inserts and CmpInsts.
ClosedPublic

Authored by vporpo on May 5 2023, 1:47 PM.

Details

Summary

This deprecates vectorizeSimpleInstructions() and replaces it with separate
functions that vectorize CmpInsts and Inserts.

Diff Detail

Event Timeline

vporpo created this revision.May 5 2023, 1:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2023, 1:47 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
vporpo requested review of this revision.May 5 2023, 1:47 PM
ABataev added inline comments.May 5 2023, 1:57 PM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
14703–14704

It does not make it simpler

14735–14736

Same

14744–14748

You have to duplicate the same code, which is more complex than before

vporpo added inline comments.May 5 2023, 2:04 PM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
14744–14748

The check for it->isTerminator() controls whether we vectorize CmpInsts. Hiding it within vectorizeSimpleInstructions is just confusing and makes it harder to read.
To avoid repeating the code we can use a lambda for this, but the separation of vectorizing Inserts and CmpInsts should be clear. Wdyt?

ABataev added inline comments.May 5 2023, 2:09 PM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
14744–14748

You can try, but check that the logic does not become more complex than before.

vporpo added inline comments.May 5 2023, 2:14 PM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
14744–14748

OK let me give it a try.

vporpo updated this revision to Diff 519973.May 5 2023, 2:28 PM

Using lambdas to avoid repetition.

ABataev added inline comments.May 8 2023, 6:35 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
14646–14648

Use dyn_cast:

if (auto *Cmp = dyn_cast<CmpInst>(I))
  return PostProcessCmps.contains(Cmp);
return isa<InsertElementInst, InsertValueInst>(I) && PostProcessInserts.contains(I);
vporpo updated this revision to Diff 520408.May 8 2023, 9:05 AM

Using dyn_cast<CmpInst>.

ABataev added inline comments.May 8 2023, 9:24 AM
llvm/include/llvm/Transforms/Vectorize/SLPVectorizer.h
147–148

Why need a template here since looks like it accepts only ArrayRef?

vporpo updated this revision to Diff 520463.May 8 2023, 12:29 PM

Using CmpInst * instead of Instruction * in the loops of vectorizeCmpInsts(). This makes it a bit more explicit that we are expecting a range of CmpInst *.

vporpo added inline comments.May 11 2023, 2:53 PM
llvm/include/llvm/Transforms/Vectorize/SLPVectorizer.h
147–148

We are passing an iterator_range and there is no ArrayRef constructor with an iterator_range argument.
I could use the actual type iterator_range<std::reverse_iterator<SmallSetVector<CmpInst *, 8>::iterator>> but it is pretty ugly and if we ever need to remove the reverse order at the call-site we would have to change this type too. Wdyt?

ABataev added inline comments.May 11 2023, 3:54 PM
llvm/include/llvm/Transforms/Vectorize/SLPVectorizer.h
147–148

SmallSetVector class, used to store instruction, has getArrayRef() member function, which can return ArrayRef.

vporpo added inline comments.May 11 2023, 4:57 PM
llvm/include/llvm/Transforms/Vectorize/SLPVectorizer.h
147–148

I don't see how getting the ArrayRef from the SmallSetVector would help. We still need to pass an iterator_range as we need to iterate in reverse.
The only way this would work is if we reverse the iteration within vectorizeCmpInsts which I don't think we should do.

ABataev added inline comments.May 12 2023, 4:35 PM
llvm/include/llvm/Transforms/Vectorize/SLPVectorizer.h
147–148

Better to avoid template. Better to pass ArrayRef and reverse it there (adding the explanation, why need to do it).

vporpo added inline comments.May 12 2023, 5:30 PM
llvm/include/llvm/Transforms/Vectorize/SLPVectorizer.h
147–148

Reversing the iteration order in vectorizeCmpInsts is not a good solution, because no one expects that the function would walk the array in reverse.

Using a range seems to be the best option here. This pattern with a template <typename RangeT> is being used in several other places throughout LLVM. I am not sure I understand why you don't like it.

ABataev added inline comments.May 12 2023, 5:58 PM
llvm/include/llvm/Transforms/Vectorize/SLPVectorizer.h
147–148

It just makes less obvous constraints and requirements for tje parameter, while ArrayRef explicitly provides it. Would be good somehow specify the expected constraints, like iterator_range<T> or something similar.

vporpo added inline comments.May 12 2023, 6:16 PM
llvm/include/llvm/Transforms/Vectorize/SLPVectorizer.h
147–148

I agree, something like iterator_range<T> makes it more explicit. Let me update the patch.

vporpo updated this revision to Diff 521854.May 12 2023, 6:20 PM

Changed RangeT to iterator_range<ItT> CmpInsts.

This revision is now accepted and ready to land.May 15 2023, 6:30 AM
This revision was landed with ongoing or failed builds.May 15 2023, 10:13 AM
This revision was automatically updated to reflect the committed changes.