This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Add instruction costs for FP_TO_UINT and FP_TO_SINT with half types
ClosedPublic

Authored by david-arm on Apr 6 2021, 3:18 AM.

Details

Summary

We were missing some instruction costs when converting vectors of
floating point half types into integers. This patch adds more costs
to the table in getCastInstrCost and updates an existing test:

Analysis/CostModel/AArch64/sve-fptoi.ll

Diff Detail

Event Timeline

david-arm created this revision.Apr 6 2021, 3:18 AM
david-arm requested review of this revision.Apr 6 2021, 3:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2021, 3:18 AM
david-arm updated this revision to Diff 337768.Apr 15 2021, 8:06 AM
  • After private discussions with @sdesmalen we realised that some of these costs were wrong, so I went back and manually generated assembly for all cases and chose costs according to the number of instructions needed.
david-arm added inline comments.Apr 19 2021, 3:20 AM
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
533

After private discussion with @sdesmalen it turns out we shouldn't need entries for illegal -> illegal types as they will both be split in the BasicTTIImpl::getCastInstrCost function.

david-arm updated this revision to Diff 338465.Apr 19 2021, 3:23 AM
  • Removed costs for illegal -> illegal type conversions.

Hey @david-arm,
Just a comment, it is me or this table is not well suited to review?
It lists the types by their cost. It should have put together instruction/types instead of instruction/cost.
I see some types/sizes missing like nxv8i32 ->nxv8f32 and nxv4i64 -> nxv4f64, but I guess they are not legal types, or I could miss it, as this table does not help me to find the types.
Is that correct?

llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
579

So much better to check if all types are covered when it is like this.

david-arm updated this revision to Diff 338799.Apr 20 2021, 3:43 AM
  • Tried to tidy up the table a bit more to group all the from type entries together.
david-arm marked an inline comment as done.Apr 20 2021, 3:44 AM
This revision is now accepted and ready to land.Apr 20 2021, 3:52 AM
CarolineConcatto accepted this revision.Apr 20 2021, 5:11 AM

Thank you @david-arm.
I like the way you put the table together now.
It is easier to check the types now/

This revision was landed with ongoing or failed builds.Apr 21 2021, 1:40 AM
This revision was automatically updated to reflect the committed changes.