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

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa added a comment.

In D54742#1318046 <https://reviews.llvm.org/D54742#1318046>, @chandlerc wrote:

> In D54742#1316748 <https://reviews.llvm.org/D54742#1316748>, @RKSimon wrote:
>
> > @chandlerc Are you happy with this now?
>
>
> Not really. The test is still too indirect and complex IMO.
>
> As I said in my comments, there are *direct* tests of the cost model, including the `getUserCost` result. Not sure why that got ignored, I gave the path for them. A concrete example:
>
>   llvm/test/Analysis/CostModel/X86/costmodel.ll
>
>
> I don't think this should be tested by complex inlining tests. Instead, we have a direct and obvious way to test this stuff and we should be using it. The fact that other architectures are *only* testing the latency or reciprocal throughput cost model, and not the code size cost model, is not a reason to bypass the clear and obvious testing layer we have here. Instead we should add exactly the test for the changed cost model that this change is proposing.


Sorry, but I'm confused. My understanding is that a test that uses '-cost-model -analyze' (like the one you reference) is using *CostModel*. For this simple test function

  define i64 @fun(i64 %Arg1, i64 %Arg2) {
    %Cmp = icmp eq i64 %Arg1, %Arg2
    %Res = zext i1 %Cmp to i64
    ret i64 %Res
  }

, CostModel will not call getUserCost(), but instead getCastInstrCost() for the zext. So my patch does not affect this test, and I don't know how to make a direct test that calls getUserCost() like you propose.


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

https://reviews.llvm.org/D54742