[PATCH] D95291: [CostModel] Remove VF from IntrinsicCostAttributes

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp:763
+    break;
+  case Intrinsic::uadd_sat:
+  case Intrinsic::usub_sat:
----------------
sdesmalen wrote:
> Why are you specially calling out uadd_sat and others?
Because of intrinsicHasPackedVectorBenefit above. For most other intrinsics it will just end up calling BaseT::getIntrinsicInstrCost.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/intrinsiccost.ll:11
 ; CHECK-COST: Found an estimated cost of 10 for VF 1 For instruction:   %1 = tail call i16 @llvm.sadd.sat.i16(i16 %0, i16 %offset)
-; CHECK-COST: Found an estimated cost of 26 for VF 2 For instruction:   %1 = tail call i16 @llvm.sadd.sat.i16(i16 %0, i16 %offset)
-; CHECK-COST: Found an estimated cost of 58 for VF 4 For instruction:   %1 = tail call i16 @llvm.sadd.sat.i16(i16 %0, i16 %offset)
-; CHECK-COST: Found an estimated cost of 122 for VF 8 For instruction:   %1 = tail call i16 @llvm.sadd.sat.i16(i16 %0, i16 %offset)
+; CHECK-COST: Found an estimated cost of 4 for VF 2 For instruction:   %1 = tail call i16 @llvm.sadd.sat.i16(i16 %0, i16 %offset)
+; CHECK-COST: Found an estimated cost of 1 for VF 4 For instruction:   %1 = tail call i16 @llvm.sadd.sat.i16(i16 %0, i16 %offset)
----------------
sdesmalen wrote:
> Are there changes to these costs because it would previously calculate `scalarization cost + 2 * cost(sadd.sat.i16)` where it can now calculates the cost as `cost(sadd.sat.2i16)`?
Yes I believe so. At least - the old cost was from Base::getIntrinsicInstrCost as the overrides from AArch64TargetTransformInfo were not taking effect. I'm not 100% sure how it was calculated but that sounds very plausible

https://reviews.llvm.org/D95292 updated the sadd.sat costs.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95291/new/

https://reviews.llvm.org/D95291