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
115

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
430

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

434

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

437

assume -> Assume

440

the -> The

450

Please consider:

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

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
420

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
142

Please consider flattening the nested if's

153

Please drop the braces for bodies with a single statement

628

Please reword this comment to be clearer

lib/Transforms/Vectorize/LoopVectorize.cpp
6973

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

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

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
141

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
430

fixed

434

fixed

437

fixed

440

fixed

450

changed

475

fixed

include/llvm/IR/User.h
241

done

lib/Analysis/CostModel.cpp
420

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
141

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?

142

fixed

153

done

628

fixed

lib/Transforms/Vectorize/LoopVectorize.cpp
6973

fixed

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

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
431

auto

435

auto

446

This should be:

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

Use auto inside a dyn_cast<>

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

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

467

auto

472

auto

477

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
631

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
511

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

include/llvm/Analysis/TargetTransformInfoImpl.h
446

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
420

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

lib/Transforms/Vectorize/LoopVectorize.cpp
6973

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.