Skip to content

[SCCP] Add context to SimplifyQuery #100831

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 29, 2024
Merged

[SCCP] Add context to SimplifyQuery #100831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion llvm/lib/Transforms/Utils/SCCPSolver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1503,7 +1503,7 @@ void SCCPInstVisitor::visitBinaryOperator(Instruction &I) {
Value *V2 = SCCPSolver::isConstant(V2State)
? getConstant(V2State, I.getOperand(1)->getType())
: I.getOperand(1);
Value *R = simplifyBinOp(I.getOpcode(), V1, V2, SimplifyQuery(DL));
Value *R = simplifyBinOp(I.getOpcode(), V1, V2, SimplifyQuery(DL, &I));
auto *C = dyn_cast_or_null<Constant>(R);
if (C) {
// Conservatively assume that the result may be based on operands that may
Expand Down
21 changes: 21 additions & 0 deletions llvm/test/Transforms/SCCP/float-denormal-simplification.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
; RUN: opt -passes=sccp -S %s | FileCheck %s

define float @test_ieee() #0 {
; CHECK-LABEL: @test_ieee(
; CHECK-NEXT: ret float 0x36F4000000000000
;
%1 = fmul float 2.802596928649634e-44, 2.000000e+00
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer a more meaningful test. We probably shouldn't be making constant folding dependent on the expected FP environment of the context function, and never flush them. This test should at minimum change to use a canonicalize intrinsic

Copy link
Contributor Author

@hashemthomas1 hashemthomas1 Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

My goal was to imitate the tests in: /llvm/test/Transforms/InstSimplify/constant-fold-fp-denormal.ll
since they're minimal and reproduce the "bug" in IPSCCP due to the missing context.
Sorry, but I didn't understand the comment on how it could be more meaningful.
But I do understand your general point of view, my goal for now was to at least make things consistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean passing the context should enable more optimizations than this. Replacing the fmul with a canonicalize should show the same thing, but be more true to the IR rules around where the flush would be guaranteed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood.
I can work on adding tests that show other optimizations enabled by the context.
Replacing fmul with llvm.canonicalize doesn't reproduce the inconsistent behavior because the context was added to simplifyBinOp (canonicalize is handled elsewhere).
But, if you think the tests shouldn't look like this regardless, I'll work on changing them as well 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean passing the context should enable more optimizations than this

After diving more into the code it felt like the amount of optimizations is scarce for two reasons:

  1. The binary operation that reaches SimplifyBinOp in this MR must have at least one operand that has the ValueLatticeElement tag: constant.
  2. Integers has ValueLatticeElement tag: constantrange.

This leaves: floats and vector types as candidates for other optimizations that can appear in the test.

Regarding floats, I didn't find other optimizations that use the context passed to the function for other than deciding to flush the inputs/output. The context was used to find the denormal-fp-math attribute.

Regarding vector types, I don't have much experience in that area, but I couldn't write a test with a missed optimization opportunity in case the context is missing.

Therefore, with my currently limited knowledge of the code in general, I couldn't come up with better tests.

ret float %1
}

define float @test_preserve_sign() #1 {
; CHECK-LABEL: @test_preserve_sign(
; CHECK-NEXT: ret float 0.000000e+00
;
%1 = fmul float 2.802596928649634e-44, 2.000000e+00
ret float %1
}

attributes #0 = {"denormal-fp-math"="ieee,ieee"}
attributes #1 = {"denormal-fp-math"="preserve-sign,preserve-sign"}
Loading