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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc requested changes to this revision.
chandlerc added a comment.
This revision now requires changes to proceed.

As was mentioned in the original patch, this does still need tests. There are tests here, for example in `test/Analysis/CostModel/...`. You should be able to see some differences in costs, especially for architectures where the resulting cost computation differs significantly.



================
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))
----------------
You don't need this line at all -- the checks below are sufficient.


================
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;
----------------
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.


https://reviews.llvm.org/D54742