[PATCH] D149740: [InlineCost][TargetTransformInfo][AMDGPU] Consider cost of alloca instructions in the caller (1/2)

Juan Manuel Martinez CaamaƱo via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 28 02:26:30 PDT 2023


jmmartinez added inline comments.


================
Comment at: llvm/unittests/Analysis/InlineCostTest.cpp:147
+      10); // 2*InstCost which defaults to 5 + AllocaCost which defaults to 0
+  ASSERT_EQ(GetFeature(InlineCostFeatureIndex::sroa_losses), 0);
+}
----------------
mtrofin wrote:
> jmmartinez wrote:
> > mtrofin wrote:
> > > jmmartinez wrote:
> > > > mtrofin wrote:
> > > > > nit: is there something that we can test that's not 0 (to distinguish from "no effect"). Like maybe the total cost?
> > > > I think that would make the test fragile, since any change in the way the cost is calculated would break the test despite not changing how the sroa savings/losses are computed.
> > > > 
> > > > Would it be good if I add a test for the case where the sroa cost is lost?
> > > Not sure what the concern is - if folks change the cost function (not very frequent), a test here would highlight to them whether that side-effect is desired, which is a good thing. Assuming it is, updating should be as easy as updating the number - which is also a good thing, when tracking down regressions, it offers an additional signal ("here's one effect of this change").
> > > 
> > > Adding an additional test for where the sroa cost is lost is good, too.
> > > Adding an additional test for where the sroa cost is lost is good, too.
> > 
> > Will do!
> > 
> > > Not sure what the concern is - if folks change the cost function (not very frequent), a test here would highlight to them whether that side-effect is desired, which is a good thing. Assuming it is, updating should be as easy as updating the number - which is also a good thing, when tracking down regressions, it offers an additional signal ("here's one effect of this change").
> > 
> > I've just realized that there is no "total cost" equivalent in the InliningCostFeatures. The only values affected by this modification are the sroa savings and losses. These costs do not get accumulated in a single value since they are used to feed the MLInlineAdvisor. Or am I missing something?
> SROAArgCosts is used for total cost estimation - see `onDisableSROA`
There is `InlineCostCallAnalyzer::onDisableSROA`, where the cost is used for the total estimation,

But there is the very similar `InlineCostFeatureAnalyzer::onDisableSROA` where the cost is only used for the SROA savings/losses estimation.

In the unittests the second one is getting tested, where there is no total cost estimation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149740/new/

https://reviews.llvm.org/D149740



More information about the llvm-commits mailing list