[PATCH] D28104: update TTI costs for arithmetic instructions on X86\SLM arch.

[Simon Pilgrim via Phabricator via llvm-commits]

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