This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Correct costs for vector ceil/floor/trunc/round
ClosedPublic

Authored by reames on Aug 16 2022, 7:37 AM.

Details

Summary

Add vector costs for ceil/floor/trunc/round. As can be seen in the tests, the prior default costs were a significant under estimate of the actual code generated.

These costs are computed by simply generating code with the current backend, and then counting the number of instructions. I discount one vsetvli, and ignore the return.

These costs were generated by some simple scripting. Assuming we're happy with the code structure, I plan to add a bunch more intrinsics using the same approach (in individual reviews).

Diff Detail

Event Timeline

reames created this revision.Aug 16 2022, 7:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2022, 7:37 AM
reames requested review of this revision.Aug 16 2022, 7:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2022, 7:37 AM
craig.topper added inline comments.Aug 16 2022, 10:53 AM
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
341

Shouldn't it be LT.first * Entry->Cost? We'll pay the cost for each piece of the split.

reames added inline comments.Aug 16 2022, 10:56 AM
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
341

Yep, I copied this from another block of code and apparently didn't give it enough thought. Will fix.

reames updated this revision to Diff 453081.Aug 16 2022, 11:42 AM

Two changes:

  1. Fix the legalization cost Craig pointed out.
  2. Fix a bug with struct types which can't be legalized.
Miss_Grape added inline comments.
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
263

Add v8f16 for floor

Intrinsic::floor, MVT::v8f16, 14
Miss_Grape added inline comments.Aug 17 2022, 2:26 AM
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
291

why set 8? Isn't it 7 ???

reames added inline comments.Aug 17 2022, 7:48 AM
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
263

Half type is not included in this patch at all. It can be added separately.

291

Good catch, I had a bug in my script. I was only discounting the vsetvli if it started with vsetv prefix. This uses the vsetivli variant. Will upload a corrected patch.

reames updated this revision to Diff 453285.Aug 17 2022, 7:53 AM

Discount vsetivli properly.

craig.topper added inline comments.Aug 17 2022, 10:31 AM
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
263

Why is nxv2f32 cheaper than nxv4f32?

craig.topper added inline comments.Aug 17 2022, 10:47 AM
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
263

Ok it's bcecause LMUL>1 generates

vmflt.vv        v11, v12, v8, v0.t
vmv1r.v v0, v11

due to an earlyclobber needed for the narrowing overlap rules.

LMUL <=1 doesn't have the earlyclobber because the overlap would always be "in the lowest-numbered part of the source register group". So. it generates

vmflt.vv        v0, v10, v8, v0.t

FYI, I just broke some of these costs. https://reviews.llvm.org/D132058

@craig.topper Anything holding this back? (Assume I update costs per D132058 if that lands first.)

craig.topper accepted this revision.Aug 19 2022, 9:32 AM

LGTM. The I pushed my patch so the costs need to be bumped.

This revision is now accepted and ready to land.Aug 19 2022, 9:32 AM
This revision was landed with ongoing or failed builds.Aug 19 2022, 10:44 AM
This revision was automatically updated to reflect the committed changes.

Looking at my patch, I thought I increased the number of instructions for every version of ceil and floor, but the cost from your previous revision went from 16 to 15. Am I confused?

Looking at my patch, I thought I increased the number of instructions for every version of ceil and floor, but the cost from your previous revision went from 16 to 15. Am I confused?

As we discussed on that review, you're patch ended up removing 2 vsetvlis. Since I was only discounting 1 of the original three, it's still a net savings of instructions.

Looking at my patch, I thought I increased the number of instructions for every version of ceil and floor, but the cost from your previous revision went from 16 to 15. Am I confused?

As we discussed on that review, you're patch ended up removing 2 vsetvlis. Since I was only discounting 1 of the original three, it's still a net savings of instructions.

I misread the description here and thought you discounted all vsetvlis, not just the first one.