8372845: C2: Fold identity hash code if object is constant

[liach]

Folding identity hash as constant if the incoming argument is constant would be useful for quick map lookups, such as for the Classifier proposal. Currently, identity hash is not constant because it loads the object header/mark word. We can add an explicit bypass to load an existing hash eagerly instead.

---

Progress

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

Issue

• JDK-8372845: C2: Fold identity hash code if object is constant (Enhancement - P4)

Reviewers

• Vladimir Kozlov (@vnkozlov - Reviewer) 🔄 Re-review required (review applies to 4a82f79d)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28589

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28589.diff

Using Webrev

Link to Webrev Comment

[liach]

/author @iwanowww

[bridgekeeper]

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

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@liach
Setting overriding author to Vladimir Ivanov <[email protected]>. When this pull request is integrated, the overriding author will be used in the commit.

[openjdk]

@liach The following labels will be automatically applied to this pull request:

• graal
• hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 02: Full - Incremental (b1d8be39)
• 01: Full - Incremental (69225241)
• 00: Full (4a82f79d)

[iwanowww]

/label remove graal
/label add hotspot-runtime

[iwanowww]

@liach Thanks for taking care of the fix. Here's a more polished version:
c6c4e9f

[openjdk]

@iwanowww
The graal label was successfully removed.

[openjdk]

@iwanowww
The hotspot-runtime label was successfully added.

[iwanowww]

/contributor add liach

[openjdk]

@iwanowww Only the author (@liach) is allowed to issue the contributor command.

[liach]

I have one question: would it be safer for us to move the constant detection after generate_virtual_guard in the is_virtual if block? I think it may be possible for users to create a Object::hashCode site with a constant receiver that is of a specialized class that overrides hashCode.

[TobiHartmann]

I think it may be possible for users to create a Object::hashCode site with a constant receiver that is of a specialized class that overrides hashCode.

Yes, I think so too. We need a test for this scenario.

Just an observation: This patch will only allow folding during parsing. I would expect that often, opportunities only arise after other optimizations already took place. For example, something like this would not be optimized if we run with -XX:+AlwaysIncrementalInline, right?

    static final Object a = new Object();
    
    @ForceInline
    public Object getter(Object obj) {
        return obj;
    }

    public long test() {
        return getter(a).hashCode();
    }

Another example:

Object val = new Object();
 
int limit = 2;
for (; limit < 4; limit *= 2);
for (int i = 2; i < limit; i++) {
    val = a;
}

return val.hashCode(); // After loop opts, C2 knows that val == a

So ideally, we would move this optimization to IGVN. This would also help Valhalla, where we need to (re-)compute the hashcode for a scalarized value object and would therefore like to fold the computation as aggressively as possible.

[iwanowww]

would it be safer for us to move the constant detection after generate_virtual_guard in the is_virtual if block?

Good catch. I missed that the intrinsic is shared between System::identityHashCode() and Object::hashCode.

I'm not sure it makes sense to support Object::hashCode unless C2 can eliminate generate_virtual_guard for a constant receiver. I'd just limit constant folding to !is_virtual case for now.

[iwanowww]

Just an observation: This patch will only allow folding during parsing. I would expect that often, opportunities only arise after other optimizations already took place.

I deliberately omitted post-parse optimization opportunities for now. It would require a gradual lowering of the representation from a high-level macro node to low-level poking at object header. Moreover, final representation has complex control, so either the macro node should be a CFG node or a way to determine a location in CFG for a data-only macro node and expanding it there needs to be supported. (There are other use cases for such functionality, like lowering data nodes into pure calls, but no readily available implementation is there yet.)

IMO something to work on in a follow-up enhancement.

[iwanowww]

I'm not sure it makes sense to support Object::hashCode unless C2 can eliminate generate_virtual_guard for a constant receiver. I'd just limit constant folding to !is_virtual case for now.

Or, alternatively, inspect constant object's v-table during compilation and ensure that corresponding slot points at Object::hashCode.

[vnkozlov]

Good. Yes, we can work on constant folding in IGVN later.

[vnkozlov]

@iwanowww please fix title to match JBS.

[iwanowww]

@vnkozlov I can't since I'm not the author of the PR :-)

[liach]

I tried to come up with an example where the buggy code from Vladimir would inline to identityHashCode when the right call would be virtual - couldn't construct such a case unfortunately :(

I think we can deal with IGVN later, as this involves creating new macro node and other infrastructure support.

[liach]

/touch

[openjdk]

@liach The pull request is being re-evaluated and the inactivity timeout has been reset.

[openjdk]

@liach hotspot has been added to this pull request based on files touched in new commit(s).