[PATCH] D132296: [RISCV] Add cost model for compare and select instructions.

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

LGTM to the tests and select, and *integer* comparisons only.  Feel free to land those, and then move the fcmp to it's own patch (or  continue discussion here).

Please land the tests first (updated for current ToT costs), then the change (with the tests updated again) so that the change in result is visible in the tests.



================
Comment at: llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp:588
+    // Support natively.
+    if (CmpInst::isIntPredicate(VecPred) || VecPred == CmpInst::FCMP_OEQ ||
+        VecPred == CmpInst::FCMP_OGT || VecPred == CmpInst::FCMP_OGE ||
----------------
Using a switch would be more idiomatic.  Optional change


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp:591
+        VecPred == CmpInst::FCMP_OLT || VecPred == CmpInst::FCMP_OLE ||
+        VecPred == CmpInst::FCMP_UNE) {
+      return LT.first * 1;
----------------
The fcmp handling isn't right for the case where you have a vector hardware without float or double.  This is the same problem I ran into on the intrinsic costing side.  


================
Comment at: llvm/test/Analysis/CostModel/RISCV/rvv-cmp.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_analyze_test_checks.py
+; RUN: opt -passes="print<cost-model>" 2>&1 -disable-output -mtriple=riscv64 -mattr=+v,+f,+d,+zfh,+experimental-zvfh -riscv-v-vector-bits-min=128 -riscv-v-fixed-length-vector-lmul-max=1 < %s | FileCheck %s
+; Check that we don't crash querying costs when vectors are not enabled.
----------------
Please replace "-riscv-v-vector-bits-min=128" with "-riscv-v-vector-bits-min=-1" and remove "-riscv-v-fixed-length-vector-lmul-max=1".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132296