[PATCH] D84995: [AMDGPU][CostModel] Add f16, f64 and contract cases to fused costs estimation.

[Daniil Fukalov via Phabricator via llvm-commits]

dfukalov added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp:514
+    if ((SLT == MVT::f32 || SLT == MVT::f64 || SLT == MVT::f16) && CxtI &&
+        CxtI->hasOneUse())
       if (const auto *FAdd = dyn_cast<BinaryOperator>(*CxtI->user_begin())) {
----------------
arsenm wrote:
> I think we don't actually need the hasOneUse case. Only with the conditionally available 2-operand VOP2 forms is there a code size savings by not fusing 
Generally speaking, fused operation cost will be almost the same as FADD/FSUB that is estimated in detail in next case of this switch.
If the FMUL result is used elsewhere than FADD/FSUB that means the FMUL possible will not be eliminated by fusing.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp:518
+        if ((OPC == ISD::FADD || OPC == ISD::FSUB) &&
+            (!HasFP32Denormals ||
+             (CxtI->hasAllowContract() && FAdd->hasAllowContract())))
----------------
arsenm wrote:
> The !HasFP32Denormals check is low applying to all types?
Yes, since here we do not estimate FMUL cost, but estimate it will be fused and LLVM_FALLTHROUGH in other cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84995