Page MenuHomePhabricator

[VP] Binary floating-point intrinsics.
ClosedPublic

Authored by simoll on Dec 17 2020, 10:24 AM.

Details

Summary

This patch implements vector-predicated intrinsics on IR level for fadd, fsub, fmul, fdiv and frem.
There operate in the default floating-point environment. We will use constrained fp operand bundles for constrained vector-predicated fp math (D93455).

Diff Detail

Event Timeline

simoll created this revision.Dec 17 2020, 10:24 AM
simoll requested review of this revision.Dec 17 2020, 10:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 17 2020, 10:24 AM
simoll added inline comments.Dec 17 2020, 10:26 AM
llvm/include/llvm/IR/VPIntrinsics.def
174

.. fsub is missing here.. i will supplement it and add a unit test to check the integrity of the VPIntrinsics.def file.

simoll updated this revision to Diff 312772.Dec 18 2020, 5:48 AM
  • This patch is based on D93534 .
  • Added missing vp.fsub.
  • Added constrained fp mapping tests (whether the constrained intrinsic actually exists and has as many metadata params as the VPIntrinsic accepts). This is in preparation of D93455 .
frasercrmck added inline comments.Apr 15 2021, 1:48 AM
llvm/docs/LangRef.rst
18272

I realise this inherits from the documentation of the integer intrinsics, but I was wondering if this can be expressed as being equivalent to <%evl x float> in the following example. Would that be any clearer that the intrinsic is conceptually working on vectors of length %evl isn't actually executing the lanes above %evl (as in %t below)?

18296

addition -> subtraction

18310

same here: addition and maybe add <i_fsub>?

18345

addition -> multiplication

18359

addition -> multiplication

18394

addition -> division

18408

same here

18443

addition -> remainder

18457

addition -> remainder

llvm/include/llvm/IR/VPIntrinsics.def
162

integer -> floating-point?

vkmr added a reviewer: vkmr.Apr 15 2021, 3:44 AM
simoll added inline comments.Apr 19 2021, 1:23 AM
llvm/docs/LangRef.rst
18272

I see what are you getting at with this. I am not sure that introducing novel syntax only for explaining things is really helpful. My take here is that most aren't used to the %evl concept - but it is not a complicated thing per se.

The unit test changes in this patch depend on the cleanup work done in D93534 . Please have a look at that one too!

simoll updated this revision to Diff 340529.Apr 26 2021, 8:01 AM
simoll marked 8 inline comments as done.
simoll edited the summary of this revision. (Show Details)
  • Fixed LangRef copy pasta bugs.
  • Formatting in VPIntrinsics.def
  • NFC
simoll marked an inline comment as done.Apr 26 2021, 8:06 AM

With D93534 out of the way, we should have cleared all hurdles for this one. Where do we stand, comments, LGTMs?

frasercrmck added inline comments.May 11 2021, 8:06 AM
llvm/unittests/IR/VPIntrinsicTest.cpp
22

Is this used?

154

Does this condition no longer hold?

simoll marked 2 inline comments as done.May 12 2021, 1:18 AM
simoll added inline comments.
llvm/unittests/IR/VPIntrinsicTest.cpp
39

This is why we need the sstream include.

83

This assertion is redundantly checked in the GetParamPos test.

Matt added a subscriber: Matt.May 20 2021, 2:51 AM

Any input for this patch? Happy for people to step up for a LGTM ;-)

majnemer added inline comments.
llvm/docs/LangRef.rst
18275

Would it be more general/useful to have the intrinsics take an alternative value just like llvm.masked.load and its passthru operand? You would get identical behavior to your proposal by setting this passthru to undef.

simoll added inline comments.Jun 7 2021, 5:40 AM
llvm/docs/LangRef.rst
18275

My take is this: if there was a passthru parameter we'd still have to optimize/match the op+select into op_with_passthru. So, if the simple op+select idiom (that people are already using, btw) suffices to model passthru why add a redundant parameter/code path?

We had this discussion a while back: https://reviews.llvm.org/D57504#1851456

majnemer added inline comments.Jun 9 2021, 12:02 AM
llvm/docs/LangRef.rst
18275

Ah, I see. Fair enough :)

simoll added inline comments.Jun 9 2021, 1:09 AM
llvm/docs/LangRef.rst
18275

Thanks for following up on your drive-by comment ;-)

craig.topper added inline comments.Jun 9 2021, 2:19 PM
llvm/include/llvm/IR/IntrinsicInst.h
401 ↗(On Diff #340529)

Should these be in D93455 so they aren't unused functions?

404 ↗(On Diff #340529)

These names should be lower case per coding standards.

406 ↗(On Diff #340529)

a exception -> an exception

llvm/unittests/IR/VPIntrinsicTest.cpp
22

This looks duplicated

simoll updated this revision to Diff 351101.Jun 10 2021, 3:09 AM
simoll marked 6 inline comments as done.
simoll edited the summary of this revision. (Show Details)

Removed unused HasRoundingMode, etc functions.
Cleanup.

simoll updated this revision to Diff 351135.Jun 10 2021, 4:53 AM

Make clang-format happy

This revision is now accepted and ready to land.Jun 11 2021, 11:35 AM
This revision was automatically updated to reflect the committed changes.