This is an archive of the discontinued LLVM Phabricator instance.

[SLP] Function for instruction cost calculation, NFC.
AbandonedPublic

Authored by ABataev on Mar 6 2017, 6:29 AM.

Details

Summary

Added function BoUpSLP::getCost() to calculate the scalar or vector
cost of the specified list of instructions (expected scalar
instructions) or vector - scalar cost estimation.

Event Timeline

ABataev created this revision.Mar 6 2017, 6:29 AM
mkuper added inline comments.Mar 6 2017, 11:49 AM
lib/Transforms/Vectorize/SLPVectorizer.cpp
420

This seems like a weird interface, for a couple of reasons:

  1. It's somewhat vague - I'd expect different APIs to get a cost and to get the diffs.
  2. It returns an Optional, but none of the uses actually check the returned value is valid. And when would you expect to return None anyway? Shouldn't we recognize any instruction here? If not, then do we expect to assert, or just silently decide the instruction will be scalarized?
  3. Only the Call case actually wants the scalar and the vector costs separately, and I don't really understand why - it ends up subtracting them.

Splitting out a function that takes the vector and scalar type, and returns the difference, instead of having the subtraction repeated in every case, doesn't sound like a bad idea. Can you explain why this is an improvement over that, though?

1847–1848

Isn't the point of getCostOrDiff that you should be able to pass the vector and the scalar types, and get the difference? Why are you using two calls and then subtracting?

4895

This looks unrelated.

ABataev added inline comments.Mar 6 2017, 11:46 PM
lib/Transforms/Vectorize/SLPVectorizer.cpp
420

Michael, I reworked it. It is required for another path I'm going to publish today.

1847–1848

I reworked this code

4895

Yes, part of another patch, will be removed

ABataev updated this revision to Diff 90802.Mar 7 2017, 12:01 AM

Address Michael's comments

ABataev updated this revision to Diff 90812.Mar 7 2017, 1:08 AM

Updated to latest revision

mkuper edited edge metadata.Mar 7 2017, 11:17 AM

This makes sense. Some minor comments inline.

lib/Transforms/Vectorize/SLPVectorizer.cpp
420

This still doesn't explain under what conditions it makes sense to return a None cost.
Does it mean "we don't know"? Something else?

1850

Any reason the shuffle case can't live with the rest in getCost()?
(The other special cases here are really special, but this seems pretty standard - it doesn't seem to use any extra information, only the VL)

1884

Why not put the llvm_unreachable here? I think it'd be clearer.

ABataev abandoned this revision.Apr 14 2017, 11:48 AM