Skip to content

Conversation

Icohedron
Copy link
Contributor

@Icohedron Icohedron commented Feb 11, 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 [Scalarizer] Fix to only scalarize if intrinsic was marked as isTriviallyScalarizable #113625 still works

…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.
@Icohedron Icohedron marked this pull request as ready for review February 11, 2025 23:32
@llvmbot llvmbot added llvm:analysis Includes value tracking, cost tables and constant folding llvm:transforms labels Feb 11, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 11, 2025

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-analysis

Author: Deric Cheung (Icohedron)

Changes

Addresses issue #126809

  • Made uadd_with_overflow trivially scalarizable in isTriviallyScalarizable from VectorUtils.cpp
  • Updated the test Scalarizer/uadd_overflow.ll to check that uadd_with_overflow gets scalarized

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

3 Files Affected:

  • (modified) llvm/lib/Analysis/VectorUtils.cpp (+1)
  • (added) llvm/test/Transforms/Scalarizer/sincos.ll (+17)
  • (modified) llvm/test/Transforms/Scalarizer/uadd_overflow.ll (+12-4)
diff --git a/llvm/lib/Analysis/VectorUtils.cpp b/llvm/lib/Analysis/VectorUtils.cpp
index ad80e458ab57d..97b29cfc3c737 100644
--- a/llvm/lib/Analysis/VectorUtils.cpp
+++ b/llvm/lib/Analysis/VectorUtils.cpp
@@ -125,6 +125,7 @@ bool llvm::isTriviallyScalarizable(Intrinsic::ID ID,
   // https://github.com/llvm/llvm-project/issues/112408
   switch (ID) {
   case Intrinsic::frexp:
+  case Intrinsic::uadd_with_overflow:
     return true;
   }
   return false;
diff --git a/llvm/test/Transforms/Scalarizer/sincos.ll b/llvm/test/Transforms/Scalarizer/sincos.ll
new file mode 100644
index 0000000000000..6510cbc33ccb7
--- /dev/null
+++ b/llvm/test/Transforms/Scalarizer/sincos.ll
@@ -0,0 +1,17 @@
+; 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 <4 x float> @test_(<4 x float> %Val) {
+; CHECK-LABEL: define <4 x float> @test_(
+; CHECK-SAME: <4 x float> [[VAL:%.*]]) {
+; CHECK-NEXT:    [[R:%.*]] = call { <4 x float>, <4 x float> } @llvm.sincos.v4f32(<4 x float> [[VAL]])
+; CHECK-NEXT:    [[EL:%.*]] = extractvalue { <4 x float>, <4 x float> } [[R]], 0
+; CHECK-NEXT:    ret <4 x float> [[EL]]
+;
+  %r = call { <4 x float>, <4 x float> } @llvm.sincos.v4f32(<4 x float>  %Val)
+  %el = extractvalue { <4 x float>, <4 x float> } %r, 0
+  ret <4 x float> %el
+}
+
diff --git a/llvm/test/Transforms/Scalarizer/uadd_overflow.ll b/llvm/test/Transforms/Scalarizer/uadd_overflow.ll
index 39094451523a5..f266e5f08b3f6 100644
--- a/llvm/test/Transforms/Scalarizer/uadd_overflow.ll
+++ b/llvm/test/Transforms/Scalarizer/uadd_overflow.ll
@@ -1,13 +1,21 @@
 ; 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:    [[B_I0:%.*]] = extractelement <3 x i32> [[B]], i64 0
+; CHECK-NEXT:    [[R_I0:%.*]] = call { i32, i1 } @llvm.uadd.with.overflow.i32(i32 [[B_I0]], i32 [[B_I0]])
+; CHECK-NEXT:    [[B_I1:%.*]] = extractelement <3 x i32> [[B]], i64 1
+; CHECK-NEXT:    [[R_I1:%.*]] = call { i32, i1 } @llvm.uadd.with.overflow.i32(i32 [[B_I1]], i32 [[B_I1]])
+; CHECK-NEXT:    [[B_I2:%.*]] = extractelement <3 x i32> [[B]], i64 2
+; CHECK-NEXT:    [[R_I2:%.*]] = call { i32, i1 } @llvm.uadd.with.overflow.i32(i32 [[B_I2]], i32 [[B_I2]])
+; CHECK-NEXT:    [[EL_ELEM0:%.*]] = extractvalue { i32, i1 } [[R_I0]], 0
+; CHECK-NEXT:    [[EL_ELEM01:%.*]] = extractvalue { i32, i1 } [[R_I1]], 0
+; CHECK-NEXT:    [[EL_ELEM02:%.*]] = extractvalue { i32, i1 } [[R_I2]], 0
+; CHECK-NEXT:    [[EL_UPTO0:%.*]] = insertelement <3 x i32> poison, i32 [[EL_ELEM0]], i64 0
+; CHECK-NEXT:    [[EL_UPTO1:%.*]] = insertelement <3 x i32> [[EL_UPTO0]], i32 [[EL_ELEM01]], i64 1
+; CHECK-NEXT:    [[EL:%.*]] = insertelement <3 x i32> [[EL_UPTO1]], i32 [[EL_ELEM02]], i64 2
 ; 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)

Icohedron added a commit to Icohedron/llvm-project that referenced this pull request Feb 11, 2025
This commit depends on changes to be merged from PR llvm#126815
@Icohedron Icohedron changed the title [Scalarizer] Make uadd_with_overflow scalarizable [Scalarizer] Make *_with_overflow intrinsics scalarizable Feb 12, 2025
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.

LGTM with one minor

@inbelic inbelic linked an issue Feb 13, 2025 that may be closed by this pull request
@inbelic inbelic merged commit 37ed2e6 into llvm:main Feb 13, 2025
8 checks passed
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm:analysis Includes value tracking, cost tables and constant folding llvm:transforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Scalarizer] Intrinsic uadd_with_overflow should be scalarizable
5 participants