Page MenuHomePhabricator

Inline Cost improvement - GetElementPtr with constant operands
ClosedPublic

Authored by knaumov on Jun 2 2020, 12:37 PM.

Details

Summary

Currently, InlineCost doesn't simplify GetElementPtr with constant operands. In this patch, we are introducing this functionality.
The improvement is built on a similar lambda-structure as the other simplification of instructions with constant operands.

Diff Detail

Event Timeline

knaumov created this revision.Jun 2 2020, 12:37 PM

What is the impact on the performance of the generated code?

Also, consider offering an opt-in flag.

llvm/lib/Analysis/InlineCost.cpp
1012

Code style: Index (not index)

knaumov updated this revision to Diff 267975.Jun 2 2020, 1:39 PM

Answering @mtrofin 's comments:

As such cases (GEPs from a constant address with constant operands) will be simplified by the pipeline to a constant, InlineCost should be able to see this to apply the correct cost to the instruction. As eventually, the instruction will turn to a constant, the cost is 0.

Answering @mtrofin 's comments:

As such cases (GEPs from a constant address with constant operands) will be simplified by the pipeline to a constant, InlineCost should be able to see this to apply the correct cost to the instruction. As eventually, the instruction will turn to a constant, the cost is 0.

That sounds fine, my point is that having some run on benchmarks to quantify the benefit would help understand the value of this particular omission from the cost analysis (or at minimum show there's no regression, or understand where the regressions may be). It could shed important insights into what matters for the cost analysis, basically.

Having an optin/out flag would help teams quickly handle any unexpected regressions they may encounter in the field.

That sounds fine, my point is that having some run on benchmarks to quantify the benefit would help understand the value of this particular omission from the cost analysis (or at minimum show there's no regression, or understand where the regressions may be). It could shed important insights into what matters for the cost analysis, basically.

The motivation for this change is the sequence generated by our downstream frontend. Essentially we are looking at something like this:

void foo(ID) {
  X = constant_table[ID];
  if (X == some constant) {
    ...
  }
}

This is a bit oversimplified, but when constant ID is passed into foo it can significantly reduce the cost of inlining. But in order to recognize this we need to recognize a gep of constant operands (the gep is used to compute the address of constant_table[ID]).

Teaching InlineCost to recognize geps of constant operands look like a generic enhancements. If you'd like we can do some performance verification on Clang.

That sounds fine, my point is that having some run on benchmarks to quantify the benefit would help understand the value of this particular omission from the cost analysis (or at minimum show there's no regression, or understand where the regressions may be). It could shed important insights into what matters for the cost analysis, basically.

The motivation for this change is the sequence generated by our downstream frontend. Essentially we are looking at something like this:

void foo(ID) {
  X = constant_table[ID];
  if (X == some constant) {
    ...
  }
}

This is a bit oversimplified, but when constant ID is passed into foo it can significantly reduce the cost of inlining. But in order to recognize this we need to recognize a gep of constant operands (the gep is used to compute the address of constant_table[ID]).

Teaching InlineCost to recognize geps of constant operands look like a generic enhancements. If you'd like we can do some performance verification on Clang.

If it's easy, it'd be nice to have. I was mostly concerned with staging it, i.e. having first an easy way to revert in the field, should there be a regression, and then removing that flag later - since adding a flag should be very straight forward.

knaumov updated this revision to Diff 270939.Jun 15 2020, 6:54 PM
  • Added flag for the change which is true by default

I have been struggling to collect the data @mtrofin has asked for to prove the usefulness of the patch. I will continue to do so, but meanwhile, I suggest accepting the change. Once I have gathered needed data, I will post new differential presenting the results and (most likely) deleting the flag.

mtrofin accepted this revision.Jun 15 2020, 7:33 PM
  • Added flag for the change which is true by default

I have been struggling to collect the data @mtrofin has asked for to prove the usefulness of the patch. I will continue to do so, but meanwhile, I suggest accepting the change. Once I have gathered needed data, I will post new differential presenting the results and (most likely) deleting the flag.

LGTM

This revision is now accepted and ready to land.Jun 15 2020, 7:33 PM
This revision was automatically updated to reflect the committed changes.