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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp:763
+    break;
+  case Intrinsic::uadd_sat:
+  case Intrinsic::usub_sat:
----------------
dmgreen wrote:
> 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.
Is this change then to keep the cost the same as before this patch? I would have expected NElts not to need any change, maybe the cost was just wrong before.


================
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)
----------------
dmgreen wrote:
> 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.
Ah, I hadn't seen that patch yet, that explains things. Given that D95292 hasn't landed, this patch is missing a dependence?


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

https://reviews.llvm.org/D95291