This patch adds cost model for vector compare and select instructions. For vector FP compare instruction, it only add the comparisions supported natively.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp | ||
---|---|---|
808 | Using a switch would be more idiomatic. Optional change | |
811 | 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. | |
llvm/test/Analysis/CostModel/RISCV/rvv-cmp.ll | ||
2 | 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". |
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp | ||
---|---|---|
811 | Thanks a lot, if the target do not support the input floating point vector type(need to scalarize), I change it to use the base function. | |
llvm/test/Analysis/CostModel/RISCV/rvv-cmp.ll | ||
2 | Done. |
Could you give https://reviews.llvm.org/D132827 approved, so that I can land the test first.
I went ahead and formally approved that review, but for future reference, our review approval is only loosely tied to specific reviews. In a case like this where I've specifically given approval for particular pieces, you can land those without need for a separate review. This even applies when you've *already* created a separate review; just make sure you include a comment on that review to point to the approval comment.
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp | ||
---|---|---|
739–758 | Scalarize isn't what we're doing here. Fix the comment. | |
740 | I'm not sure about the correctness of this floating point test. There's too many interacting extensions, and I'm not sure when these predicates get set. Looking at the code, I think the check is right here, but I'm not sure. @craig.topper What's your take here? Style wise, I'd encourage you to write this as: switch (VecPred) case OneOfMany: case TwoOfMany: ... case NOfMany: return LT.first * 1; }; } Specifically, let all of the unhandled cases fall through. |
I'm not sure about the correctness of this floating point test. There's too many interacting extensions, and I'm not sure when these predicates get set. Looking at the code, I think the check is right here, but I'm not sure.
@craig.topper What's your take here?
Style wise, I'd encourage you to write this as:
if (has appropriate type clause) {
}
Specifically, let all of the unhandled cases fall through.