[PATCH] D29631: SystemZTargetTransformInfo cost functions and some common code changes

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

I've looked only at the SystemZ parts ... look basically good to me, but see a number of inline comments.



================
Comment at: lib/Target/SystemZ/SystemZTargetTransformInfo.cpp:269
+        else
+          NumStores++;
       }
----------------
This seems to partially duplicate the logic in getMemoryOpCost ... is there a way to unify those?


================
Comment at: lib/Target/SystemZ/SystemZTargetTransformInfo.cpp:343
+        Opcode == Instruction::AShr || Opcode == Instruction::Or) {
+      return NumVectors;
+    }
----------------
Shouldn't we have And and Xor here as well?


================
Comment at: lib/Target/SystemZ/SystemZTargetTransformInfo.cpp:397
+    // Or requires one instruction, although it has custom handling for i64.
+    if (Opcode == Instruction::Or)
+      return 1;
----------------
Likewise ...


================
Comment at: lib/Target/SystemZ/SystemZTargetTransformInfo.cpp:486
+        else //  Src is <16 x i64>
+          return (DstScalarBits == 32 ? 4 : (DstScalarBits == 16 ? 6 : 7));
+      }
----------------
Can this not be handled generically via getElSizeLog2Diff like below for the unpack case?


================
Comment at: lib/Target/SystemZ/SystemZTargetTransformInfo.cpp:531
+            { ISD::SIGN_EXTEND, MVT::v16i32, 12},
+            { ISD::SIGN_EXTEND, MVT::v16i64, 23},
+          };
----------------
These tables look odd ... but I guess if they accurately reflect the cost of the code that is currently being generated, this is fine with me until we improve codegen.


================
Comment at: lib/Target/SystemZ/SystemZTargetTransformInfo.cpp:621
+      return (SrcScalarBits <= 64 ?
+              (SrcScalarBits >= 32 ? 1 : 2 /*i8/i16 extend*/) : LIBCALL_COST);
+    
----------------
When do we get libcalls?  At least with z196 and above this should never happen, so we might want to take the ISA level into account here.


================
Comment at: lib/Target/SystemZ/SystemZTargetTransformInfo.cpp:737
+          if (CmpScalarBits == 64 && SelScalarBits == 16 && VF == 16)
+            PackCost -= 2; // Minor adjustment
+        }
----------------
This needs more explanation why this adjustment is needed in this case (and only in this case).


https://reviews.llvm.org/D29631