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

[Mohammed Agabaria via Phabricator via llvm-commits]

magabari marked an inline comment as done.
magabari added a comment.

fixed Zvi 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)) {
----------------
zvi wrote:
> Is this cast guaranteed to succeed? If yes, use cast<> instead, otherwise check for nullptr
fixed


================
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!");
----------------
zvi wrote:
> dyn_cast<>() + assert() should be replaced with cast<>()
fixed


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


================
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
----------------
zvi wrote:
> the -> The
fixed


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:450
+
+        if (!IntElement) {
+          // not an int constant element
----------------
zvi wrote:
> Please consider:
> 
>   if ( ConstantInt* IntElement = dyn_cast<ConstantInt>(VectorValue->getAggregateElement(i)) { ... } else return MaxRequireSize
changed


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


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


================
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(), 
----------------
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. 


================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:141
+  if (ST->isSLM()) {
+    if (ISD == ISD::MUL && LT.second == MVT::v4i32 ) {
+      if (Args.size() == 2) {
----------------
RKSimon wrote:
> 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.
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?


================
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.
----------------
zvi wrote:
> Please consider flattening the nested if's
fixed


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


================
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).
----------------
zvi wrote:
> Please reword this comment to be clearer
fixed


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


================
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
----------------
zvi wrote:
> 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.
fixed


https://reviews.llvm.org/D28104