This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Lower fpto*i.sat intrinsics.
ClosedPublic

Authored by jbramley on May 12 2021, 12:15 PM.

Details

Summary

AArch64's fctv* instructions implement the saturating behaviour that the
fpto*i.sat intrinsics require, in cases where the destination width
matches the saturation width. Lowering them removes a lot of unnecessary
generated code.

Only scalar lowerings are supported for now.

Diff Detail

Event Timeline

jbramley created this revision.May 12 2021, 12:15 PM
jbramley requested review of this revision.May 12 2021, 12:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2021, 12:15 PM
nikic added a subscriber: nikic.May 12 2021, 12:52 PM

For the record, there is also the somewhat stale D86078 for this.

For the record, there is also the somewhat stale D86078 for this.

Thanks for pointing that out. From a quick look, the author of that patch, @ebevhan, hasn't been active since Oct. 2020, so I am assuming this work can best be continued here.

For the record, there is also the somewhat stale D86078 for this.

Thanks, I wasn't aware of that. At a glance, it looks like that patch implements DstWidth != SatWidth and vector types; I left that as a TODO, hoping to seek feedback on the overall approach first. I think they'll be fairly clean incremental changes but I can roll them into this patch if that's preferred.

By the way, the use-case for this is Rust, which has well-defined sematics for FP-to-integer casts so this becomes a fairly common operation.

SjoerdMeijer added inline comments.May 13 2021, 1:11 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
3304

Nit: I don't see precedent for checking opcodes again; I think this assert can be removed.

3315

Nit: "must be smaller or equal ...", or "cannot exceed ..."

3318

The other patch was doing this:

if (SrcVT.isVector()) {
  SDValue Vec = LowerVectorFP_TO_INT(Op, DAG);
  if (Vec != Op)
    return Vec;
}

Haven't looked into this, but does that make sense? Can we reuse that?

3322

Nit: the coding style says that we don't need brackets around ifs if they contain one statement. So to save some space we can remove the brackets and place the TODO before the if.

3328

Nit: remove the brackets, and just use dl(Op) directly as an argument.

3336

Same nit, don't need the brackets.

16025

Nit: the comment makes sense, I am not sure it belongs here though.

llvm/test/CodeGen/AArch64/round-fptosi-sat-scalar.ll
44

I haven't checked, but we don't have a f16 -> i32 variant of this?

195

A trunc from a f16 to a f16 should be a no-op? Do we need this? I see similar patterns below, so I must be missing something...

Thanks, I wasn't aware of that. At a glance, it looks like that patch implements DstWidth != SatWidth and vector types; I left that as a TODO, hoping to seek feedback on the overall approach first. I think they'll be fairly clean incremental changes but I can roll them into this patch if that's preferred.

I think the TODO is fine, and that could be addressed with a follow up.

jbramley added inline comments.May 13 2021, 1:59 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
3318

The other patch also extends LowerVectorFP_TO_INT to handle the saturating semantics. It's probably a reasonable approach, and didn't see any objections there.

llvm/test/CodeGen/AArch64/round-fptosi-sat-scalar.ll
44

Isn't that @llvm.fptosi.sat.i32.f16, in @testmswh, above?

195

We need it because trunc truncates to an integer, leaving the result in the same FP format (like frintz in the fallback machine instruction sequence). It's not a no-op.

All of the tests in this file are round + convert sequences, like round.conv.ll. I added other tests to fptosi-sat-scalar.ll (etc) for the simple, standalone conversions.

SjoerdMeijer added inline comments.May 13 2021, 2:11 AM
llvm/test/CodeGen/AArch64/round-fptosi-sat-scalar.ll
44

Yep, sorry, missed that!

195

Ah okay, thanks, and I see now. I read this too quickly.

jbramley marked 10 inline comments as done.May 13 2021, 6:27 AM
jbramley added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
16025

Agreed. It probably makes more sense near the setTargetDAGCombine configuration in AArch64TargetLowering.

jbramley marked an inline comment as done.May 13 2021, 6:52 AM
jbramley added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.h
960

Can I just ignore these? Matching the de-facto convention seems appropriate, but I think these are responsible for the reported build failure.

SjoerdMeijer added inline comments.May 13 2021, 7:00 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.h
960

Yep, this is an unhelpful message in this case; this is just following the convention/style in this file so is alright.
I had a quick look, but am not sure what the cause of the failure is as it seems all regression tests are passing, but somewhere it is doing an Exit 1. I would ignore this and blame it on a glitch or flaky bot.

jbramley updated this revision to Diff 345143.May 13 2021, 7:56 AM
jbramley marked an inline comment as done.

Addressed review comments.

This addresses all review comments, except for the one about SrcVT.isVector(),
which I think we agreed is Ok as a follow-up patch.

SjoerdMeijer accepted this revision.May 13 2021, 12:24 PM

This LGTM as a first version.

This revision is now accepted and ready to land.May 13 2021, 12:24 PM

Thanks!

Could someone commit it for me please? I don't have access myself.

Thanks!

Could someone commit it for me please? I don't have access myself.

Happy to commit this on your behalf, but requesting an account should be straightforward so that you can commit it yourself. That might be convenient because it sounds like you have some follow up patches in the pipeline.

This revision was landed with ongoing or failed builds.May 17 2021, 2:20 AM
This revision was automatically updated to reflect the committed changes.