This is an archive of the discontinued LLVM Phabricator instance.

update TTI costs for arithmetic instructions on X86\SLM arch.
ClosedPublic

Authored by magabari on Dec 25 2016, 5:59 AM.

Details

Summary

This patch updates arithmetic instructions costs on SLM arch.

updated instructions:
pmulld, pmullw, pmulhw, mulsd, mulps, mulpd, divss, divps, divsd, divpd, addpd and subpd.

special optimization case which replaces pmulld with pmullw\pmulhw\pshuf seq. in case if the real operands bitwidth <= 16.

Diff Detail

Event Timeline

magabari updated this revision to Diff 82466.Dec 25 2016, 5:59 AM
magabari retitled this revision from to update TTI costs for arithmetic instructions on X86\SLM arch..
magabari updated this object.
RKSimon edited edge metadata.Dec 26 2016, 3:25 PM

This patch also enables memory interleaving for SLM arch.

Is there any reason why this shouldn't be split off into its own patch?

lib/Analysis/CostModel.cpp
55

Would we not be better off adding a const iterator variant for value_op_begin()/value_op_end() instead of losing the const qualifier?

Please can you rebase this patch after rL290267? It updated the vXi64 multiply lowering codegen.

lib/Target/X86/X86TargetTransformInfo.cpp
118

This is not target specific - is there no existing known bits helper code that already does this that we could use? Else move this to somewhere to allow reuse?

magabari updated this revision to Diff 82526.Dec 27 2016, 1:24 AM
magabari edited edge metadata.

rebasing above r290592

magabari updated this object.Dec 27 2016, 6:05 AM
magabari edited edge metadata.
magabari updated this revision to Diff 82545.Dec 27 2016, 10:10 AM

fixed Simon notes.

  1. adding const_op_value_iterator for User class
  2. removing Interleaving changes to be submitted later in separated patch
  3. Moving minRequiredSize function to BaseTTI
  4. changes on vXi64 costs
zvi added inline comments.Dec 27 2016, 11:44 PM
include/llvm/Analysis/TargetTransformInfoImpl.h
434

Is this cast guaranteed to succeed? If yes, use cast<> instead, otherwise check for nullptr

438

dyn_cast<>() + assert() should be replaced with cast<>()

441

assume -> Assume

444

the -> The

454

Please consider:

if ( ConstantInt* IntElement = dyn_cast<ConstantInt>(VectorValue->getAggregateElement(i)) { ... } else return MaxRequireSize
479

Please consider (and same for another occuerence below):

if (const CastInst* Cast = dyn_cast<SExtInst>(Val))

include/llvm/IR/User.h
241

Does this change need any GTest unittest coverage?

lib/Analysis/CostModel.cpp
441

IMHO it would be better to move this to the case::Instruction::Mul label even at the cost of duplicating Op1VK and Op2VK

lib/Target/X86/X86TargetTransformInfo.cpp
145

Please consider flattening the nested if's

156

Please drop the braces for bodies with a single statement

616

Please reword this comment to be clearer

lib/Transforms/Vectorize/LoopVectorize.cpp
6948

I->operand_values() will save you a line :)

test/Analysis/CostModel/X86/slm-arith-costs.ll
33

It would be nice if the constants in the test-cases would be in the boundaries of values that can/cannot be shrunk into smaller types.

RKSimon added inline comments.Dec 29 2016, 3:02 AM
lib/Target/X86/X86TargetTransformInfo.cpp
144

It'd be beneficial for all SSE targets to reuse the minRequiredElementSize code for better integer multiply costs - vXi64 especially but pre-SSE41 for vXi32 as well. So if you can split it from the rest of the SLM specific costs that would be useful. If not, we can try and do it in a later patch.

magabari marked an inline comment as done.Jan 3 2017, 2:19 AM

fixed Zvi comments

include/llvm/Analysis/TargetTransformInfoImpl.h
434

fixed

438

fixed

441

fixed

444

fixed

454

changed

479

fixed

include/llvm/IR/User.h
241

done

lib/Analysis/CostModel.cpp
441

Hmmm .. I think this solution better than duplicating code. If that an issue I can get the operands for all instruction (removing the if-statement), because there is nothing really special to make a separated case for mul. This code can be used for all other instructions later.

lib/Target/X86/X86TargetTransformInfo.cpp
144

I agree with you that we need to do some refactoring here for other targets. However, I believe it's better to do the refactoring in a separate commit and focus on SLM in this one. what do you think?

145

fixed

156

done

616

fixed

lib/Transforms/Vectorize/LoopVectorize.cpp
6948

fixed

test/Analysis/CostModel/X86/slm-arith-costs.ll
33

fixed

Please can you update the diff?

magabari updated this revision to Diff 83043.Jan 4 2017, 6:10 AM
magabari edited edge metadata.
magabari marked an inline comment as done.

fixed Zvi comments

RKSimon added inline comments.Jan 4 2017, 8:34 AM
include/llvm/Analysis/TargetTransformInfoImpl.h
435

auto

439

auto

450

This should be:

for(unsigned i = 0, e = VT->getNumElements(); i < e; ++i) {
452

Use auto inside a dyn_cast<>

if (auto* IntElement = dyn_cast<ConstantInt>(VectorValue->getAggregateElement(i))) {
461

Replace with a MinRequiredSize = std::max(MinRequiredSize, ElementMinRequiredSize) ?

471

auto

476

auto

481

auto

test/Analysis/CostModel/X86/slm-arith-costs.ll
2

Wouldn't it be better to try and add SLM as a check-prefix to existing cost test files?

324

Get rid of this metadata - AFAICT you don't need it.

RKSimon added inline comments.Jan 5 2017, 3:03 PM
lib/Target/X86/X86TargetTransformInfo.cpp
618

I've been simplifying the code in X86TTIImpl::getArithmeticInstrCost and this won't fire now. It would be be better off in the earlier ST->isSLM() LUT.

magabari marked 10 inline comments as done.Jan 8 2017, 1:35 AM

fixed Simon comments

test/Analysis/CostModel/X86/slm-arith-costs.ll
2

I have checked but i think the test itself not covered in previous tests like arith.ll because the focus here is to shrink the operands to small datatype.

some of the cases can be merged with arith.ll do you want me to merge those?

magabari updated this revision to Diff 83552.Jan 8 2017, 1:45 AM
magabari edited edge metadata.

fixed following Simon comments

RKSimon accepted this revision.Jan 8 2017, 3:54 AM
RKSimon edited edge metadata.

LGTM with a couple of minors.

include/llvm/Analysis/TargetTransformInfo.h
521

Please add a comment describing the purpose of Args and its optional nature.

include/llvm/Analysis/TargetTransformInfoImpl.h
450

Why is the increment on the next line? It doesn't seem to be 80-column issue.

include/llvm/IR/User.h
259

Commit this along with the unittests as a separate pre-commit.

lib/Analysis/CostModel.cpp
441

I think we can safely provide the Operands to all cases now.

lib/Transforms/Vectorize/LoopVectorize.cpp
6948

Same as per CostModel.cpp

test/Analysis/CostModel/X86/slm-arith-costs.ll
2

No lets's keep them in one place. Sorry for the noise.

This revision is now accepted and ready to land.Jan 8 2017, 3:54 AM
magabari updated this revision to Diff 83921.Jan 10 2017, 11:34 PM
magabari edited edge metadata.
magabari marked 3 inline comments as done.

fixed following Simon comments

This revision was automatically updated to reflect the committed changes.