[Scalarizer] Fix to only scalarize if intrinsic was marked as isTriviallyScalarizable#113625
Merged
[Scalarizer] Fix to only scalarize if intrinsic was marked as isTriviallyScalarizable#113625
Conversation
Member
|
@llvm/pr-subscribers-llvm-transforms Author: Farzon Lotfi (farzonl) Changesfixes #113624 Full diff: https://github.com/llvm/llvm-project/pull/113625.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/Scalarizer.cpp b/llvm/lib/Transforms/Scalar/Scalarizer.cpp
index 772f4c6c35ddec..94e10b707102af 100644
--- a/llvm/lib/Transforms/Scalar/Scalarizer.cpp
+++ b/llvm/lib/Transforms/Scalar/Scalarizer.cpp
@@ -1084,6 +1084,16 @@ bool ScalarizerVisitor::visitExtractValueInst(ExtractValueInst &EVI) {
ValueVector Res;
if (!isStructOfMatchingFixedVectors(OpTy))
return false;
+ if (CallInst *CI = dyn_cast<CallInst>(Op)) {
+ Function *F = CI->getCalledFunction();
+ if (!F)
+ return false;
+ Intrinsic::ID ID = F->getIntrinsicID();
+ if (ID == Intrinsic::not_intrinsic || !isTriviallyScalarizable(ID))
+ return false;
+ // Note: Only proceed if Operand is a`CallInst` and it is defined in `isTriviallyScalarizable`.
+ } else
+ return false;
Type *VecType = cast<FixedVectorType>(OpTy->getContainedType(0));
std::optional<VectorSplit> VS = getVectorSplit(VecType);
if (!VS)
diff --git a/llvm/test/Transforms/Scalarizer/uadd_overflow.ll b/llvm/test/Transforms/Scalarizer/uadd_overflow.ll
new file mode 100644
index 00000000000000..39094451523a5e
--- /dev/null
+++ b/llvm/test/Transforms/Scalarizer/uadd_overflow.ll
@@ -0,0 +1,16 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt %s -passes='function(scalarizer)' -S | FileCheck %s
+
+; Test to make sure that struct return intrinsics that are not `isTriviallyScalarizable` do not get scalarized.
+
+define <3 x i32> @test_(<3 x i32> %a, <3 x i32> %b) {
+; CHECK-LABEL: define <3 x i32> @test_(
+; CHECK-SAME: <3 x i32> [[A:%.*]], <3 x i32> [[B:%.*]]) {
+; CHECK-NEXT: [[R:%.*]] = call { <3 x i32>, <3 x i1> } @llvm.uadd.with.overflow.v3i32(<3 x i32> [[B]], <3 x i32> [[B]])
+; CHECK-NEXT: [[EL:%.*]] = extractvalue { <3 x i32>, <3 x i1> } [[R]], 0
+; CHECK-NEXT: ret <3 x i32> [[EL]]
+;
+ %r = call { <3 x i32>, <3 x i1> } @llvm.uadd.with.overflow.v3i32(<3 x i32> %b, <3 x i32> %b)
+ %el = extractvalue { <3 x i32>, <3 x i1> } %r, 0
+ ret <3 x i32> %el
+}
|
…allyScalarizable fixes llvm#113624
d1e3d20 to
5f9928a
Compare
mariusz-sikora-at-amd
approved these changes
Oct 25, 2024
Contributor
mariusz-sikora-at-amd
left a comment
There was a problem hiding this comment.
Thanks for the fix !
jayfoad
reviewed
Oct 25, 2024
| // Note: Fall through means Operand is a`CallInst` and it is defined in | ||
| // `isTriviallyScalarizable`. | ||
| } else | ||
| return false; |
Contributor
There was a problem hiding this comment.
Will you ever see non-intrinsic calls here? If so, please add a test case for that.
Also nit: the "else" clause should have braces if the "if" clause has braces.
| ValueVector Res; | ||
| if (!isStructOfMatchingFixedVectors(OpTy)) | ||
| return false; | ||
| if (CallInst *CI = dyn_cast<CallInst>(Op)) { |
Contributor
There was a problem hiding this comment.
Can you just handle IntrinsicInst here, instead of manually checking for a known callee and looking up the id?
Member
Author
There was a problem hiding this comment.
Still need to lookup the id, but yes this could be IntrinsicInst.
Closed
NoumanAmir657
pushed a commit
to NoumanAmir657/llvm-project
that referenced
this pull request
Nov 4, 2024
…allyScalarizable (llvm#113625) fixes llvm#113624
Icohedron
added a commit
to Icohedron/llvm-project
that referenced
this pull request
Feb 11, 2025
…able Since uadd_with_overflow is now marked isTriviallyScalarizable, another intrinsic needs to take its place for testing the bug fix introduced in llvm#113625 for issue llvm#113624.
inbelic
pushed a commit
that referenced
this pull request
Feb 13, 2025
Addresses issue #126809 - Made `uadd_with_overflow`, `sadd_with_overflow`, `usub_with_overflow`, `ssub_with_overflow`, `umul_with_overflow`, and `smul_with_overflow` trivially scalarizable in `isTriviallyScalarizable()` from `VectorUtils.cpp` - Renamed and updated the test `Scalarizer/uadd_overflow.ll` to `Scalarizer/uadd_with_overflow.ll` to check that `uadd_with_overflow` gets scalarized - Added a test `Scalarizer/sincos.ll` to ensure the bug fix #113625 still works
joaosaffran
pushed a commit
to joaosaffran/llvm-project
that referenced
this pull request
Feb 14, 2025
) Addresses issue llvm#126809 - Made `uadd_with_overflow`, `sadd_with_overflow`, `usub_with_overflow`, `ssub_with_overflow`, `umul_with_overflow`, and `smul_with_overflow` trivially scalarizable in `isTriviallyScalarizable()` from `VectorUtils.cpp` - Renamed and updated the test `Scalarizer/uadd_overflow.ll` to `Scalarizer/uadd_with_overflow.ll` to check that `uadd_with_overflow` gets scalarized - Added a test `Scalarizer/sincos.ll` to ensure the bug fix llvm#113625 still works
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Feb 24, 2025
) Addresses issue llvm#126809 - Made `uadd_with_overflow`, `sadd_with_overflow`, `usub_with_overflow`, `ssub_with_overflow`, `umul_with_overflow`, and `smul_with_overflow` trivially scalarizable in `isTriviallyScalarizable()` from `VectorUtils.cpp` - Renamed and updated the test `Scalarizer/uadd_overflow.ll` to `Scalarizer/uadd_with_overflow.ll` to check that `uadd_with_overflow` gets scalarized - Added a test `Scalarizer/sincos.ll` to ensure the bug fix llvm#113625 still works
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #113624