This is an archive of the discontinued LLVM Phabricator instance.

[DAG] Create fptoui.sat from clamped fptosi
ClosedPublic

Authored by dmgreen on Oct 25 2021, 3:33 AM.

Details

Summary

As an extension to D111976, this converts clamp fptosi, clamped between 0 and (2^n)-1 to a fptoui.sat. This can greatly help on targets with conversions that naturally saturate, such as Arm.

X86 disables the transform as some of the test cases increases in size. A fptoui.sat necessitates a fp clamp without native support, so there is little use in converting if the instruction is just going to be expanded.

Diff Detail

Event Timeline

dmgreen created this revision.Oct 25 2021, 3:33 AM
dmgreen requested review of this revision.Oct 25 2021, 3:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 25 2021, 3:33 AM
craig.topper added inline comments.Oct 25 2021, 11:55 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
4857

Can these be const APInt &? They don't seem to be modified.

llvm/lib/Target/X86/X86ISelLowering.h
1085

Formatting looks wrong here

llvm/test/CodeGen/RISCV/fpclamptosat.ll
187

Something weird is happening here. We should be able to fold the slti and beqz together. I'll investigate.

craig.topper added inline comments.Oct 25 2021, 12:51 PM
llvm/test/CodeGen/RISCV/fpclamptosat.ll
187

This an artifact of DAGTypeLegalizer::IntegerExpandSetCCOperands creating a select of setccs. So we end up with a select_cc as part of the condition of another select_cc. Both of the selects get expanded to control flow and tail duplication leaves us with the silly looking code.

dmgreen updated this revision to Diff 382965.Oct 28 2021, 2:44 AM

Thanks - updated as per comments.

SjoerdMeijer accepted this revision.Dec 1 2021, 3:49 AM

Looks like a good change to me. Please wait a day or two with committing this in case @craig.topper has more comments.

This revision is now accepted and ready to land.Dec 1 2021, 3:49 AM
RKSimon added a comment.EditedDec 1 2021, 3:59 AM

I'm happy to take a look at x86 codegen later on - I'm hoping to look at https://llvm.org/PR52581 soon

dmgreen updated this revision to Diff 391063.Dec 1 2021, 9:49 AM

Thanks - that sounds good. I wasn't sure how the X86 converts saturated but it would be nice if they could use this same code. They were just a bit bigger in places at the moment for the unsigned cases.

This is just a rebase to the latest main, now the other parts are in tree.

This revision was landed with ongoing or failed builds.Dec 5 2021, 1:26 AM
This revision was automatically updated to reflect the committed changes.