Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[VP] Mark llvm.experimental.vp.splice as having no functional equivalent #70647

Merged
merged 1 commit into from
Nov 6, 2023

Conversation

lukel97
Copy link
Contributor

@lukel97 lukel97 commented Oct 30, 2023

llvm.experimental.vp.splice has different semantics from
llvm.experimental.splice (since it takes uses the EVL arguments in a way other
than just masking the tail elements), so it shouldn't be expanded to the
unpredicated version.

Coincidentally there's no support for llvm.experimental.vp.splice in
ExpandVectorPredication, so it wasn't getting expanded, but we shouldn't mark
it as functionally equivalent anyway since there's other users of the property
now e.g. in VectorCombine.

llvm.experimental.vp.splice has different semantics from
llvm.experimental.splice (since it takes uses the EVL arguments in a way other
than just masking the tail elements), so it shouldn't be expanded to the
unpredicated version.

Coincidentally there's no support for llvm.experimental.vp.splice in
ExpandVectorPredication, so it wasn't getting expanded, but we shouldn't mark
it as functionally equivalent anyway since there's other users of the property
now e.g. in VectorCombine.
@llvmbot
Copy link
Member

llvmbot commented Oct 30, 2023

@llvm/pr-subscribers-llvm-ir

Author: Luke Lau (lukel97)

Changes

llvm.experimental.vp.splice has different semantics from
llvm.experimental.splice (since it takes uses the EVL arguments in a way other
than just masking the tail elements), so it shouldn't be expanded to the
unpredicated version.

Coincidentally there's no support for llvm.experimental.vp.splice in
ExpandVectorPredication, so it wasn't getting expanded, but we shouldn't mark
it as functionally equivalent anyway since there's other users of the property
now e.g. in VectorCombine.


Full diff: https://github.com/llvm/llvm-project/pull/70647.diff

1 Files Affected:

  • (modified) llvm/include/llvm/IR/VPIntrinsics.def (+1-1)
diff --git a/llvm/include/llvm/IR/VPIntrinsics.def b/llvm/include/llvm/IR/VPIntrinsics.def
index 55a68ff5768ddb0..0ca60bcfbb3d03f 100644
--- a/llvm/include/llvm/IR/VPIntrinsics.def
+++ b/llvm/include/llvm/IR/VPIntrinsics.def
@@ -699,7 +699,7 @@ VP_PROPERTY_NO_FUNCTIONAL
 END_REGISTER_VP(vp_merge, VP_MERGE)
 
 BEGIN_REGISTER_VP(experimental_vp_splice, 3, 5, EXPERIMENTAL_VP_SPLICE, -1)
-VP_PROPERTY_FUNCTIONAL_INTRINSIC(experimental_vector_splice)
+VP_PROPERTY_NO_FUNCTIONAL
 END_REGISTER_VP(experimental_vp_splice, EXPERIMENTAL_VP_SPLICE)
 
 ///// } Shuffles

@RKSimon
Copy link
Collaborator

RKSimon commented Nov 3, 2023

Should we just drop handling for it for now or is it possible to create a test case?

@lukel97
Copy link
Contributor Author

lukel97 commented Nov 3, 2023

Should we just drop handling for it for now or is it possible to create a test case?

We don't currently handle it in ExpandVectorPredication.cpp to begin with, but I guess we might in future?
I ran through all the users of VPIntrinsic::getFunctionalOpcode()/VPIntrinsic::getFunctionalIntrinsicID()/VPIntrinsic::getConstrainedIntrinsicID() anyway. Looks like the only users are ExpandVectorPredication.cpp and the scalarizeVPIntrinsic combine in VectorCombine.cpp, but the latter only operates on binary ops so it would never have triggered to begin with. So I don't think it's possible to write a failing test case currently.

@RKSimon
Copy link
Collaborator

RKSimon commented Nov 3, 2023

So would we be better off removing it until we have an actual use case?

@lukel97
Copy link
Contributor Author

lukel97 commented Nov 3, 2023

So would we be better off removing it until we have an actual use case?

By "it" do you mean removing the VP_PROPERTY_FUNCTIONAL_INTRINSIC property? Or the actual experimental.vp.splice intrinsic?
We can't remove the former without replacing it with VP_PROPERTY_NO_FUNCTIONAL as of #66199, because now we statically assert that every intrinsic has their functional equivalent marked, if that's what you mean

Copy link
Collaborator

@RKSimon RKSimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation! LGTM

@lukel97 lukel97 merged commit 9627602 into llvm:main Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants