[InlineCost] CallAnalyzer: use TTI info for extractvalue - they are free (PR50099)

[lebedev.ri]

Summary

It seems incorrect to use TTI data in some places, and override it in others.
In this case, TTI says that extractvalue are free, yet we bill them.

While this doesn't address https://bugs.llvm.org/show_bug.cgi?id=50099 yet,
it reduces the cost from 55 to 50 while the threshold is 45.

[xbolva00]

llvm/lib/Analysis/InlineCost.cpp

1757

Just question: this check is also in visitInstruction so maybe you can just return visitInstruction(I)?

[lebedev.ri]

llvm/lib/Analysis/InlineCost.cpp

1757

Not really, they have different side-effects.

[aeubanks]

llvm/lib/Analysis/InlineCost.cpp

1757

do you mean the disableSROA? shouldn't that be happening anyway? or else we should handle SROA for extractvalue.

1766

should we do the same for insertvalue?

llvm/test/Transforms/Inline/X86/extractvalue.ll

1

can the first RUN be deleted?

[lebedev.ri]

llvm/lib/Analysis/InlineCost.cpp

1757

do you mean the disableSROA?

Yes.

shouldn't that be happening anyway? or else we should handle SROA for extractvalue.

Well, not as per the comment at the end of this function:

// SROA can look through these but give them a cost.
return false;

Regardless of whether or not that is wrong, it seems like a separate problem for another patch.

Regardless, deferring to visitInstruction() seems wrong,
because i think getUserCost() is much cheaper than the simplifyInstruction()
that would run first.

1766

For consistency - maybe.
To model them as free - are insertvalues free? D66098 only modelled extractvalues.
So again, seems like a separate issue.

llvm/test/Transforms/Inline/X86/extractvalue.ll

1

I'm only following the example in other tests in the directory.
I think if it's not needed, then perhaps all other such runlines should be dropped first?

[lebedev.ri]

@aeubanks thank you for taking a look!

[aeubanks]

llvm/lib/Analysis/InlineCost.cpp

1757

If we first check getUserCost() then we may miss out on some simplification that CallAnalyzer::simplifyInstruction() could do for future users of the extractelement. It seems more in line (hehe) with the rest of CallAnalyzer to call simplifyInstruction() first. Lots of CallAnalyzer methods immediately call simplifyInstruction().

Also, looking at SROA.cpp, it doesn't seem to handle extractelements.

1766

oh, of course insertvalues aren't free, ignore my comment

[lebedev.ri]

@aeubanks thank you for taking a look!

llvm/lib/Analysis/InlineCost.cpp

1757

Aha, okay, let's do that then.

1766

Note that i'm not saying that they are/aren't free.
I'm only saying that currently they are modelled as non-free.
I *suspect* they may also be free, but they are so rare
that i haven't seen a motivational example including them...

[lebedev.ri]

Address inline comment.

[aeubanks]

looks like the child patch somehow found its way here

llvm/lib/Analysis/InlineCost.cpp

1763

I think Base::visitExtractValue() is the proper way

[lebedev.ri]

@aeubanks thank you for taking a look!
Addressing inline comments.

llvm/lib/Analysis/InlineCost.cpp

1763

Well, then all the code that uses this "proper way" is likely broken,
and either doesn't have test coverage,
or the test changes weren't analyzed well enough.

Because then we don't fall back to CallAnalyzer::visitInstruction(),
and don't consider it as zero-cost as per costmodel...

[aeubanks]

overall looks good now, just nits

llvm/lib/Analysis/InlineCost.cpp

1763

I thought Base::visitExtractValue() ends up calling CallAnalyzer::visitInstruction(). Base::visitExtractValue() goes through InstVisitor's instruction hierarchy to figure out what to call next. Currently it always ends up calling CallAnalyzer::visitInstruction(), but if in the future InstVisitor put something in between visitExtractValue() and visitInstruction() then we'd want to respect that.

[lebedev.ri]

@aeubanks thank you for taking a look!
The nit is non-addressable, so no changes are made.

llvm/lib/Analysis/InlineCost.cpp

1763

I thought Base::visitExtractValue() ends up calling CallAnalyzer::visitInstruction().

That is not what is happening, no.
Moreover, i'm not sure InstVisitor would ever do that.

[aeubanks]

llvm/lib/Analysis/InlineCost.cpp

1763

I took a closer look, InstVisitor does do something like that, but in this case it calls CallAnalyzer::visitUnaryInstruction() before getting to visitInstruction(). If we delete CallAnalyzer::visitUnaryInstruction() what I suggested should work. D101577 to remove visitUnaryInstruction().

[lebedev.ri]

@aeubanks thank you for the review and cleaning the CallAnalyzer's InstVisitor!

llvm/lib/Analysis/InlineCost.cpp

bool CallAnalyzer::visitStore(StoreInst &I) {
   // 2. We should probably at some point thread MemorySSA for the callee into
   // this and then use that to actually compute *really* precise savings.
   disableLoadElimination();
   return false;
 }
 
 bool CallAnalyzer::visitExtractValue(ExtractValueInst &I) {
   // Constant folding for extract value is trivial.
   if (simplifyInstruction(I, [&](SmallVectorImpl<Constant *> &COps) {
         return ConstantExpr::getExtractValue(COps[0], I.getIndices());
       }))
     return true;
 
-  // SROA can look through these but give them a cost.
-  return false;
+  // SROA can't look through these, but they may be free.
+  return Base::visitExtractValue(I);
 }
 
 bool CallAnalyzer::visitInsertValue(InsertValueInst &I) {
   // Constant folding for insert value is trivial.
   if (simplifyInstruction(I, [&](SmallVectorImpl<Constant *> &COps) {
         return ConstantExpr::getInsertValue(/*AggregateOperand*/ COps[0],
                                             /*InsertedValueOperand*/ COps[1],
                                             I.getIndices());
       }))
     return true;
 
-  // SROA can look through these but give them a cost.
-  return false;
+  // SROA can't look through these, but they may be free.
+  return Base::visitInsertValue(I);
 }
 
 /// Try to simplify a call site.
 ///
 /// Takes a concrete function and callsite and tries to actually simplify it by
 /// analyzing the arguments and call itself with instsimplify. Returns true if
 /// it has simplified the callsite to some other entity (a constant), making it
 /// free.

llvm/test/Transforms/Inline/X86/extractvalue.ll

 ; RUN: opt < %s -inline -inline-threshold=0 -debug-only=inline-cost -print-instruction-comments -S -mtriple=x86_64-unknown-linux-gnu 2>&1 | FileCheck %s
 ; RUN: opt < %s -passes='cgscc(inline)' -inline-threshold=0 -debug-only=inline-cost -print-instruction-comments -S -mtriple=x86_64-unknown-linux-gnu 2>&1 | FileCheck %s
 ; REQUIRES: asserts
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-unknown"
 
 ; Check that extractvalue's are free.
 
 ; CHECK: Analyzing call of callee... (caller:caller_range)
 ; CHECK-NEXT: define i32 @callee({ i32, i32 } %arg) {
-; CHECK-NEXT: ; cost before = -35, cost after = -30, threshold before = 0, threshold after = 0, cost delta = 5
+; CHECK-NEXT: ; cost before = -35, cost after = -35, threshold before = 0, threshold after = 0, cost delta = 0
 ; CHECK-NEXT:   %r = extractvalue { i32, i32 } %arg, 0
-; CHECK-NEXT: ; cost before = -30, cost after = -30, threshold before = 0, threshold after = 0, cost delta = 0
+; CHECK-NEXT: ; cost before = -35, cost after = -35, threshold before = 0, threshold after = 0, cost delta = 0
 ; CHECK-NEXT:   ret i32 %r
 ; CHECK-NEXT: }
 
 define i32 @callee({i32, i32} %arg) {
   %r = extractvalue {i32, i32} %arg, 0
   ret i32 %r
 }
 
 define i32 @caller_range({i32, i32} %arg) {
   %r = call i32 @callee({i32, i32} %arg)
   ret i32 %r
 }