Page MenuHomePhabricator

[Support] Migrate more high level cost functions to using InstructionCost
AbandonedPublic

Authored by david-arm on Nov 23 2020, 4:54 AM.

Details

Summary

This patch changes the following TargetTransformInfo and BasicTTIIMpl
interfaces to return a InstructionCost class:

getUserCost
getInstructionLatency
getInstructionThroughput

As a result of changing getUserCost I have also had to fix up various
places where it is called. In places where it's obvious that we should
never encounter an invalid cost I have added an assert and dereferenced
the value. In other places where trivial I have simply added isValid()
checks and bailed out of an optimisation.

See this patch for the introduction of the type: https://reviews.llvm.org/D91174
See this thread for context: http://lists.llvm.org/pipermail/llvm-dev/2020-November/146408.html

Diff Detail

Event Timeline

david-arm created this revision.Nov 23 2020, 4:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2020, 4:54 AM
david-arm requested review of this revision.Nov 23 2020, 4:54 AM
david-arm updated this revision to Diff 307541.Nov 25 2020, 1:51 AM
david-arm edited the summary of this revision. (Show Details)
  • Rebased off parent patch.
  • Changed calls to isInvalid to !isValid instead, since I feel this is more future-proof in case additional states are added to the InstructionCost class.
david-arm updated this revision to Diff 308020.Nov 27 2020, 4:26 AM
david-arm added a subscriber: paulwalker-arm.
  • After some private discussions with @paulwalker-arm I think that we shouldn't rely upon the comparison operators treating invalid costs as infinite. Instead it's better to explicitly check for validity before comparing.

Thank you @david-arm for the patch.
It helps to understand a bit more how to use the new class InstructionCost in D91174
I hope you don't mind but I have a couple of questions.

llvm/lib/Transforms/Scalar/LoopUnswitch.cpp
306 ↗(On Diff #308020)

This assumes that Metrics.NumInsts.getValue(), should we check if Metrics.NumInsts.isValid()?

llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
2602

Why we do not use InstructionCost in this function when computing the Instruction costs?
You've only replaced in one place, but not others.

llvm/lib/Transforms/Scalar/SpeculateAroundPHIs.cpp
506

The behaviour of SpecCost invalid is the same if SpecCost is lower than CostSaving?
Should LLVM_DEBUG inform that the costs are invalid?

david-arm added inline comments.Nov 30 2020, 4:46 AM
llvm/lib/Transforms/Scalar/LoopUnswitch.cpp
306 ↗(On Diff #308020)

Yeah I could have used isValid() here too, but getValue() returns an Optional<int>, which cannot be used directly anyway. I thought it was cleaner to write it this way and only reference Metric.NumInsts once. The alternative would be to write:

if (Metrics.NumInsts.isValid())

Props.SizeEstimation = *(Metrics.NumInsts.getValue());

else

Props.SizeEstimation = std::numeric_limits<unsigned>::max();

Both ways of writing are functionally correct though! I've tended to use isValid() in places where we don't need to extract the value, i.e. for asserts, before doing comparisons or to bail out early from an optimisation.

llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
2602

This patch only changes code that's relevant to TTI.getUserCost(), etc and I've tried to avoid making the patch too large by only changing the places that absolutely had to use InstructionCost. The code below had existing asserts that the instruction cost is valid, so I thought I could do the same thing. If the InstructionCost is valid that means we can safely extract the value from InstructionCost and store as an integer in BBCostMap. It may be that at some point in the future we will encounter invalid costs, but I thought it was best not to try to guess what might happen and just wait to see if it's really a problem or not.

llvm/lib/Transforms/Scalar/SpeculateAroundPHIs.cpp
506

The InstructionCost has an operator<< that prints out "Invalid" if the cost is invalid so the debug would look like this:

" Not profitable, speculation cost: ...\n"
" Cost savings: ...\n"
" SpecCost: Invalid\n"

I just thought that was probably enough for the user to figure out something had gone wrong that's all.

paulwalker-arm added inline comments.Nov 30 2020, 5:15 AM
llvm/lib/Transforms/Scalar/LoopUnswitch.cpp
306 ↗(On Diff #308020)

For the case where the number of instructions is unknown why not treat this as a reason not to unswitch the loop rather than than trying to second guess how Props.SizeEstimation will be used when determining a sensible value to set it to? You could even add an LLVM_DEBUG to report when this happens.

I think this will be a general comment of my for any such code where we want to support not knowing the cost of something or more specifically when the cost is invalid and thus unusable.

david-arm added inline comments.Nov 30 2020, 5:39 AM
llvm/lib/Transforms/Scalar/LoopUnswitch.cpp
306 ↗(On Diff #308020)

Sure, I completely agree with you. My problem here is that I simply didn't know what value to set it to when invalid. It has to be set to something and I wasn't trying to second guess here - I was just trying to ensure it had some sort of obvious, debuggable value, rather than random data. The flag "Metrics.notDuplicatable" will be set to true when invalid and you'll see this is checked below, however I couldn't be sure that Props wouldn't still be checked even if notDuplicatable is true. The Props structure goes into a LoopsProperties map that is kept around for later - it's not thrown away if not duplicatable.

sdesmalen added inline comments.Dec 13 2020, 10:42 PM
llvm/lib/Analysis/CodeMetrics.cpp
182

If the basic-block contains an instruction that would have an Invalid codesize-cost, that means it can't be expanded and having such an instruction in the block is an error. So you can just dereference TTI.getUserCost(...) here directly (which will fail when the cost is invalid). That also removes the need for a lot of the other changes.

llvm/lib/Transforms/Scalar/LoopDataPrefetch.cpp
303 ↗(On Diff #308020)

nit: the extra parenthesis are not required, e.g. unsigned LoopSize = *Metrics.NumInsts.getValue();

llvm/lib/Transforms/Scalar/LoopFlatten.cpp
324

The same holds true here (that you can just dereference RepeatedInstrCost) because the cost should never be Invalid.

david-arm updated this revision to Diff 311862.Dec 15 2020, 4:23 AM
david-arm retitled this revision from [Analysis] Migrate more high level cost functions to using InstructionCost to [Support] Migrate more high level cost functions to using InstructionCost.
david-arm edited the summary of this revision. (Show Details)
david-arm marked 4 inline comments as done.Dec 15 2020, 4:31 AM
david-arm added inline comments.
llvm/lib/Transforms/Scalar/LoopFlatten.cpp
324

Hi @sdesmalen, I wasn't sure if that was the case here because we're iterating through blocks in the FlattenInfo structure that the caller created. I wondered if there was a possibility someone might have created some new instructions or modified existing ones and it seemed trivial to add a check here.

david-arm updated this revision to Diff 315417.Jan 8 2021, 8:52 AM
david-arm marked an inline comment as done.
  • Following discussions on D94069, I've removed unnecessary checks for isValid when comparing costs.
ctetreau added inline comments.Jan 26 2021, 10:41 AM
llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
1017

Where are all of these "return -1"'s handled?

llvm/lib/Transforms/Scalar/LoopUnrollPass.cpp
533

I think this is a functional change. Before it returned a non-None garbage value, and now it returns None. Is there a test that can be written for this?

david-arm abandoned this revision.Mar 15 2021, 5:53 AM