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

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Analysis/TargetTransformInfo.cpp:102
 
 IntrinsicCostAttributes::IntrinsicCostAttributes(Intrinsic::ID Id,
                                                  const CallBase &CI,
----------------
samparker wrote:
> Could it help to further reduce complexity in TTI if we moved the vectorizer-specific changes into the vectorizer? (Assuming that constructors are one of the different ways at TTI is used by the vectorizer)
I have given it a try, and ended up adjusting some of the other constructors at the same time. It doesn't feel like the cleanest thing in the world, but keeps the code in the vectorizer. Let me know what you think.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp:763
+    break;
+  case Intrinsic::uadd_sat:
+  case Intrinsic::usub_sat:
----------------
sdesmalen wrote:
> 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.
Yeah, I think the cost was incorrect before from vector instructions, but correct from the vectorizer's costing of vectorized scalar instructions (because it was looking at RetTy and ignoring VF). I was hoping someone from AMDGPU could take a look and check this was OK. I tried some f16 FMA and ssat routines on a gfx900 and they all ended up with the same vectorization - which is a good sign at least.


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

https://reviews.llvm.org/D95291