[ConstraintElim] Add facts implied by llvm.abs #73189

alexander-shaposhnikov · 2023-11-23T00:49:11Z

Add "abs(x) >=s x" fact.

https://alive2.llvm.org/ce/z/gOrrU3

Test plan: ninja check-all
Happy to add more tests covering llvm.abs.

llvmbot · 2023-11-23T00:49:36Z

@llvm/pr-subscribers-llvm-transforms

Author: Alexander Shaposhnikov (alexander-shaposhnikov)

Changes

Add "abs(x) >= x" fact.

https://alive2.llvm.org/ce/z/gOrrU3

Test plan: ninja check-all
Happy to add more tests covering llvm.abs.

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

2 Files Affected:

(modified) llvm/lib/Transforms/Scalar/ConstraintElimination.cpp (+10)
(added) llvm/test/Transforms/ConstraintElimination/abs.ll (+26)

diff --git a/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp b/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
index 7aadd810c1da387..59aaa677ccbf9a9 100644
--- a/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
+++ b/llvm/lib/Transforms/Scalar/ConstraintElimination.cpp
@@ -995,6 +995,11 @@ void State::addInfoFor(BasicBlock &BB) {
       continue;
     }
 
+    if (match(&I, m_Intrinsic<Intrinsic::abs>())) {
+      WorkList.push_back(FactOrCheck::getInstFact(DT.getNode(&BB), &I));
+      continue;
+    }
+
     Value *A, *B;
     CmpInst::Predicate Pred;
     // For now, just handle assumes with a single compare as condition.
@@ -1629,6 +1634,11 @@ static bool eliminateConstraints(Function &F, DominatorTree &DT, LoopInfo &LI,
 
     ICmpInst::Predicate Pred;
     if (!CB.isConditionFact()) {
+      if (Value *X; match(CB.Inst, m_Intrinsic<Intrinsic::abs>(m_Value(X)))) {
+        AddFact(CmpInst::ICMP_SGE, CB.Inst, X);
+        continue;
+      }
+
       if (auto *MinMax = dyn_cast<MinMaxIntrinsic>(CB.Inst)) {
         Pred = ICmpInst::getNonStrictPredicate(MinMax->getPredicate());
         AddFact(Pred, MinMax, MinMax->getLHS());
diff --git a/llvm/test/Transforms/ConstraintElimination/abs.ll b/llvm/test/Transforms/ConstraintElimination/abs.ll
new file mode 100644
index 000000000000000..1cf56c6e37a8c81
--- /dev/null
+++ b/llvm/test/Transforms/ConstraintElimination/abs.ll
@@ -0,0 +1,26 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
+; RUN: opt -passes=constraint-elimination -S %s | FileCheck %s
+
+define i1 @abs_false(i32 %arg) {
+; CHECK-LABEL: define i1 @abs_false(
+; CHECK-SAME: i32 [[ARG:%.*]]) {
+; CHECK-NEXT:    [[ABS:%.*]] = tail call i32 @llvm.abs.i32(i32 [[ARG]], i1 false)
+; CHECK-NEXT:    ret i1 true
+;
+  %abs = tail call i32 @llvm.abs.i32(i32 %arg, i1 false)
+  %cmp = icmp sge i32 %abs, %arg
+  ret i1 %cmp
+}
+
+define i1 @abs_true(i32 %arg) {
+; CHECK-LABEL: define i1 @abs_true(
+; CHECK-SAME: i32 [[ARG:%.*]]) {
+; CHECK-NEXT:    [[ABS:%.*]] = tail call i32 @llvm.abs.i32(i32 [[ARG]], i1 true)
+; CHECK-NEXT:    ret i1 true
+;
+  %abs = tail call i32 @llvm.abs.i32(i32 %arg, i1 true)
+  %cmp = icmp sge i32 %abs, %arg
+  ret i1 %cmp
+}
+
+declare i32 @llvm.abs.i32(i32, i1 immarg)

github-actions · 2023-11-23T00:51:41Z

✅ With the latest revision this PR passed the C/C++ code formatter.

dtcxzyw · 2023-11-23T02:10:58Z

What is the motivation of this patch? I think it is easy to implement this fold in InstSimplify.

alexander-shaposhnikov · 2023-11-29T20:38:53Z

@fhahn - this is indeed not a very frequent case, but the optimization kicked in a few times when i was testing it internally, so I don't have a strong opinion.

fhahn

Thanks for the patch! IMO it is very worthwhile to teach ConstraintElim about more intrinsics, as this can enable additional optimizations by the pass that cannot easily be done by other passes.

@alexander Shaposhnikov Assuming you still have the original motivating case, any chance you could add a more complicated test case that can only be handled by ConstraintElim?

fhahn · 2023-11-29T21:02:22Z

llvm/lib/Transforms/Scalar/ConstraintElimination.cpp

@@ -995,6 +995,11 @@ void State::addInfoFor(BasicBlock &BB) {
      continue;
    }

+    if (match(&I, m_Intrinsic<Intrinsic::abs>())) {


Now that we support a number of intrinsics, we should probably covert that to a switch on the intrinsic ID, so it is easy to see what intrinsics are handled at a glance, as well as making it obvious that they are all handled in the same way

fhahn · 2023-11-30T16:34:21Z

(it also looks like the patch needs clang-format-diff)

alexander-shaposhnikov · 2023-12-03T13:53:38Z

Addressed comments.
P.S. The suggested edit from clang-format seems to be off (perhaps, the bot might be running an outdated version of the tool)

llvm/lib/Transforms/Scalar/ConstraintElimination.cpp

llvm/test/Transforms/ConstraintElimination/abs.ll

alexander-shaposhnikov · 2023-12-04T23:40:17Z

Address comments

fhahn · 2023-12-05T11:50:44Z

llvm/test/Transforms/ConstraintElimination/abs.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
+; RUN: opt -passes=constraint-elimination -S %s | FileCheck %s
+
+define i1 @abs_int_min_is_not_poison(i32 %arg) {


Could you submit the tests separately and also add a version with constant arguments (both positive and negative)?

need to clarify: which constant arguments are you referring to ?

ping @fhahn - happy to send a separate PR with the tests, but need a clarification regarding "constant arguments" - what did you mean

Constants to the llvm.abs calls

llvm/lib/Transforms/Scalar/ConstraintElimination.cpp

alexander-shaposhnikov · 2023-12-25T22:14:27Z

@fhahn - sorry about the delay, just sent a separate PR with tests:
#76374

PR Link: llvm/llvm-project#73189

dtcxzyw · 2023-12-26T03:22:27Z

What is the motivation of this patch? Is this pattern from a real-world application? IMHO we should not add optimizations to improve the "coverage".

alexander-shaposhnikov · 2023-12-26T03:47:55Z

@dtcxzyw - yes, llvm.abs calls originate e.g. from std::abs and this showed up multiple times when i was testing the patch internally.

dtcxzyw · 2023-12-26T03:54:29Z

@dtcxzyw - yes, llvm.abs calls originate e.g. from std::abs and this showed up multiple times when i was testing the patch internally.

Great! Please add a more complicated test that can only be handled by ConstraintElim as @fhahn suggested.
Could you please provide the link to the code repo? I would like to add it into my benchmark :)

alexander-shaposhnikov · 2023-12-26T03:58:44Z

@dtcxzyw , many thanks, actually the suggested test has already been added (GitHub seems to hide resolved comment threads) (see c13819c)

dtcxzyw · 2023-12-26T04:26:55Z

@dtcxzyw , many thanks, actually the suggested test has already been added (GitHub seems to hide resolved comment threads) (see c13819c)

All the test cases should be handled by InstSimplify.

Could you please provide the link to the code repo?

I mean the link to the `real-world application".

alexander-shaposhnikov · 2023-12-26T05:56:32Z

@dtcxzyw

All the test cases should be handled by InstSimplify

The test that involves arithmetic was intentionally made simple. Regarding InstSimplify - unfortunately I would probably disagree with the approach. ConstraintElim is a more general / robust solution and iirc it was introduced for these purposes (as an alternative to expanding the complexity of patterns in InstCombine/InstSimplify), but I'd allow @nikic and @fhahn to weigh in / correct me. I think I've seen a few more cases where ConstraintElim stumbles on intrinsics, this was just the simplest one.

Could you please provide the link to the code repo?

The codebase is internal unfortunately.
I've just checked the list of github issues - it looks like it was independently noticed by other people here #72653

dtcxzyw · 2023-12-26T06:13:06Z

abs(x) <=u x also holds.
Alive2: https://alive2.llvm.org/ce/z/x4fPYp

alexander-shaposhnikov · 2023-12-26T06:48:29Z

P.S. I'd like to expand my previous comment a bit - I've just checked and currently InstSimplify doesn't handle these cases
(if the args are not constant):

./bin/opt -passes=instsimplify -S ~/abs.ll

define i1 @abs_int_min_is_not_poison(i32 %arg) {
  %abs = tail call i32 @llvm.abs.i32(i32 %arg, i1 false)
  %cmp = icmp sge i32 %abs, %arg
  ret i1 %cmp
}

define i1 @abs_int_min_is_poison(i32 %arg) {
  %abs = tail call i32 @llvm.abs.i32(i32 %arg, i1 true)
  %cmp = icmp sge i32 %abs, %arg
  ret i1 %cmp
}

define i1 @abs_plus_one(i32 %arg) {
  %abs = tail call i32 @llvm.abs.i32(i32 %arg, i1 true)
  %abs_plus_one = add nsw i32 %abs, 1
  %cmp = icmp sge i32 %abs_plus_one, %arg
  ret i1 %cmp
}

define i1 @abs_constant_negative_arg() {
  ret i1 true
}

define i1 @abs_constant_positive_arg() {
  ret i1 true
}

; Function Attrs: nocallback nofree nosync nounwind speculatable willreturn memory(none)
declare i32 @llvm.abs.i32(i32, i1 immarg) #0

attributes #0 = { nocallback nofree nosync nounwind speculatable willreturn memory(none) }

To me teaching ConstrainElim about intrinsics seems to be a better idea than putting more patterns into InstSimplify,
but maybe I'm missing something.

fhahn

Thanks for the latest update, looks mostly good, with a few small remaining comments inline and suggestions for follow-up simplifications.

It looks like the code-formatting checks failed, so it would be good to make sure the code is formatted correctly.

The test that involves arithmetic was intentionally made simple. Regarding InstSimplify - unfortunately I would probably disagree with the approach. ConstraintElim is a more general / robust solution and iirc it was introduced for these purposes (as an alternative to expanding the complexity of patterns in InstCombine/InstSimplify), but I'd allow @nikic and @fhahn to weigh in / correct me. I think I've seen a few more cases where ConstraintElim stumbles on intrinsics, this was just the simplest one.

I don't think it's a question of either/or, there should be value in handling it in both. InstCombine/InstSimplify is used at more different points in the pipeline (vs ConstraintElimination running once). The main advantage of support in ConstraintElimination isn't simplifying relatively simple stuff like

  %abs = tail call i32 @llvm.abs.i32(i32 %arg, i1 false)
   %cmp = icmp sge i32 %abs, %arg

but more complicated expressions.

llvm/lib/Transforms/Scalar/ConstraintElimination.cpp

alexander-shaposhnikov · 2023-12-29T07:41:57Z

p.s. clang-format doesn't seem to format "if" properly:

Add tests for llvm.abs. This is a preparation for #73189 Test plan: ninja check-all

fhahn

LGTM, thanks!

p.s. clang-format doesn't seem to format "if" properly:

having multiple statements in an if is very uncommon in the codebase I think and should probably be avoided for consistency.

llvm/lib/Transforms/Scalar/ConstraintElimination.cpp

fhahn · 2023-12-30T10:17:26Z

LGTM, thanks!

p.s. clang-format doesn't seem to format "if" properly:

having multiple statements in an if is very uncommon in the codebase I think and should probably be avoided for consistency.

fhahn

Still LGTM, thanks!

fhahn · 2024-01-08T12:24:26Z

@alexander-shaposhnikov are you planning on working on the suggestions for follow-ups?

alexander-shaposhnikov · 2024-01-08T18:58:25Z

@fhahn - yup, I'm planning to send a follow-up PR ~this week (if it's not urgent)

fhahn · 2024-01-17T10:43:49Z

@fhahn - yup, I'm planning to send a follow-up PR ~this week (if it's not urgent)

Sounds great, just wanted to check, it's not urgent at all.

alexander-shaposhnikov requested review from nikic and fhahn November 23, 2023 00:49

llvmbot added the llvm:transforms label Nov 23, 2023

fhahn reviewed Nov 29, 2023

View reviewed changes

alexander-shaposhnikov force-pushed the handle_abs branch from 3f958f6 to 7ae48d2 Compare December 3, 2023 13:48

alexander-shaposhnikov requested a review from fhahn December 3, 2023 19:33

fhahn reviewed Dec 4, 2023

View reviewed changes

llvm/lib/Transforms/Scalar/ConstraintElimination.cpp Outdated Show resolved Hide resolved

llvm/test/Transforms/ConstraintElimination/abs.ll Show resolved Hide resolved

alexander-shaposhnikov force-pushed the handle_abs branch from 7ae48d2 to c13819c Compare December 4, 2023 23:35

alexander-shaposhnikov requested a review from fhahn December 4, 2023 23:40

fhahn reviewed Dec 5, 2023

View reviewed changes

alexander-shaposhnikov requested a review from fhahn December 6, 2023 20:54

alexander-shaposhnikov mentioned this pull request Dec 25, 2023

[ConstraintElim] Add tests for llvm.abs #76374

Merged

dtcxzyw added a commit to dtcxzyw/llvm-opt-benchmark that referenced this pull request Dec 26, 2023

pre-commit: test PR73189

e2363c4

PR Link: llvm/llvm-project#73189

dtcxzyw mentioned this pull request Dec 26, 2023

pre-commit: test PR73189 dtcxzyw/llvm-opt-benchmark#48

Closed

dtcxzyw mentioned this pull request Dec 26, 2023

[InstCombine] Simplify compare abs(X) and X. #76385

Merged

alexander-shaposhnikov force-pushed the handle_abs branch from c13819c to 79e2989 Compare December 28, 2023 08:33

fhahn reviewed Dec 28, 2023

View reviewed changes

alexander-shaposhnikov force-pushed the handle_abs branch from 79e2989 to dbc9d62 Compare December 29, 2023 07:37

alexander-shaposhnikov added a commit that referenced this pull request Dec 30, 2023

[ConstraintElim] Add tests for llvm.abs (#76374)

1efc0a3

Add tests for llvm.abs. This is a preparation for #73189 Test plan: ninja check-all

alexander-shaposhnikov force-pushed the handle_abs branch from dbc9d62 to eefaeb0 Compare December 30, 2023 06:08

fhahn approved these changes Dec 30, 2023

View reviewed changes

llvm/lib/Transforms/Scalar/ConstraintElimination.cpp Outdated Show resolved Hide resolved

[ConstraintElim] Add facts implied by llvm.abs

0014eaf

alexander-shaposhnikov force-pushed the handle_abs branch from eefaeb0 to 0014eaf Compare January 2, 2024 18:21

fhahn approved these changes Jan 2, 2024

View reviewed changes

alexander-shaposhnikov merged commit 3af59cf into llvm:main Jan 2, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ConstraintElim] Add facts implied by llvm.abs #73189

[ConstraintElim] Add facts implied by llvm.abs #73189

alexander-shaposhnikov commented Nov 23, 2023 •

edited by dtcxzyw

llvmbot commented Nov 23, 2023

github-actions bot commented Nov 23, 2023 •

edited

dtcxzyw commented Nov 23, 2023

alexander-shaposhnikov commented Nov 29, 2023

fhahn left a comment

fhahn Nov 29, 2023

alexander-shaposhnikov Dec 3, 2023

fhahn commented Nov 30, 2023

alexander-shaposhnikov commented Dec 3, 2023

alexander-shaposhnikov commented Dec 4, 2023

fhahn Dec 5, 2023

alexander-shaposhnikov Dec 5, 2023 •

edited

alexander-shaposhnikov Dec 12, 2023

fhahn Dec 12, 2023

alexander-shaposhnikov commented Dec 25, 2023

dtcxzyw commented Dec 26, 2023

alexander-shaposhnikov commented Dec 26, 2023

dtcxzyw commented Dec 26, 2023

alexander-shaposhnikov commented Dec 26, 2023 •

edited

dtcxzyw commented Dec 26, 2023

alexander-shaposhnikov commented Dec 26, 2023 •

edited

dtcxzyw commented Dec 26, 2023

alexander-shaposhnikov commented Dec 26, 2023

fhahn left a comment

alexander-shaposhnikov commented Dec 29, 2023

fhahn left a comment

fhahn commented Dec 30, 2023

fhahn left a comment

fhahn commented Jan 8, 2024

alexander-shaposhnikov commented Jan 8, 2024

fhahn commented Jan 17, 2024

[ConstraintElim] Add facts implied by llvm.abs #73189

[ConstraintElim] Add facts implied by llvm.abs #73189

Conversation

alexander-shaposhnikov commented Nov 23, 2023 • edited by dtcxzyw

llvmbot commented Nov 23, 2023

github-actions bot commented Nov 23, 2023 • edited

dtcxzyw commented Nov 23, 2023

alexander-shaposhnikov commented Nov 29, 2023

fhahn left a comment

Choose a reason for hiding this comment

fhahn Nov 29, 2023

Choose a reason for hiding this comment

alexander-shaposhnikov Dec 3, 2023

Choose a reason for hiding this comment

fhahn commented Nov 30, 2023

alexander-shaposhnikov commented Dec 3, 2023

alexander-shaposhnikov commented Dec 4, 2023

fhahn Dec 5, 2023

Choose a reason for hiding this comment

alexander-shaposhnikov Dec 5, 2023 • edited

Choose a reason for hiding this comment

alexander-shaposhnikov Dec 12, 2023

Choose a reason for hiding this comment

fhahn Dec 12, 2023

Choose a reason for hiding this comment

alexander-shaposhnikov commented Dec 25, 2023

dtcxzyw commented Dec 26, 2023

alexander-shaposhnikov commented Dec 26, 2023

dtcxzyw commented Dec 26, 2023

alexander-shaposhnikov commented Dec 26, 2023 • edited

dtcxzyw commented Dec 26, 2023

alexander-shaposhnikov commented Dec 26, 2023 • edited

dtcxzyw commented Dec 26, 2023

alexander-shaposhnikov commented Dec 26, 2023

fhahn left a comment

Choose a reason for hiding this comment

alexander-shaposhnikov commented Dec 29, 2023

fhahn left a comment

Choose a reason for hiding this comment

fhahn commented Dec 30, 2023

fhahn left a comment

Choose a reason for hiding this comment

fhahn commented Jan 8, 2024

alexander-shaposhnikov commented Jan 8, 2024

fhahn commented Jan 17, 2024

alexander-shaposhnikov commented Nov 23, 2023 •

edited by dtcxzyw

github-actions bot commented Nov 23, 2023 •

edited

alexander-shaposhnikov Dec 5, 2023 •

edited

alexander-shaposhnikov commented Dec 26, 2023 •

edited

alexander-shaposhnikov commented Dec 26, 2023 •

edited