This is an archive of the discontinued LLVM Phabricator instance.

[CostModel] Add basic fptoi_sat costs
ClosedPublic

Authored by dmgreen on Apr 22 2022, 9:01 AM.

Details

Summary

This adds some basic fptosi_sat and fptoui_sat target independent cost modelling. The fptosi_sat is modelled as a fmin/fmax to saturate the value, followed by a fp convert. The signed values then have an additional fcmp+select for handling Nan correctly.

The AArch64/Arm costs may be more incorrect, as the instruction exist natively. This can be fixed with target specific cost updates.

Diff Detail

Event Timeline

dmgreen created this revision.Apr 22 2022, 9:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2022, 9:01 AM
dmgreen requested review of this revision.Apr 22 2022, 9:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2022, 9:01 AM
RKSimon added inline comments.Apr 23 2022, 3:53 AM
llvm/test/Analysis/CostModel/X86/fptoi_sat.ll
11

Drop the AVX prefix here as well

dmgreen updated this revision to Diff 424818.Apr 24 2022, 11:04 PM

Remove extra AVX.

You still need to get rid of the AVX checks - the script won't touch them once they aren't mentioned in any check-prefixes lists

llvm/test/Analysis/CostModel/X86/fptoi_sat.ll
735

These need stripping now

Hi @dmgreen, just out of curiosity why do we only care about NaNs for the signed case? Looking at the documentation for llvm.fptosi.sat and llvm.fptoui.sat they both state that "if any argument is NaN, zero is returned". So don't you have to do a fcmp+select in both cases?

dmgreen updated this revision to Diff 424912.Apr 25 2022, 8:04 AM

Hi @dmgreen, just out of curiosity why do we only care about NaNs for the signed case? Looking at the documentation for llvm.fptosi.sat and llvm.fptoui.sat they both state that "if any argument is NaN, zero is returned". So don't you have to do a fcmp+select in both cases?

The code that creates it is here: https://github.com/llvm/llvm-project/blob/5ad07ac400dad1cbc7c7c0a5e6325165da993fb1/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp#L9175. The short version is that the unsigned case has clamped the minimum to 0 already. For the signed version 0 is in the middle of its range so needs an extra check.

llvm/test/Analysis/CostModel/X86/fptoi_sat.ll
735

Oh sorry, I was on autopilot a bit, with how many different check lines there are.
I've rebased them onto what is hopefully a better baseline.

RKSimon accepted this revision.Apr 25 2022, 8:58 AM

LGTM - cheers

This revision is now accepted and ready to land.Apr 25 2022, 8:58 AM
This revision was landed with ongoing or failed builds.Apr 27 2022, 1:30 AM
This revision was automatically updated to reflect the committed changes.