[PATCH] D102755: [AArch64] Add cost tests for bitreverse

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:295
+        CostTableLookup(BitreverseTbl, ICA.getID(),
+                        (TLI->getTypeLegalizationCost(DL, RetTy)).second);
+    if (TLI->getValueType(DL, RetTy, true) != MVT::Other)
----------------
Can we change it to only call getTypeLegalizationCost once.


================
Comment at: llvm/test/Analysis/CostModel/AArch64/bitreverse-cost-model.ll:8
+; SIZE-LABEL: 'cost8'
+; SIZE-NEXT:  Cost Model: Found an estimated cost of 1 for instruction: %b = call i8 @llvm.bitreverse.i8(i8 %a)
+; SIZE-NEXT:  Cost Model: Found an estimated cost of 1 for instruction: ret i8 %b
----------------
I see this doesn't give a cost of 2. I think it is trying to use the legal type that i8 will be converted to (i32) for the costs.

Maybe.. can we add something that says "if the original element type is not the same as the legal element type, add 1"?


================
Comment at: llvm/test/Analysis/CostModel/AArch64/bitreverse.ll:2
 ; NOTE: Assertions have been autogenerated by utils/update_analyze_test_checks.py
 ; RUN: opt < %s -mtriple=aarch64-unknown-linux-gnu -cost-model -analyze | FileCheck %s
 
----------------
I'm not sure why this testcase was added when there was already one in review adding the same thing. It seems strange. Especially as it missed half of the important cases.

Can you combine the two tests into one? Making sure we have the tests from both. Less boilerplate in them would be good too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102755