Skip to content

[VPlan] Use correct constructor when cloning VPWidenIntrinsicRecipe without underlying CallInst #137493

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

Conversation

lukel97
Copy link
Contributor

@lukel97 lukel97 commented Apr 27, 2025

I noticed this when working on a patch downstream, and I don't think this is an issue upstream yet.

But if a VPWidenIntrinsicRecipe is created without an underlying CallInst, e.g. in createEVLRecipe, it will crash if you try to clone it because it assumes the CallInst always exists.

This fixes it by using the CallInst-less constructor in this case.

@llvmbot
Copy link
Member

llvmbot commented Apr 27, 2025

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-vectorizers

Author: Luke Lau (lukel97)

Changes

I noticed this when working on a patch downstream, and I don't think this is an issue upstream yet.

But if a VPWidenIntrinsicRecipe is created without an underlying CallInst, e.g. in createEVLRecipe, it will crash if you try to clone it because it assumes the CallInst always exists.

This fixes it by using the CallInst-less constructor in this case.


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

1 Files Affected:

  • (modified) llvm/lib/Transforms/Vectorize/VPlan.h (+7-3)
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h
index bd6e15d3fb7a5..925e0b6c68d45 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.h
+++ b/llvm/lib/Transforms/Vectorize/VPlan.h
@@ -1347,9 +1347,13 @@ class VPWidenIntrinsicRecipe : public VPRecipeWithIRFlags, public VPIRMetadata {
   ~VPWidenIntrinsicRecipe() override = default;
 
   VPWidenIntrinsicRecipe *clone() override {
-    return new VPWidenIntrinsicRecipe(*cast<CallInst>(getUnderlyingValue()),
-                                      VectorIntrinsicID, {op_begin(), op_end()},
-                                      ResultTy, getDebugLoc());
+    if (auto *CI = getUnderlyingValue())
+      return new VPWidenIntrinsicRecipe(*cast<CallInst>(CI), VectorIntrinsicID,
+                                        {op_begin(), op_end()}, ResultTy,
+                                        getDebugLoc());
+    else
+      return new VPWidenIntrinsicRecipe(
+          VectorIntrinsicID, {op_begin(), op_end()}, ResultTy, getDebugLoc());
   }
 
   VP_CLASSOF_IMPL(VPDef::VPWidenIntrinsicSC)

…ithout underlying CallInst

I noticed this when working on a patch downstream, and I don't think this is an issue upstream yet.

But if a VPWidenIntrinsicRecipe is created without an underlying CallInst, e.g. in createEVLRecipe, it will crash if you try to clone it because it assumes the CallInst always exists.

This fixes it by using the CallInst-less constructor in this case.
@lukel97 lukel97 force-pushed the loop-vectorize/vpwidenintrinsicrecipe-clone-no-callinst branch from cececf5 to 5ace973 Compare April 27, 2025 05:28
Copy link
Contributor

@fhahn fhahn left a comment

Choose a reason for hiding this comment

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

We generally prefer leaving clone() implementations unimplemented for cases which are never cloned (such as used in EVL only), but in this cases there may be other places that may want to create VPWidenIntrinsicRecipes w/o underlying instruction

LGTM

@lukel97 lukel97 merged commit 92c3af7 into llvm:main Apr 28, 2025
11 checks passed
jyli0116 pushed a commit to jyli0116/llvm-project that referenced this pull request Apr 28, 2025
…ithout underlying CallInst (llvm#137493)

I noticed this when working on a patch downstream, and I don't think
this is an issue upstream yet.

But if a VPWidenIntrinsicRecipe is created without an underlying
CallInst, e.g. in createEVLRecipe, it will crash if you try to clone it
because it assumes the CallInst always exists.

This fixes it by using the CallInst-less constructor in this case.
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
…ithout underlying CallInst (llvm#137493)

I noticed this when working on a patch downstream, and I don't think
this is an issue upstream yet.

But if a VPWidenIntrinsicRecipe is created without an underlying
CallInst, e.g. in createEVLRecipe, it will crash if you try to clone it
because it assumes the CallInst always exists.

This fixes it by using the CallInst-less constructor in this case.
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
…ithout underlying CallInst (llvm#137493)

I noticed this when working on a patch downstream, and I don't think
this is an issue upstream yet.

But if a VPWidenIntrinsicRecipe is created without an underlying
CallInst, e.g. in createEVLRecipe, it will crash if you try to clone it
because it assumes the CallInst always exists.

This fixes it by using the CallInst-less constructor in this case.
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
…ithout underlying CallInst (llvm#137493)

I noticed this when working on a patch downstream, and I don't think
this is an issue upstream yet.

But if a VPWidenIntrinsicRecipe is created without an underlying
CallInst, e.g. in createEVLRecipe, it will crash if you try to clone it
because it assumes the CallInst always exists.

This fixes it by using the CallInst-less constructor in this case.
Ankur-0429 pushed a commit to Ankur-0429/llvm-project that referenced this pull request May 9, 2025
…ithout underlying CallInst (llvm#137493)

I noticed this when working on a patch downstream, and I don't think
this is an issue upstream yet.

But if a VPWidenIntrinsicRecipe is created without an underlying
CallInst, e.g. in createEVLRecipe, it will crash if you try to clone it
because it assumes the CallInst always exists.

This fixes it by using the CallInst-less constructor in this case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants