[PATCH] D76124: [TTI] Remove getOperationCost

[Sam Parker via Phabricator via llvm-commits]

samparker marked 2 inline comments as done.
samparker added inline comments.


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfoImpl.h:844
+      break;
+    case Instruction::GetElementPtr: {
+      const GEPOperator *GEP = cast<GEPOperator>(U);
----------------
spatel wrote:
> The earlier review comment shifted, but I think it still applies. The GEP opcode is always handled above here, right?
Ah, I've messed this up rebasing.


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfoImpl.h:861
+      if (getCastInstrCost(Opcode, Ty, OpTy, I) == TTI::TCC_Free ||
+          TargetTTI->getCastInstrCost(Opcode, Ty, OpTy, I) == TTI::TCC_Free)
+        return TTI::TCC_Free;
----------------
spatel wrote:
> This still isn't clear to me (let me know if I'm misunderstanding and/or reading too much into this). 
> 
> getCastInstrCost() returns some integer value as a cost. Do we really want to use the "TCC_Free" enum value as an alias for "0" in the comparison statement? If we are going to use that definition, then do we need to update the documentation comments to specify that TCC_Free is a designated return value and fixed at zero?
My immediate response was, 'isn't that the purpose of an enum?' But I now I think I know what you mean, though I don't feel that it really matters here and isn't out-of-line with how this layer already operates. Maybe it could be addressed at the same time as fixing the unsigned/int mess..? Also, the next step here will be to return the raw value anyway, without the condition, to get a more accurate answer.


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

https://reviews.llvm.org/D76124