This is an archive of the discontinued LLVM Phabricator instance.

[CostModel][AArch64] Improve cost model for vector reduction intrinsics
ClosedPublic

Authored by RosieSumpter on Jun 18 2021, 7:40 AM.

Details

Summary

OR, XOR and AND entries are added to the cost table. An extra cost
is added when vector splitting occurs.

This is done to address the issue of a missed SLP vectorization
opportunity due to unreasonably high costs being attributed to the vector
Or reduction (see: https://bugs.llvm.org/show_bug.cgi?id=44593).

Diff Detail

Event Timeline

RosieSumpter created this revision.Jun 18 2021, 7:40 AM
RosieSumpter requested review of this revision.Jun 18 2021, 7:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 18 2021, 7:40 AM

Do And and Xor reductions work in the same way, with the same costs? Can we do those at the same time too?

Matt added a subscriber: Matt.Jun 19 2021, 7:22 AM
RosieSumpter retitled this revision from [CostModel][AArch64] Improve cost model for vector reduce Or intrinsic to [CostModel][AArch64] Improve cost model for vector reduction intrinsics.
RosieSumpter edited the summary of this revision. (Show Details)
  • Added XOR and AND entries to the table and their cases (which fall through to the code for OR)
  • Modified the reduce-and.ll test, and created a reduce-xor.ll test

Nice one. This work was motivated by a missed opportunity in the SLP vectoriser.
Can you separately create a reduced test case for that, so that we can first precommit that?
Then, that tests should be modified here.

SjoerdMeijer added inline comments.Jun 21 2021, 5:17 AM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1793–1797

You can say something here how this looks like for these new opcodes that we are adding.

I would also appreciate a pointer to the codegen test so that's easy to verify this. I was going to say that a cost of 15 looks high, but I was too lazy to find the codegen test. :)

1836

Nit: to reduce indentation and increase readability, can you return early here:

const auto *Entry = CostTableLookup(CostTblNoPairwise, ISD, MTy);
if (!Entry)
  break;
...
1837

If this cast can fail, we are accessing a null pointer below?

Can you separately create a reduced test case for that, so that we can first precommit that?

Sure, will do that

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1793–1797

There is a codegen test for i1 types (test/CodeGen/AArch64/vecreduce-bool.ll) but I couldn't find a test for the other vector types. These values are just from a manual check I did of the codegen (e.g. llc reduce-or.ll -mtriple=aarch64)

1837

I don't think a failed cast would make it this far since the cast calls assert() doesn't it? If it still needs checking, could do an if(ValVTy) before the following code?

The slp-or-reduction.ll test (https://reviews.llvm.org/rG2251f33bef38) is updated here and shows that the SLP vectorizer is now invoked.

Updates have been made to the XOR and AND SLP vectorization regression tests. They now show that vectorization occurs with this cost model change.

SjoerdMeijer added inline comments.Jun 22 2021, 8:32 AM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
1793–1797

In that case, would you mind adding those codegen tests (separately)?
You can then refer to that here in a comment.

1837

Ah yes, I read this as a dyn_cast, but it's a cast, so is correct.

RosieSumpter marked 2 inline comments as done.
  • Added v2i32 costs to table
  • Added v2i32 to cost model tests
  • Added comment referring to codegen tests
  • Break early if there's no table entry
SjoerdMeijer accepted this revision.Jun 24 2021, 2:00 AM

Thanks, this looks like a good improvement to me.

This revision is now accepted and ready to land.Jun 24 2021, 2:00 AM
This revision was landed with ongoing or failed builds.Jun 24 2021, 4:16 AM
This revision was automatically updated to reflect the committed changes.