Skip to content
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

JIT: Expand optRelopImpliesRelop for constants as op1 #102024

Merged
merged 2 commits into from
May 11, 2024

Conversation

jakobbotsch
Copy link
Member

VN does not guarantee that constants in relops are first, so swap them if this isn't the case.

Need this to be able to utilize optRelopImpliesRelop to answer overflow related questions for strength reduction.

VN does not guarantee that constants in relops are first, so swap them
if this isn't the case.

Need this to be able to utilize `optRelopImpliesRelop` to answer
overflow related questions for strength reduction.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 8, 2024
Copy link
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

@jakobbotsch jakobbotsch marked this pull request as ready for review May 10, 2024 17:24
@jakobbotsch
Copy link
Member Author

cc @dotnet/jit-contrib PTAL @EgorBo @AndyAyersMS

Diffs

// Returns:
// True if something was inferred; otherwise false.
//
bool Compiler::optRelopTryInferWithOneEqualOperand(const VNFuncApp& domApp,
Copy link
Member

Choose a reason for hiding this comment

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

If not being handled in this PR, could you log an issue for enabling the same to be done for TYP_SIMD?

We have all the same relops and same transforms that are valid there as well, so it'd be nice to ensure consistency

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not really sure what that would entail. We already give up on relops between floating points because many of these inferences don't hold there.
I think you should open an issue about the inferences you think we should be able to make for these.

Copy link
Member

Choose a reason for hiding this comment

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

Commutative swapping should still apply. That is, x > y is the same as y < x even for floating-point.

What doesn't hold true is that x > y is not the same as !(x <= y) (but only due to NaN, so if we know the inputs aren't NaN it becomes safe)

Copy link
Member

Choose a reason for hiding this comment

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

I'll work on getting an issue open for it

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR doesn't do the kind of general canonicalization of VNs we were discussing on Discord the other day. Since @SingleAccretion pointed out that this came with non-trivial TP considerations, I just decided to do the easy thing and canonicalize right at this point, which is part of RBO's reasoning about inequalities. This PR doesn't really change much except it allows some of that reasoning to apply for a few more cases.

Copy link
Member

@EgorBo EgorBo left a comment

Choose a reason for hiding this comment

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

Regarding swap relops, I think I've seen locally the same diffs If I put it here

// We canonicalize commutative operations.
// (Perhaps should eventually handle associative/commutative [AC] ops -- but that gets complicated...)
if (VNFuncIsCommutative(func))
{
// Order arg0 arg1 by numerical VN value.
if (arg0VN > arg1VN)
{
std::swap(arg0VN, arg1VN);
}
}

Namely, oper is compare and op1 is IsVNConstant (but not handle!) -> swap

@jakobbotsch
Copy link
Member Author

Regarding swap relops, I think I've seen locally the same diffs If I put it here

// We canonicalize commutative operations.
// (Perhaps should eventually handle associative/commutative [AC] ops -- but that gets complicated...)
if (VNFuncIsCommutative(func))
{
// Order arg0 arg1 by numerical VN value.
if (arg0VN > arg1VN)
{
std::swap(arg0VN, arg1VN);
}
}

Namely, oper is compare and op1 is IsVNConstant (but not handle!) -> swap

I think if we want to canonicalize constants we should do it consistently, not just to target this one specific transformation on relops. But it sounds like that takes some more work, so I will leave it for another time.

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

LGTM too

@jakobbotsch jakobbotsch merged commit 7ce5d0f into dotnet:main May 11, 2024
105 of 107 checks passed
@jakobbotsch jakobbotsch deleted the more-rbo branch May 11, 2024 16:57
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
VN does not guarantee that constants in relops are first, so swap them
if this isn't the case. This allows the logic to prove the value of more relops.
@github-actions github-actions bot locked and limited conversation to collaborators Jun 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants