Page MenuHomePhabricator

[RISCV] Add inline expansion for vector ftrunc/fceil/ffloor.
ClosedPublic

Authored by craig.topper on Tue, Nov 9, 11:39 PM.

Details

Summary

This prevents scalarization of fixed vector operations or crashes
on scalable vectors.

We don't have direct support for these operations. To emulate
ftrunc we can convert to the same sized integer and back to fp using
round to zero. We don't need to do a convert if the value is large
enough to have no fractional bits or is a nan.

The ceil and floor lowering would be better if we changed FRM, but
we don't model FRM correctly yet. So I've used the trunc lowering
with a conditional add or subtract with 1.0 if the truncate rounded
in the wrong direction.

There are also missed opportunities to use masked instructions.

Diff Detail

Event Timeline

craig.topper created this revision.Tue, Nov 9, 11:39 PM
craig.topper requested review of this revision.Tue, Nov 9, 11:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Nov 9, 11:39 PM
Herald added a subscriber: MaskRay. · View Herald Transcript

Being myself far from an expert in floating-point, the logic and generated code seem correct to me.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1691

I understand our current RISCVISD::V<op>_VL nodes fall short here because they don't allow mask undisturbed, right?

craig.topper added inline comments.Thu, Nov 11, 12:08 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1691

That or we need to pattern match fadd/fsub+vselect during isel. The other complexity is that if we use RISCVISD::V<op>_VL here, we have to convert fixed types to scalable in this function. Using target independent nodes avoided that.

To what extent is this generic code that other targets would benefit from?

I'm also not the most comfortable with floating-point. I was wondering if we could prove these transformations with https://alive2.llvm.org/ce/. I tried something simple but it doesn't recognize copysign. That's not to say it's impossible to express that operation in another way, but it's cumbersome.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1714

Can use Src here.

To what extent is this generic code that other targets would benefit from?

I'm also not the most comfortable with floating-point. I was wondering if we could prove these transformations with https://alive2.llvm.org/ce/. I tried something simple but it doesn't recognize copysign. That's not to say it's impossible to express that operation in another way, but it's cumbersome.

I think it is generic unless the target doesn't have integer vectors that match their fp vectors. The other issue might be unsupported FP comparisons, but the comparisons used here are the ones used by C comparison operators.

The algorithm I used is the same as this code from gcc https://godbolt.org/z/Eqjnad9fe but using select instead of branches. Though I did change the cmpnle (which is setugt) to setogt so that it matched the compares RISCV supports and was in a position to allow a masked operation. I don't think the input can be NaN there so changing unordered to ordered shouldn't matter.

I just tried to use alive2 with nsz to avoid caring about the sign of -0.0, but I'm getting a poison value where I don't think I should. https://alive2.llvm.org/ce/z/mtnitt

I just tried to use alive2 with nsz to avoid caring about the sign of -0.0, but I'm getting a poison value where I don't think I should. https://alive2.llvm.org/ce/z/mtnitt

Playing with https://alive2.llvm.org/ce/z/j8SJkv is giving really strange results. It thinks fptosi of -1.25 and -1.0 both give -1, and that -0.0 gives 0, but thinks -0.25 gives poison. That's surely wrong per the definition of fptosi? I could vaguely understand it if it also viewed -0.0 as giving poison, but it doesn't...

I just tried to use alive2 with nsz to avoid caring about the sign of -0.0, but I'm getting a poison value where I don't think I should. https://alive2.llvm.org/ce/z/mtnitt

Playing with https://alive2.llvm.org/ce/z/j8SJkv is giving really strange results. It thinks fptosi of -1.25 and -1.0 both give -1, and that -0.0 gives 0, but thinks -0.25 gives poison. That's surely wrong per the definition of fptosi? I could vaguely understand it if it also viewed -0.0 as giving poison, but it doesn't...

@aqjune or @nlopes can you help with the alive2 behavior here?

Address review comment

jrtc27 added a comment.EditedThu, Nov 11, 9:48 AM

I just tried to use alive2 with nsz to avoid caring about the sign of -0.0, but I'm getting a poison value where I don't think I should. https://alive2.llvm.org/ce/z/mtnitt

Playing with https://alive2.llvm.org/ce/z/j8SJkv is giving really strange results. It thinks fptosi of -1.25 and -1.0 both give -1, and that -0.0 gives 0, but thinks -0.25 gives poison. That's surely wrong per the definition of fptosi? I could vaguely understand it if it also viewed -0.0 as giving poison, but it doesn't...

@aqjune or @nlopes can you help with the alive2 behavior here?

It's also mishandling fptoui; (-1.0, -0.0] are all legal inputs despite being negative as the result is only poison if the truncated value can't be represented, but 0 can be (see footnote 50 of the C99 spec in 6.3.1.4 which explicitly calls this out; the C semantics are intended to be directly mapped onto fpto[us]i), but it views even fptoui -0.0 as being poison, not 0.

Hello all,
I couldn't find this mail because it was somehow buried in other mails.

I made a pull request that fixes the issue as https://github.com/AliveToolkit/alive2/pull/767 .

craig.topper planned changes to this revision.Mon, Nov 29, 4:25 PM

Here's a new alive2 proof for the trunc. This patch does need to be changed to insert an ISD::FREEZE on the input.

https://alive2.llvm.org/ce/z/8YCDfb

Here's the alive2 for ceil https://alive2.llvm.org/ce/z/zgLLd5 it times out the web, but I ran it through alive2 on my local machine.

Thanks, Craig. From what I can see, taking the alive2 for ceil and modifying it to what you've got for floor also times out rather than finding a verification error. If it works for you locally I'm happy.

Thanks, Craig. From what I can see, taking the alive2 for ceil and modifying it to what you've got for floor also times out rather than finding a verification error. If it works for you locally I'm happy.

I tested this for floor locally and it also passed https://alive2.llvm.org/ce/z/r4zopB

frasercrmck accepted this revision.Wed, Dec 1, 12:11 AM

LGTM but there's one clang-format issue that should be addressed. Thanks, @craig.topper!

This revision is now accepted and ready to land.Wed, Dec 1, 12:11 AM
This revision was landed with ongoing or failed builds.Wed, Dec 1, 11:25 AM
This revision was automatically updated to reflect the committed changes.