This is an archive of the discontinued LLVM Phabricator instance.

[SLP] Fix cost model w.r.t. operand properties
ClosedPublic

Authored by reames on Aug 24 2022, 8:12 AM.

Details

Summary

We allow the target to report different costs depending on properties of the operands; given this, we have to make sure we pass the right set of operands and account for the fact that different scalar instructions can have operands with different properties.

As a motivating example, consider a set of multiplies which each multiply by a constant (but not all the same constant). Most of the constants are power of two (but not all).

If the target doesn't have support for non-uniform constant immediates, this will likely require constant materialization and a non-uniform multiply. However, depending on the balance of target costs for constant scalar multiplies vs a single vector multiply, this might or might not be a profitable vectorization.

This ends up basically being a rewrite of the existing code. Normally, I'd scope the change more narrowly, but I kept noticing things which seemed highly suspicious, and none of the existing code appears to have any test coverage at all. I think this is a case where simply throwing out the existing code and starting from scratch is reasonable.

This is a follow on to Alexey's D126885, but also handles the arithmetic instruction case since the existing code appears to have the same problem.

Diff Detail

Event Timeline

reames created this revision.Aug 24 2022, 8:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 24 2022, 8:12 AM
reames requested review of this revision.Aug 24 2022, 8:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 24 2022, 8:12 AM
RKSimon added inline comments.Aug 24 2022, 8:17 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6569

Don't use auto (even though it was used before.....)

reames added inline comments.Aug 24 2022, 8:33 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6569

Why? This is certainly allowed by LLVM coding standards.

Check D115757, it supports same kind for all operations.

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
6569

Allowed if it is easy to determine the type, better to use the actual type where possible.

reames updated this revision to Diff 462010.Sep 21 2022, 2:41 PM

Defer to reviewer style preference.

LGTM but naturally test coverage would be a plus. Have you had any luck with your multiply by (pow2 and non-pow2) constant example?

This revision was not accepted when it landed; it landed in state Needs Review.Sep 23 2022, 8:40 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

LGTM but naturally test coverage would be a plus. Have you had any luck with your multiply by (pow2 and non-pow2) constant example?

JFYI, I'm not ignoring the request for test coverage. I tried multiple times to come up with a viable test for this, and each time, I'd stumble across an unrelated issue first. There's so many interacting problems here that finding the right combination of input to trip this difference is effectively impossible. I'm going to work through a couple of the other issues, and hopefully as I do, I can start building a reasonable test corpus.

LGTM but naturally test coverage would be a plus. Have you had any luck with your multiply by (pow2 and non-pow2) constant example?

JFYI, I'm not ignoring the request for test coverage. I tried multiple times to come up with a viable test for this, and each time, I'd stumble across an unrelated issue first. There's so many interacting problems here that finding the right combination of input to trip this difference is effectively impossible. I'm going to work through a couple of the other issues, and hopefully as I do, I can start building a reasonable test corpus.

I feel your pain! Getting everything to work as you'd expect with costs is never straightforward...