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

[Zvi Rackover via Phabricator via llvm-commits]

zvi added inline comments.


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:430
+  unsigned minRequiredElementSize(const Value* Val, bool &isSigned) {
+    const Constant* VectorValue = dyn_cast<Constant>(Val);
+    if (isa<ConstantDataVector>(Val) || isa<ConstantVector>(Val)) {
----------------
Is this cast guaranteed to succeed? If yes, use cast<> instead, otherwise check for nullptr


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:434
+      // required size for each element
+      VectorType *VT = dyn_cast<VectorType>(Val->getType());
+      assert(VT && "Wrong Vector Type!");
----------------
dyn_cast<>() + assert() should be replaced with cast<>()


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:437
+
+      // assume unsigned elements
+      isSigned = false;
----------------
assume -> Assume


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:440
+
+      // the max required size is the total vector width divided by num
+      // of elements in the vector
----------------
the -> The


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:450
+
+        if (!IntElement) {
+          // not an int constant element
----------------
Please consider:

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


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:475
+    const CastInst* Cast = dyn_cast<SExtInst>(Val);
+    if (Cast) {
+      isSigned = true;
----------------
Please consider (and same for another occuerence below):

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



================
Comment at: include/llvm/IR/User.h:241
 
+  struct const_value_op_iterator
+      : iterator_adaptor_base<const_value_op_iterator, const_op_iterator,
----------------
Does this change need any GTest unittest coverage?


================
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(), 
----------------
IMHO it would be better to move this to the case::Instruction::Mul label even at the cost of duplicating Op1VK and Op2VK 


================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:142
+    if (ISD == ISD::MUL && LT.second == MVT::v4i32 ) {
+      if (Args.size() == 2) {
+        // Check if the operands can be shrinked into a smaller datatype.
----------------
Please consider flattening the nested if's


================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:153
+
+        if (OpMinSize <= 7) {
+          return LT.first * 3; // pmullw/sext
----------------
Please drop the braces for bodies with a single statement


================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:628
+  static const CostTblEntry CustomLoweredSLM[] = {
+    // A v2i64/v4i64 and multiply is custom lowered as a series of long
+    // multiplies(3), shifts(3) and adds(2).
----------------
Please reword this comment to be clearer


================
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());
----------------
I->operand_values() will save you a line :)


================
Comment at: test/Analysis/CostModel/X86/slm-arith-costs.ll:32
+  %zext = zext <4 x i8> %a to <4 x i32> 
+  %res = mul nsw <4 x i32> %zext, <i32 250, i32 250, i32 250, i32 250>
+  ret <4 x i32> %res
----------------
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.


https://reviews.llvm.org/D28104