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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:720
            "Should not initialize SROA costs for null value.");
-    SROAArgCosts[Arg] = 0;
+    SROACostSavings += SROAArgCosts[Arg] =
+        TTI.getCallerAllocaCost(&CandidateCall, Arg);
----------------
nit: can you do this in 2 steps, easier to read. same in `onInitializeSROAArg`. thanks


================
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);
+}
----------------
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`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149740