This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Model interleave and deinterleave shuffles in cost model
ClosedPublic

Authored by luke on Mar 9 2023, 3:17 AM.

Details

Summary

Interleave and deinterleave shuffles are lowered by a more efficient
sequence if the element size is smaller than ELEN.

Diff Detail

Event Timeline

luke created this revision.Mar 9 2023, 3:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2023, 3:17 AM
luke requested review of this revision.Mar 9 2023, 3:18 AM
luke retitled this revision from [RISCV] Model interleave and deinterleave shuffles to [RISCV] Model interleave and deinterleave shuffles in cost model.Mar 9 2023, 4:17 AM
luke updated this revision to Diff 503726.Mar 9 2023, 4:17 AM

Don't count cost of li

luke updated this revision to Diff 503727.Mar 9 2023, 4:19 AM

Don't count cost of li

luke added inline comments.Mar 9 2023, 4:23 AM
llvm/test/Analysis/CostModel/RISCV/shuffle-interleave.ll
44

The motivation behind this patch is to use this shuffle cost for the cost of an interleaved memory access, so the shuffles in this test case are the same as the ones generated by the loop vectorizer.

Unfortunately, because these shuffles change the length of the vector, TargetTransformImpl.h returns -1 in getInstructionCost:

case Instruction::ShuffleVector: {
  auto *Shuffle = dyn_cast<ShuffleVectorInst>(U);
  // ...

  if (Shuffle->changesLength()) {
   // ...
    return CostKind == TTI::TCK_RecipThroughput ? -1 : 1;
  }

Which makes it difficult to test. Any ideas here?

luke added inline comments.Mar 9 2023, 4:24 AM
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
340

I excluded the li here since in loops it's an invariant that's hoisted out. Do we still need to be pessimistic and include it here?

luke updated this revision to Diff 503754.Mar 9 2023, 6:37 AM

Split test out to precommit test case

reames added a comment.Mar 9 2023, 8:49 AM

Very close to LGTM, please apply changes and I think we're good to go.

llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
331

I think that LT.second must be fixed if the original type was. Turn this into an assert?

337

You need to check that Mask[0] is either 0 or 1 here. (Or generalize the lowering.)

343

Creating the masks just to check them this way is a bit inefficient. I'm fine with this landing as is, but we probably need to generalize out isInterleaveMask utilities. We already have that code in the lowering, so this is at least our second copy.

Follow up change is fine.

llvm/test/Analysis/CostModel/RISCV/shuffle-interleave.ll
17

This cost is also suspiciously high. This really should be a single slideup. Yet another follow on change. :)

44

Seems like we need to make a change to generic code and TTI interface. There's no reason we shouldn't be costing length changing shuffles.

A generic means would be to model a non-length changing shuffle followed by a vector insert or extract.

This is definite a separate change.

luke marked an inline comment as not done.Mar 9 2023, 9:02 AM
luke added inline comments.
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
331

I thought so too, but <2 x i64> can get scalarized to i64 on zve32x! The zve32x test case in https://reviews.llvm.org/D145697 exercises this

343

I agree, I was hoping there would be something equivalent to isReverse()/isTransposeMask() in ShuffleVectorInst.
I was also wondering if there should also be a dedicated ShuffleKind for this like SK_[DE]INTERLEAVE.

luke added inline comments.Mar 9 2023, 9:05 AM
llvm/test/Analysis/CostModel/RISCV/shuffle-interleave.ll
17

If you stick this into llc then the shuffle actually disappears entirely.
We could try and model concatenating shuffles, but I think this is also blocked on the comment below

reames added inline comments.Mar 9 2023, 9:38 AM
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
337

This remains open. This is the only blocking item.

luke updated this revision to Diff 503820.Mar 9 2023, 9:58 AM

Check mask[0] != 0 | 1

luke updated this revision to Diff 503822.Mar 9 2023, 10:00 AM
luke marked 2 inline comments as done.

Check mask[0] != 0 | 1

luke added inline comments.Mar 9 2023, 10:01 AM
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
337

Sorry for the delay, was wrestling with arc

reames accepted this revision.Mar 9 2023, 10:13 AM

LGTM

This revision is now accepted and ready to land.Mar 9 2023, 10:13 AM