[InlineCost, NFC] Change CallAnalyzer::isGEPFree to use TTI::getUserCost instead of TTI::getGEPCost

[eastig]

Summary

Currently CallAnalyzer::isGEPFree uses TTI::getGEPCost to check if GEP is free. TTI::getGEPCost cannot handle cases when GEPs participate in Def-Use dependencies (see https://reviews.llvm.org/D31186 for example). There is TTI::getUserCost which can calculate the cost:

int TargetTransformInfo::getUserCost (const User * U) const
Estimate the cost of a given IR user when lowered.

This can estimate the cost of either a ConstantExpr or Instruction when lowered. It has two primary advantages over the getOperationCost and getGEPCost above, and one significant disadvantage: it can only be used when the IR construct has already been formed.

The advantages are that it can inspect the SSA use graph to reason more accurately about the cost. For example, all-constant-GEPs can often be folded into a load or other instruction, but if they are used in some other context they may not be folded. This routine can distinguish such cases.

The returned cost is defined in terms of TargetCostConstants, see its comments for a detailed explanation of the cost values.

This patch gets CallAnalyzer::isGEPFree to use TTI::getUserCost instead of TTI::getGEPCost.

[efriedma]

Is there any functional change here? (If there is, needs a test; if not, please add a note to the commit message.)

lib/Analysis/InlineCost.cpp

352

Dead loop?

[eastig]

It's NFC. 'getUserCost' does not do any analysis at the moment, just calls getGEPCost.

lib/Analysis/InlineCost.cpp

352

Yep.

[eastig]

Removed dead code.

[efriedma]

LGTM.

[haicheng]

Hi Evgeny,

Please see the embed comment. SimplifiedValues maps variables to known constants. So, I think you change is not NFC.

Haicheng

lib/Analysis/InlineCost.cpp

348–352

Why removing this loop won't cause functional change?

[efriedma]

Oh, oops, you're right, sorry. Sorry, I wasn't paying close enough attention to the loop.

I'm surprised we don't have any test coverage for this...

[haicheng]

In D33685#768100, @efriedma wrote:

I'm surprised we don't have any test coverage for this...

That is my bad. I did not add a test for this functionality when I wrote this piece of code. I can add one now.

Haicheng

[eastig]

Hi Haicheng,

I see the problem.

Thank you.

[eastig]

Hi Haicheng,

Could you please provide a test case?
I'd like to look at it.

Thanks,
Evgeny

[eastig]

We have:

1. We can inform getGEPCost about simplified indices to help it with calculating the cost. But getGEPCost does not take into account the context which GEPs are used in.
2. We have getUserCost which can take the context into account but we cannot inform about simplified indices.

Any ideas how to resolve this?

[efriedma]

We could add a version of getUserCost() which takes both an instruction and a list of operands, I guess?

[eastig]

In D33685#768949, @efriedma wrote:

We could add a version of getUserCost() which takes both an instruction and a list of operands, I guess?

Yes, I've got the similar idea. After some thinking about it, I am not sure it should be a list. I see in the original code the name 'SimplifiedValues' is used with an lookup interface. I think something similar should be used.
How about llvm/IR/ValueMap.h? Or maybe DenseMap?

[eastig]

As D34057(getUserCost with an additional list of operands) is landed we can
switch to use it.
This is NFC because at the end getGEPCost is called with the same arguments.

[eastig]

Kindly ping.

[efriedma]

LGTM

[eastig]

Thank you, Eli.

lib/Analysis/InlineCost.cpp

bool CallAnalyzer::accumulateGEPOffset(GEPOperator &GEP, APInt &Offset) {
   return true;
 }
 
 /// \brief Use TTI to check whether a GEP is free.
 ///
 /// Respects any simplified values known during the analysis of this callsite.
 bool CallAnalyzer::isGEPFree(GetElementPtrInst &GEP) {
   SmallVector<Value *, 4> Indices;
   for (User::op_iterator I = GEP.idx_begin(), E = GEP.idx_end(); I != E; ++I)
     if (Constant *SimpleOp = SimplifiedValues.lookup(*I))
        Indices.push_back(SimpleOp);
      else
        Indices.push_back(*I);
-  return TargetTransformInfo::TCC_Free ==
-         TTI.getGEPCost(GEP.getSourceElementType(), GEP.getPointerOperand(),
-                        Indices);
+  return TargetTransformInfo::TCC_Free == TTI.getUserCost(&GEP);
 }
 
 bool CallAnalyzer::visitAlloca(AllocaInst &I) {
   // Check whether inlining will turn a dynamic alloca into a static
   // alloca and handle that case.
   if (I.isArrayAllocation()) {
     Constant *Size = SimplifiedValues.lookup(I.getArraySize());
     if (auto *AllocSize = dyn_cast_or_null<ConstantInt>(Size)) {