[PATCH] D54742: [CodeMetrics] Don't let extends of i1 be free.

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa added inline comments.


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:810
 
-    if (const CastInst *CI = dyn_cast<CastInst>(U)) {
-      // Result of a cmp instruction is often extended (to be used by other
-      // cmp instructions, logical or return instructions). These are usually
-      // nop on most sane targets.
-      if (isa<CmpInst>(CI->getOperand(0)))
-        return TTI::TCC_Free;
+    if (const CastInst *CI = dyn_cast<CastInst>(U))
       if (isa<SExtInst>(CI) || isa<ZExtInst>(CI) || isa<FPExtInst>(CI))
----------------
chandlerc wrote:
> You don't need this line at all -- the checks below are sufficient.
OK, moved the cast to the return statement.


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:811-815
-      // Result of a cmp instruction is often extended (to be used by other
-      // cmp instructions, logical or return instructions). These are usually
-      // nop on most sane targets.
-      if (isa<CmpInst>(CI->getOperand(0)))
-        return TTI::TCC_Free;
----------------
chandlerc wrote:
> The intent (that seems pretty clear from this comment) was not to necessarily say that *all* i1 extends were free, but that `cmp(ext(cmp(...)))` is free. That is, an extend which only exists to chain one comparison to another. That seems much more plausible than saying that *any* extend of an i1 is free.
> 
> However, there is no way of modeling such a "free" extend in this area.... At least, not with the cost model as we have it set up.
> 
> We could potentially model this by saying that the second `cmp` is free and clearly documenting that the reason is because we will have already accounted for the cost when visiting the extend.
> 
> That still leaves open the question: are chained comparisons actually free on all targets? My guess is that they are not. For example, even on x86, one of the most CISCy targets, chained comparisons of this kind will rarely if ever be "free" in any meaningful sense.... If that is what you want to correct by making *all* of this go away (which does seem reasonable to me) I think you need to much more clearly explain this in the patch description to avoid confusion.
> However, there is no way of modeling such a "free" extend in this area.... At least, not with the cost model as we have it set up.

IIUC, those passes that want a higher level of accuracy for the cost values, could instead call getCastInstrCost() or getCmpSelInstrCost() and pass the Instruction pointer to then analyze the code and isolate cases where e.g. the second compare is free.

It would even be possible for a target to override getUserCost() and perhaps do something similar, since the User pointer is available, although getUserCost() is supposed to be simple and quick, I think.



https://reviews.llvm.org/D54742