[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/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:
> > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149740