Page MenuHomePhabricator

[VP][RISCV] Add vp.ceil and RISC-V support
ClosedPublic

Authored by eopXD on Sep 24 2022, 5:53 AM.

Diff Detail

Event Timeline

eopXD created this revision.Sep 24 2022, 5:53 AM
eopXD requested review of this revision.Sep 24 2022, 5:53 AM
craig.topper added inline comments.Sep 24 2022, 9:49 AM
llvm/test/CodeGen/RISCV/rvv/fixed-vectors-ceil-vp.ll
15

This is using the wrong AVL and the mask has been ignored.

eopXD updated this revision to Diff 462717.Sep 25 2022, 4:25 AM

Update code.

craig.topper added inline comments.Sep 25 2022, 8:00 PM
llvm/docs/LangRef.rst
21434

"ceil value" -> ceiling

21448

"ceil value" -> ceiling

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

VP_CEIL is only valid for vectors. MVT::f16 is a scalar.

665

Isn't this already done by line 701 after updating FloatingPointVPOps?

909

Already done by line 919?

1983

Swap the if and else so IsVP doesn't need to be inverted

1984

Use std::tie(Mask, VL) = getDefaultVLOps(VT, ContainerVT, DL, DAG, Subtarget)

eopXD updated this revision to Diff 462816.Sep 25 2022, 11:24 PM
eopXD marked 5 inline comments as done.

Address comments.

eopXD marked 2 inline comments as done.Sep 25 2022, 11:28 PM
craig.topper added inline comments.Sep 26 2022, 9:39 AM
llvm/include/llvm/IR/VPIntrinsics.def
252

This should be VP_FCEIL to match the naming of ISD::FCEIL.

eopXD updated this revision to Diff 462954.Sep 26 2022, 10:03 AM

Address comments.

craig.topper requested changes to this revision.Sep 26 2022, 10:11 AM

Missing test for scalable vectors.

llvm/test/CodeGen/RISCV/rvv/fixed-vectors-vp-ceil.ll
1 ↗(On Diff #462954)

This does not follow the established naming like fixed-vectors-vfadd-vp.ll

This revision now requires changes to proceed.Sep 26 2022, 10:11 AM
eopXD updated this revision to Diff 462962.Sep 26 2022, 10:25 AM
eopXD marked an inline comment as done.

Rename test case and add scalable vector test case.

eopXD marked an inline comment as done.Sep 26 2022, 10:40 AM

Forgive me if I've missed something but you're adding support for widening and splitting in the legalizer without adding tests for that behaviour.

craig.topper added inline comments.Sep 27 2022, 8:13 AM
llvm/docs/LangRef.rst
21448

Remove the word "value". I believe you copied "absolute value" and only changed the word "absolute". "absolute value" is the name of an operation "ceiling value" is not.

Forgive me if I've missed something but you're adding support for widening and splitting in the legalizer without adding tests for that behaviour.

There's a <vscale x 7 x double> <vscale x 16 x double>, <15 x double> and <32 x double> test. Does that cover it?

Forgive me if I've missed something but you're adding support for widening and splitting in the legalizer without adding tests for that behaviour.

There's a <vscale x 7 x double> <vscale x 16 x double>, <15 x double> and <32 x double> test. Does that cover it?

Yes, thanks. To my shame I didn't read through the whole tests before making that comment.

eopXD updated this revision to Diff 463260.Sep 27 2022, 9:23 AM
eopXD marked 2 inline comments as done.

Address comment.

This revision is now accepted and ready to land.Sep 27 2022, 9:27 AM
eopXD updated this revision to Diff 463285.Sep 27 2022, 11:07 AM

Rebase to latest main.

This revision was landed with ongoing or failed builds.Sep 27 2022, 11:08 AM
This revision was automatically updated to reflect the committed changes.
eopXD added a comment.EditedSep 27 2022, 11:13 AM

Received build failure message when generating llvm-sphinx-docs, investigating into this. Patch reverted for now.