This is an archive of the discontinued LLVM Phabricator instance.

[LSR] ignore profitable chain optimization when instruction number is the major cost
ClosedPublic

Authored by shchenz on Oct 18 2020, 8:33 PM.

Details

Summary

isProfitableChain was added based on register number cost. But now, there are some targets like X86/PowerPC taking instruction number as the major cost.

So on these targets, isProfitableChain wrongly eliminates some LSRUse based on register number when collecting chains/LSRUses, these eliminated LSRUse may impact the instruction number a lot in later rate cost phase. Thus we will get a suboptimal loop code sequence.

This patch can make some cpu2017 benchmarks improve on PowerPC.

Diff Detail

Event Timeline

shchenz created this revision.Oct 18 2020, 8:33 PM
shchenz requested review of this revision.Oct 18 2020, 8:33 PM
steven.zhang added inline comments.Oct 18 2020, 8:48 PM
llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
3102

I am not familiar with the code logic. But it makes more sense to bail out here when the instruction is the major cost.

shchenz updated this revision to Diff 299547.Oct 20 2020, 8:33 PM

add a new target hook isRegisterNumberMajorCost

shchenz edited the summary of this revision. (Show Details)Oct 20 2020, 8:33 PM
shchenz added inline comments.Oct 20 2020, 8:37 PM
llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
3102

agree with your concern here. return early always good for compiling time. But the logic here should be specific for isProfitableChain, it would make more sense to keep it inside isProfitableChain. For example what if we have more than one caller of isProfitableChain function?

samparker added inline comments.Oct 21 2020, 12:58 AM
llvm/include/llvm/Analysis/TargetTransformInfo.h
619

Probably a good idea to including LSR in the name, like the other queries.

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
2841

This can potentially interfere with the calls to isProfitableLSRChainElement, which also isn't specifically interested in register uses. It will require more cycles to process but how about moving this to after the for loop below?

shchenz updated this revision to Diff 299681.Oct 21 2020, 7:22 AM
shchenz marked an inline comment as done.

update:
1: modify the new hook name
2: move it after isProfitableLSRChainElement

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
2841

good catch. Updated

samparker added inline comments.Oct 21 2020, 7:35 AM
llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
2873

This will still interfere because if any element in the chain is a ProfitableLSRChainElement, we should return true. So the major cost check should only be executing just before we start doing final cost calculation outside of the loop.

shchenz updated this revision to Diff 299693.Oct 21 2020, 7:58 AM

update:
1: put new added hook after isProfitableLSRChainElement for real...

samparker accepted this revision.Oct 21 2020, 8:05 AM

Having separate loops sounds good to me, but please remove the extra conditional before committing. Thanks.

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
2862

You don't need this now :)

This revision is now accepted and ready to land.Oct 21 2020, 8:05 AM
shchenz added inline comments.Oct 21 2020, 5:49 PM
llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
2862

seems we still need this? The following loop does not contain the first element in the chain.

// Return the first increment in the chain.
const_iterator begin() const {
  assert(!Incs.empty());
  return std::next(Incs.begin());
}
const_iterator end() const {
  return Incs.end();
}
shchenz added inline comments.Oct 21 2020, 5:53 PM
llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
2862

Ah, I know what you mean, we should change the loop in line 2865 to use Chain.Incs and thus we can delete the first one

shchenz updated this revision to Diff 299838.Oct 21 2020, 5:59 PM

update:
1: merge header element checking into the new loop

This revision was landed with ongoing or failed builds.Oct 23 2020, 6:36 AM
This revision was automatically updated to reflect the committed changes.
xbolva00 added subscribers: RKSimon, xbolva00.EditedOct 23 2020, 6:44 AM

Do you plan to post patch to use this hook for x86?

@RKSimon @craig.topper

@jonpa @uweigand FYI as well for SystemZ. SystemZ does not take register number as major cost either.

jonpa added inline comments.Oct 26 2020, 6:35 AM
llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
2870

This seems unclear to me - should it be "If register number is *not* the...", or perhaps "*Only* if register..." ?

It would also be a bit more readable to me if the new hook was called "isNumRegsMajorCostOfLSR", since NumRegs is the name for that counter. (And the comment "number of registers").. but maybe that's just me...

shchenz added inline comments.Oct 26 2020, 7:31 PM
llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
2870

Appreciate the comments. Addressed in NFC patch https://reviews.llvm.org/rG00e573cadb2791804fd0859d0ee05b27b702e11e