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

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:431
+    if (isa<ConstantDataVector>(Val) || isa<ConstantVector>(Val)) {
+      const Constant* VectorValue = cast<Constant>(Val);
+
----------------
auto


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:435
+      // required size for each element
+      VectorType *VT = cast<VectorType>(Val->getType());
+
----------------
auto


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:446
+      for(unsigned i = 0; i < VT->getNumElements();
+        ++i) {
+        if (ConstantInt* IntElement =
----------------
This should be:
```
for(unsigned i = 0, e = VT->getNumElements(); i < e; ++i) {
```


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:448
+        if (ConstantInt* IntElement =
+              dyn_cast<ConstantInt>(VectorValue->getAggregateElement(i))) {
+          bool signedElement = IntElement->getValue().isNegative();
----------------
Use auto inside a dyn_cast<>
```
if (auto* IntElement = dyn_cast<ConstantInt>(VectorValue->getAggregateElement(i))) {
```


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:457
+          if (MinRequiredSize < ElementMinRequiredSize)
+            MinRequiredSize = ElementMinRequiredSize;
+        }
----------------
Replace with a MinRequiredSize = std::max(MinRequiredSize, ElementMinRequiredSize) ?


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:467
+
+    if (const ConstantInt* CI = dyn_cast<ConstantInt>(Val)) {
+      isSigned = CI->getValue().isNegative();
----------------
auto


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:472
+
+    if (const CastInst* Cast = dyn_cast<SExtInst>(Val)) {
+      isSigned = true;
----------------
auto


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:477
+
+    if (const CastInst* Cast = dyn_cast<ZExtInst>(Val)) {
+      isSigned = false;
----------------
auto


================
Comment at: test/Analysis/CostModel/X86/slm-arith-costs.ll:1
+; RUN: opt < %s -cost-model -analyze -mcpu=slm | FileCheck %s --check-prefix=SLM
+
----------------
Wouldn't it be better to try and add SLM as a check-prefix to existing cost test files?


================
Comment at: test/Analysis/CostModel/X86/slm-arith-costs.ll:323
+!2 = !{!"omnipotent char", !3, i64 0}
+!3 = !{!"Simple C/C++ TBAA"}
----------------
Get rid of this metadata - AFAICT you don't need it.


https://reviews.llvm.org/D28104