This is an archive of the discontinued LLVM Phabricator instance.

[LV][ARM] Tighten up MLA reduction costing
ClosedPublic

Authored by dmgreen on Jul 16 2021, 10:37 AM.

Details

Summary

This makes a couple of changes to the costing of MLA reduction patterns, to more accurately cost various patterns that can come up from vectorization.

  • The Arm implementation of getExtendedAddReductionCost is altered to only provide costs for legal or smaller types. Larger than legal types need to be split, which currently does not work very well, especially for predicated reductions where the predicate may be legal but needs to be split. Currently we limit it to legal or smaller input types.
  • The getReductionPatternCost has learnt that reduce(ext(mul(ext, ext)) is a pattern that can come up, and can be treated the same as reduce(mul(ext, ext)) providing the extension types match.
  • And it has been adjusted to not count the ext in reduce(mul(ext, ext)) as part of a reduce(mul) pattern.

Together these changes help to more accurately cost the mla reductions in cases such as where the extend types don't match or the extend opcodes are different, picking better vector factors that don't result in expanded reductions.

Diff Detail

Event Timeline

dmgreen created this revision.Jul 16 2021, 10:37 AM
dmgreen requested review of this revision.Jul 16 2021, 10:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2021, 10:37 AM
RKSimon added inline comments.Jul 20 2021, 8:29 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
7150

pre-commit these PatternMatch NFCs ?

dmgreen updated this revision to Diff 360396.Jul 21 2021, 3:36 AM
dmgreen edited the summary of this revision. (Show Details)

That does sound like a good suggestion. Split off the NFC pattern match code.

SjoerdMeijer added inline comments.Jul 28 2021, 12:51 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
7198

Do we also need to match op1?

match(Op1, m_ZExtOrSExt(m_Value())

that's what I would guess from reading the comment below.

7203

Nit: might be easier to read if this comment is just before the if.

7217

Just a quick query on the * 2, was wondering if that needs to be 3, but probably depends on my earlier question about matching op1.

dmgreen updated this revision to Diff 362299.Jul 28 2021, 1:22 AM

Thanks for taking a look. This improves the accuracy of the new double extend costing and extends the comment.

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

We check Op0->getOpcode() == Op1->getOpcode(), so do this without a matcher.

7203

The comments are all inside the ifs to be consistent with the other code here. Otherwise they need to be before the else, and I'm not sure that is very readable.

7217

It could be either way, I think, but the third extend would be of a different type. Because they are equivalent (https://alive2.llvm.org/ce/z/1dVe_y), I was just taking the simpler route and costing them as two larger extends and a multiply.

I have improved that to use the original types though. It will be good for it to be more accurate.

SjoerdMeijer accepted this revision.Jul 28 2021, 1:37 AM

Cheers, looks like a good change to me.

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

Ah, okay, missed it, but there it is.

7203

Okay, not a big fan of that style, but fair enough.

This revision is now accepted and ready to land.Jul 28 2021, 1:37 AM
This revision was landed with ongoing or failed builds.Jul 28 2021, 4:51 AM
This revision was automatically updated to reflect the committed changes.