This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by eastig on May 30 2017, 10:42 AM.

Details

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.

Event Timeline

eastig created this revision.May 30 2017, 10:42 AM
efriedma edited edge metadata.May 30 2017, 10:51 AM

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?

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

lib/Analysis/InlineCost.cpp
352

Yep.

eastig updated this revision to Diff 100752.May 30 2017, 1:00 PM

Removed dead code.

eastig retitled this revision from [InlineCost] Change CallAnalyzer::isGEPFree to use TTI::getUserCost instead of TTI::getGEPCost to [InlineCost, NFC] Change CallAnalyzer::isGEPFree to use TTI::getUserCost instead of TTI::getGEPCost.May 30 2017, 1:00 PM
efriedma accepted this revision.May 30 2017, 1:41 PM

LGTM.

This revision is now accepted and ready to land.May 30 2017, 1:41 PM

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 requested changes to this revision.May 30 2017, 2:00 PM

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...

This revision now requires changes to proceed.May 30 2017, 2:00 PM

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

Hi Haicheng,

I see the problem.

Thank you.

eastig retitled this revision from [InlineCost, NFC] Change CallAnalyzer::isGEPFree to use TTI::getUserCost instead of TTI::getGEPCost to [InlineCost] Change CallAnalyzer::isGEPFree to use TTI::getUserCost instead of TTI::getGEPCost.May 30 2017, 3:09 PM

Hi Haicheng,

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

Thanks,
Evgeny

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?

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

eastig added a comment.Jun 5 2017, 1:49 PM

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 updated this revision to Diff 104635.Jun 29 2017, 6:54 AM
eastig edited edge metadata.

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 retitled this revision from [InlineCost] Change CallAnalyzer::isGEPFree to use TTI::getUserCost instead of TTI::getGEPCost to [InlineCost, NFC] Change CallAnalyzer::isGEPFree to use TTI::getUserCost instead of TTI::getGEPCost.Jun 29 2017, 6:54 AM

Kindly ping.

This revision is now accepted and ready to land.Jul 21 2017, 4:16 PM

Thank you, Eli.

This revision was automatically updated to reflect the committed changes.