8386861: [lworld] emit_opSubstitutabilityCheck speedup

[PeyaPeyaPeyang]

When left == right is known at code generation time, the operands are necessarily the same oop, so we can jump directly to L_oops_equal.
This patch changes both x86 and aarch65 to do that.

I also considered emitting equal_result and returning early in that case.
I decided not to do that because LIR_OpSubstitutabilityCheck::emit_code() still appends the slow-path stub after emit_opSubstitutabilityCheck() returns.
An early return here would skip binding op->stub()->continuation(), leaving the appended stub with an unbound continuation label.

---

• I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (1 review required, with at least 1 Committer)

Issue

• JDK-8386861: [lworld] emit_opSubstitutabilityCheck speedup (Enhancement - P4)

Reviewers

• Martin Doerr (@TheRealMDoerr - no project role)
• Christian Hagedorn (@chhagedorn - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/2628/head:pull/2628
$ git checkout pull/2628

Update a local copy of the PR:
$ git checkout pull/2628
$ git pull https://git.openjdk.org/valhalla.git pull/2628/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2628

View PR using the GUI difftool:
$ git pr show -t 2628

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/2628.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back dyama! A progress list of the required criteria for merging this PR into lworld will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@PeyaPeyaPeyang This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8386861: [lworld] emit_opSubstitutabilityCheck speedup

Reviewed-by: mdoerr, chagedorn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 84 new commits pushed to the lworld branch:

• 6366ebf: 8387703: [lworld] address batch of comments in mainline PR
• a76b340: 8387661: [lworld] move generate_return_value_stub() to SharedRuntime
• 52cfb4e: 8387405: [lworld] is_always_flat_in_array should be always false
• ... and 81 more: https://git.openjdk.org/valhalla/compare/73d93a286199033f3325fe6c8d290b0e6636951c...lworld

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@chhagedorn) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[mlbridge]

Webrevs

• 03: Full - Incremental (91012afd)
• 02: Full - Incremental (3d82eb0b)
• 01: Full - Incremental (1f34ef08)
• 00: Full (a98300d7)

[chhagedorn]

Thanks for picking this up. I have a general question about this patch: Couldn't we already check whether left.result() and right.result() refer to the same LIR operand earlier in LIRGenerator::substitutability_check_common() before creating LIR_OpSubstitutabilityCheck?

valhalla/src/hotspot/share/c1/c1_LIRGenerator.cpp

Lines 3379 to 3381 in a918cc9

	CodeStub* slow_path = new SubstitutabilityCheckStub(left.result(), right.result(), info);
	__ substitutability_check(result, left.result(), right.result(), equal_result, not_equal_result,
	left_klass, right_klass, tmp1, tmp2, info, slow_path);

[TheRealMDoerr]

Why not

  if (left == right) {
    move(op->equal_result(), op->result_opr());
    return;
  }

?

[PeyaPeyaPeyang]

Thanks @chhagedorn and @TheRealMDoerr for pointing that out. You are absolutely right.
I initially avoided handling this there
because I thought the change might become larger, but after looking into again,
it turns out to be quite small.
I’ll update the patch. Thanks again.

[TheRealMDoerr]

Good idea. Are the platform specific parts still needed after this?

[PeyaPeyaPeyang]

You are right, they are not needed any more after this, I think.
Thanks for pointing out..!

This is not a large change, so I'll clean up the
platform-specific codes with this PR as well.

[TheRealMDoerr]

LGTM. Thanks for improving it!

[PeyaPeyaPeyang]

/integrate

[openjdk]

@PeyaPeyaPeyang This pull request has not yet been marked as ready for integration.

[PeyaPeyaPeyang]

Hello @chhagedorn

I fear I was rather too hasty and nearly integrated this.
I've applied your suggestion and moved this to LIRGenerator::substitutability_check_common().
Could you kindly take another look?

Many thanks!

[chhagedorn]

Thanks for the update! That looks good to me, too. Let me give this a spin in our testing.

Just to let you know: For the mainline pre-integration testing, we are temporarily holding off on further integrations. We still need to decide whether follow-up changes should go into a separate Valhalla branch or be integrated into mainline afterward. We will get back to you later this week with further guidance.

[PeyaPeyaPeyang]

@chhagedorn

Thanks for the review and the heads-up!
I'll hold off integrating until I hear back from you.

[chhagedorn]

For non-blocking issues like this, we should hold off from integration and then re-base the PR to mainline after Valhalla integration.

Btw, testing looked good but we should rerun testing once we are in mainline.