[PATCH] D31186: Changing TargetTransformInfo::getGEPCost to take GetElementPtrInst as parameter

[Evgeny Astigeevich via Phabricator via llvm-commits]

eastig added a comment.

Eli, Hal and Jonas, thank you for comments.

I see there are use cases I have not taken into account.

Let me summarize:

- **Requirements:**

"get*Cost" functions should try to estimate the cost of an operation when lowered.

- **User stories:**

1. An user wants to estimate the cost of an operation to make a decision whether it's worth to create it. The operation does not exist in IR.
2. An user has an operation in IR and wants to know the cost of the operation to estimate its contribution into execution.

> Actually, the inliner itself could in theory take advantage of that: if you have a function which returns a GEP, the cost depends on how the caller uses the value. That might be overkill, though, given the tiny effect in most cases.

Eli, you wrote: "the cost depends on how the caller uses the value." A question is where the dependency should be taken into account: in get*Cost or in user's place?

Hal, you wrote:

> I'll also point out that we're further walking down the path of costing instruction patterns

What do you mean "instruction patterns"?

Taking into account all of these I thin "get*Cost" functions should answer the question: What is the cost of an operation if I want to have it or I have it in DFG/CFG?

Maybe it's time to redesign API?

Jonas, an optional parameter duplicates the information passed through other parameters. It can provide all of the needed information. Also single API for all use cases might create some kind of misunderstanding.



================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:571
                  ArrayRef<const Value *> Operands) {
+    assert(GEP);
+    const Value *Ptr = GEP->getPointerOperand();
----------------
efriedma wrote:
> Useless assertion/
I prefer to have controlled crashes instead of uncontrolled ones. It has saved debugging time quite often when a pointer was passed through a chain of calls and was dereferenced in somewhere in the middle of the chain. The assert triggered at the beginning of the chain so I didn't need to examine the whole call stack.
Here it's also for the purpose of a contract: GEP must not be null.



https://reviews.llvm.org/D31186