This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add cost model for integer and float vector arithmetic instructions.
AbandonedPublic

Authored by jacquesguan on Sep 8 2022, 11:32 PM.

Details

Summary

This patch implements getArithmeticInstrCost for RISCV, supports cost
model for integer and float vector arithmetic instructions.

Diff Detail

Event Timeline

jacquesguan created this revision.Sep 8 2022, 11:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 8 2022, 11:32 PM
jacquesguan requested review of this revision.Sep 8 2022, 11:32 PM
reames requested changes to this revision.Sep 12 2022, 12:41 PM
reames added inline comments.
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
947

These costs are incorrect when e.g. a half is used without Zfh.

954

This doesn't match the cost of any hardware I'm aware of. You need to justify why this is correct and probably we need some cpu flag if you know of hardware which behaves like this.

This revision now requires changes to proceed.Sep 12 2022, 12:41 PM

Address comment and rebase main.

jacquesguan added inline comments.Sep 16 2022, 1:08 AM
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
947

There is a check above the switch:

if (!LT.second.isVector())
  return BaseT::getArithmeticInstrCost(Opcode, Ty, CostKind, Op1Info, Op2Info,
                                       Args, CxtI);

I think this would early exit if we do not support the element type of the vector.

954

It's a mistake, I changed to SEW related. Or maybe we could just use TCC_Expensive?

reames added inline comments.Nov 22 2022, 7:03 PM
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
1053

You have a bug here - you didn't pass through the last two parameters and thus get the defaults when calling the base class version of the function.

I found this when iterating on a local version of this patch with the intention of subsetting and giving feedback on the remaining part. I went ahead and landed the plumbing for this change as d2cf0bd7

rebase main.

jacquesguan added inline comments.Nov 22 2022, 11:58 PM
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.cpp
1053

OK, thank you.

I landed a slightly cleaned up subset of this in db07d79ab0. I left out the div/rem parts because that makes absolutely no sense to me. If you want to move forward with that piece, please move it to it's own patch rebased over tip of tree and explain why that's a reasonable cost model.

p.s. Please close this review. The auto-close didn't work since I had not commandeered the review.

jacquesguan abandoned this revision.Nov 29 2022, 12:37 AM

Close as most part of this revision was landed by reames.