Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[VP] Binary floating-point intrinsics.

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



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

.. 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

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)?


addition -> subtraction


same here: addition and maybe add <i_fsub>?


addition -> multiplication


addition -> multiplication


addition -> division


same here


addition -> remainder


addition -> remainder


integer -> floating-point?

vkmr added a reviewer: vkmr.Apr 15 2021, 3:44 AM
simoll added inline comments.Apr 19 2021, 1:23 AM

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
22 ↗(On Diff #340529)

Is this used?

154 ↗(On Diff #340529)

Does this condition no longer hold?

simoll marked 2 inline comments as done.May 12 2021, 1:18 AM
simoll added inline comments.
41 ↗(On Diff #340529)

This is why we need the sstream include.

83 ↗(On Diff #340529)

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.

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

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:

majnemer added inline comments.Jun 9 2021, 12:02 AM

Ah, I see. Fair enough :)

simoll added inline comments.Jun 9 2021, 1:09 AM

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

craig.topper added inline comments.Jun 9 2021, 2:19 PM

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


These names should be lower case per coding standards.


a exception -> an exception

22 ↗(On Diff #340529)

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.

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.