[PATCH] D28104: update TTI costs for arithmetic instructions on X86\SLM arch.
Simon Pilgrim via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Jan 8 03:54:33 PST 2017
RKSimon accepted this revision.
RKSimon added a comment.
This revision is now accepted and ready to land.
LGTM with a couple of minors.
================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:521
/// \return The expected cost of arithmetic ops, such as mul, xor, fsub, etc.
int getArithmeticInstrCost(
unsigned Opcode, Type *Ty, OperandValueKind Opd1Info = OK_AnyValue,
----------------
Please add a comment describing the purpose of Args and its optional nature.
================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:446
+ for(unsigned i = 0; i < VT->getNumElements();
+ ++i) {
+ if (ConstantInt* IntElement =
----------------
RKSimon wrote:
> This should be:
> ```
> for(unsigned i = 0, e = VT->getNumElements(); i < e; ++i) {
> ```
Why is the increment on the next line? It doesn't seem to be 80-column issue.
================
Comment at: include/llvm/IR/User.h:259
+ }
+
/// \brief Drop all references to operands.
----------------
Commit this along with the unittests as a separate pre-commit.
================
Comment at: lib/Analysis/CostModel.cpp:420
getOperandInfo(I->getOperand(1));
+ if (I->getOpcode() == Instruction::Mul) {
+ SmallVector<const Value*, 2> Operands(I->value_op_begin(),
----------------
magabari wrote:
> zvi wrote:
> > IMHO it would be better to move this to the case::Instruction::Mul label even at the cost of duplicating Op1VK and Op2VK
> 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.
I think we can safely provide the Operands to all cases now.
================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6973
+ if (I->getOpcode() == Instruction::Mul) {
+ SmallVector<const Value *, 4> Operands(I->value_op_begin(),
+ I->value_op_end());
----------------
magabari wrote:
> zvi wrote:
> > I->operand_values() will save you a line :)
> fixed
Same as per CostModel.cpp
================
Comment at: test/Analysis/CostModel/X86/slm-arith-costs.ll:1
+; RUN: opt < %s -cost-model -analyze -mcpu=slm | FileCheck %s --check-prefix=SLM
+
----------------
magabari wrote:
> RKSimon wrote:
> > Wouldn't it be better to try and add SLM as a check-prefix to existing cost test files?
> 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?
No lets's keep them in one place. Sorry for the noise.
https://reviews.llvm.org/D28104
More information about the llvm-commits
mailing list