8379667: C2: Deep recursion with cmovP_regNode::bottom_type

[merykitty]

Hi,

The issue that leads to the failure is in the implementation of cmoveP_regNode::bottom_type, which recursively invokes Node::bottom_type on its inputs:

const Type            *bottom_type() const { const Type *t = in(oper_input_base()+1)->bottom_type(); return (req() <= oper_input_base()+2) ? t : t->meet(in(oper_input_base()+2)->bottom_type()); } // CMoveP

This deep recursion results in the number of Type::meet being called being a quadratic function of the number of cmovP_regNode in the chain. Type::meet eventually calls TypeInterfaces::intersection_with, each time that method is called, it allocates a GrowableArray to store the _interfaces of the result, this allocation is not guarded with a ResourceMark, which results in the resource area being overloaded, hitting the MemLimit.

Simply adding a ResourceMark to TypeInterfaces::intersection_with does not work, the recursion is still expensive, and the compilation won't terminate in a reasonable amount of time. As a result, I go with a more comprehensive solution, that is to keep the bottom_type of all MachNode.

It also helps alleviate a potential issue. The type of a pointer needs to be correctly recorded for scheduling and builing of oop maps at safepoints. For this, we have a pretty ad-hoc list of MachTypeNode that store their bottom_type inside their instances. By making all MachNode keeping their bottom_type, we can remove this ad-hoc list.

Testing:

• tier1-4,hs-comp-stress
• tier5-7
• tier8,9 - Linux-x64-fastdebug

Please take a look and leave your reviews, thanks a lot.

---

• 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 (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

• JDK-8379667: C2: Deep recursion with cmovP_regNode::bottom_type (Bug - P2)

Reviewers

• Anton Seoane Ampudia (@anton-seoane - Author)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 30765

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back qamai! 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]

@merykitty The following label will be automatically applied to this pull request:

• hotspot-compiler

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

[mlbridge]

Webrevs

• 00: Full (22f5a4cb)

[openjdk]

The total number of required reviews for this PR has been set to 2 based on the presence of this label: hotspot-compiler. This can be overridden with the /reviewers command.

[anton-seoane]

This looks nice @merykitty! It's great to see such a cleanup too :)
I just have a couple questions:

[anton-seoane]

What do you mean by "unsound" here? Can you create a follow-up to track this?

[merykitty]

Currently, in the ideal graph, a PrefetchAllocationNode is a Type::ABIO or Type::MEMORY. As a result, they are connected to previous and subsequent abio and memory nodes, respectively. However, when lowered to MachNodes, since the rule are declared match(PrefetchAllocation mem), they become Type::BOTTOM, but their inputs and outputs are still abio or memory nodes, hence the unsoundness.

With the PR, at least when AllocatePrefetchStyle != 3, they preserve their correct type after matching. For AllocatePrefetchStyle == 3, it is more difficult, because memory nodes participate in anti-dependency computations, so we need to make further modifications to ensure that a PrefetchAllocationNode has a control input, and they act as a full store (its anti-dependencies do not need computing).

I filed JDK-8382547.

[anton-seoane]

I see, thanks!

[anton-seoane]

Your change seems to leave out this printout we would get before. I'm unsure about if keeping it makes easier to read/parse the dump spec or more cluttered, though...

[merykitty]

You mean the st->print(" null")? Since most nodes have bottom_type set, and the one that does not is either MachTemp, or something that is not a pointer (guaranteed by MachNode::bottom_type()), I think it is a little unnecessary.

[anton-seoane]

Looks good. Thanks!