Page MenuHomePhabricator

[SLP] Support llvm.isnan in vectorizer
Needs RevisionPublic

Authored by sepavloff on Sep 1 2021, 1:42 AM.

Details

Summary

This change adds llvm.isnan to the set of trivially vectorizable
intrinsic functions. This function differs from other functions by
signature, so processing Call instruction in vectorizeTree was
changed so that overloaded argument types of intrinsic functions were
restored using information from intrinsic description tables.

This change fixes PR51556 (Failure to vectorize llvm.isnan calls).

Diff Detail

Event Timeline

sepavloff created this revision.Sep 1 2021, 1:42 AM
sepavloff requested review of this revision.Sep 1 2021, 1:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2021, 1:42 AM

But there are concerns in https://reviews.llvm.org/D104854 whether the base change should not be reverted at all…

RKSimon requested changes to this revision.Sep 1 2021, 2:47 AM

But there are concerns in https://reviews.llvm.org/D104854 whether the base change should not be reverted at all…

This needs to be addressed first, I regret putting these isnan cost/vectorization tests/bugs in at all atm as I hadn't realised there were still so many open questions about these new fpclassify intrinsics......

llvm/include/llvm/IR/Intrinsics.h
192

The creation of hasBoundType and the refactor of these asserts could be pulled out as a preliminary NFC - independent of this patch. Since you are updating the asserts we should add assert messages as well so that they match the style guidelines.

This revision now requires changes to proceed.Sep 1 2021, 2:47 AM

But there are concerns in https://reviews.llvm.org/D104854 whether the base change should not be reverted at all…

There is a discussion: https://lists.llvm.org/pipermail/llvm-dev/2021-August/152257.html, devoted to this topic. There are no additional feedback during the last week, so I would think the problem is closed.

There are no arguments against the intrinsic, this is the only way to implement isnan, which would satisfy IEEE-754 and C standard in the case when FP exceptions are not ignored.

There were some hesitation whether this intrinsic should be optimized out in fast-math mode. So far there are no sound arguments in favor of such optimization, but there is a reference to the relevant discussion in GCC mail list, which demonstrates difficulties that rise if the effect of fast-math is understood in "naive" way. No matter if we decided optimize out this intrinsic or not, necessity of the intrinsic is not doubted.

But there are concerns in https://reviews.llvm.org/D104854 whether the base change should not be reverted at all…

There is a discussion: https://lists.llvm.org/pipermail/llvm-dev/2021-August/152257.html, devoted to this topic. There are no additional feedback during the last week, so I would think the problem is closed.

Au contraire.
It looks like goalpost shifting to me.
What is going on the is basic design disscussion, with no clear favorite,
even though a design has already been "selected" and committed.
I would like to once again urge you to restart this process.

There are no arguments against the intrinsic, this is the only way to implement isnan, which would satisfy IEEE-754 and C standard in the case when FP exceptions are not ignored.

There were some hesitation whether this intrinsic should be optimized out in fast-math mode. So far there are no sound arguments in favor of such optimization, but there is a reference to the relevant discussion in GCC mail list, which demonstrates difficulties that rise if the effect of fast-math is understood in "naive" way. No matter if we decided optimize out this intrinsic or not, necessity of the intrinsic is not doubted.

But there are concerns in https://reviews.llvm.org/D104854 whether the base change should not be reverted at all…

There is a discussion: https://lists.llvm.org/pipermail/llvm-dev/2021-August/152257.html, devoted to this topic. There are no additional feedback during the last week, so I would think the problem is closed.

Au contraire.
It looks like goalpost shifting to me.
What is going on the is basic design disscussion, with no clear favorite,
even though a design has already been "selected" and committed.
I would like to once again urge you to restart this process.

What exactly do you want me to restart? There is already a discussion on this topic, it could be continued. You could express your concerns there to warm the discussion.

But there are concerns in https://reviews.llvm.org/D104854 whether the base change should not be reverted at all…

There is a discussion: https://lists.llvm.org/pipermail/llvm-dev/2021-August/152257.html, devoted to this topic. There are no additional feedback during the last week, so I would think the problem is closed.

Au contraire.
It looks like goalpost shifting to me.
What is going on the is basic design disscussion, with no clear favorite,
even though a design has already been "selected" and committed.
I would like to once again urge you to restart this process.

What exactly do you want me to restart? There is already a discussion on this topic, it could be continued. You could express your concerns there to warm the discussion.

Revert original patch while there is a ongoing discussion

But there are concerns in https://reviews.llvm.org/D104854 whether the base change should not be reverted at all…

There is a discussion: https://lists.llvm.org/pipermail/llvm-dev/2021-August/152257.html, devoted to this topic. There are no additional feedback during the last week, so I would think the problem is closed.

Au contraire.
It looks like goalpost shifting to me.
What is going on the is basic design disscussion, with no clear favorite,
even though a design has already been "selected" and committed.
I would like to once again urge you to restart this process.

What exactly do you want me to restart? There is already a discussion on this topic, it could be continued. You could express your concerns there to warm the discussion.

Revert original patch while there is a ongoing discussion

There is no an ongoing discussion at present. The discussion was in https://lists.llvm.org/pipermail/llvm-dev/2021-August/152257.html. No essential objections were provided. I would say we reached consensus that llvm.isnan is necessary.

If you have any objection or concern please publish it in the discussion on the mail list.

Matt added a subscriber: Matt.Fri, Sep 24, 1:01 PM