This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add cost model for compare and select instructions.
ClosedPublic

Authored by jacquesguan on Aug 20 2022, 6:55 AM.

Details

Summary

This patch adds cost model for vector compare and select instructions. For vector FP compare instruction, it only add the comparisions supported natively.

Diff Detail

Event Timeline

jacquesguan created this revision.Aug 20 2022, 6:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2022, 6:55 AM
jacquesguan requested review of this revision.Aug 20 2022, 6:55 AM

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".

Rebase to pre-commit test.

Early exit when do not support correspond fp vector type.

jacquesguan added inline comments.Aug 29 2022, 4:48 AM
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.
The way the base function calculates the cost (ScalarizeCost + ElementNum * ScalarCost for fixed vector, InvalidCost for scalable vector) is right. But the cost of scalar fp compare instruction is also wrong (now is alway 1, but actually some need external call and some need more instruction) for RISCV, so we still get wrong result now.
I leave a TODO for scalar compare cost, need to create a cost list maybe.

llvm/test/Analysis/CostModel/RISCV/rvv-cmp.ll
2

Done.

reames requested changes to this revision.Aug 29 2022, 11:14 AM

Please rebase after landing the parts I asked you to separate and land previously.

This revision now requires changes to proceed.Aug 29 2022, 11:14 AM

Please rebase after landing the parts I asked you to separate and land previously.

Could you give https://reviews.llvm.org/D132827 approved, so that I can land the test first.

Please rebase after landing the parts I asked you to separate and land previously.

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.

Split, land integer part, then rebase main.

Please rebase after landing the parts I asked you to separate and land previously.

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.

Thanks, I got it.

reames added inline comments.Aug 31 2022, 2:40 PM
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:
if (has appropriate type clause) {

switch (VecPred)
case OneOfMany:
case TwoOfMany:
...
case NOfMany:
  return LT.first * 1;
};

}

Specifically, let all of the unhandled cases fall through.

Address comment.

jacquesguan added inline comments.Sep 6 2022, 11:10 PM
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
739–758

Done.

740

I changed to switch form.

reames accepted this revision.Sep 12 2022, 12:06 PM

LGTM

This revision is now accepted and ready to land.Sep 12 2022, 12:06 PM
This revision was landed with ongoing or failed builds.Sep 12 2022, 11:45 PM
This revision was automatically updated to reflect the committed changes.