BenefitInliner phase 2/3 Classes for building the IDT.

[xiacijie]

Issue: #5488

[xiacijie]

This is the phase#2 out of 3 of the BenefitInliner contribution.

This contribution consists of three commits:

1. Inlining method summary.
2. Inlining dependency tree (IDT).
3. IDT Builder (to be pushed after 1. and 2. are reviewed )

Inlining method summary and IDT are the helper classes so I believe they should be reviewed first.
@aviansie-ben Ben, if you are still on this work, could you have a look at the helper classes? Do they need to be unit tested?
@Leonardo2718 Leo, if you are still on this, could you start the reviewing? Much appreciated!

[jdmpapin]

I'm not sure what is meant by "commit history will be fixed later" in the commit titles, other than possibly that the commit messages will be cleaned up? The structure of the PR as a series of two commits is good

[jdmpapin]

Please make sure to expand the "hidden conversations," i.e. the review comments that GitHub hides by default for some reason

[jdmpapin]

In the future, would you mind commenting after you force push? If there are multiple force pushes in a row without any intervening activity, GitHub loses earlier force push diffs

The comment is also an opportunity to describe/clarify what a particular push is doing

[jdmpapin]

In addition to the review comments just above, a number of my earlier comments have not been addressed, at least not fully:

• I thought that we had agreed in discussions with @karimhamdanali and @dinolii that we would split the summary optimization conditions into positive and negative cases. This PR has been partially updated in a way that prevents the summary from expressing the possibility of folding instanceof to false. Is this distinction still coming? And supposing it is, will this PR be updated to make the distinction, or will that be separate follow-up work?
• IDT::Print needs to use TR_VerboseLog::CriticalSection (which I wrote specifically because of this PR)
• The name IDTPreorderPriorityQueue is still misleading, and the comment doesn't clarify it much
• The stray semicolon is still there

[mingweiarthurli]

In addition to the review comments just above, a number of my earlier comments have not been addressed, at least not fully:

* [ ]  I thought that we had agreed in discussions with @karimhamdanali and @dinolii that we would [split the summary optimization conditions into positive and negative cases](https://github.com/eclipse/omr/pull/5508#discussion_r677669681). This PR has been partially updated in a way that [prevents the summary from expressing the possibility of folding `instanceof` to false](https://github.com/eclipse/omr/pull/5508/#discussion_r749527842). Is this distinction still coming? And supposing it is, will this PR be updated to make the distinction, or will that be separate follow-up work?

* [ ]  [`IDT::Print` needs to use `TR_VerboseLog::CriticalSection`](https://github.com/eclipse/omr/pull/5508/#discussion_r751647306) (which I wrote specifically because of this PR)

* [ ]  The name [`IDTPreorderPriorityQueue` is still misleading](https://github.com/eclipse/omr/pull/5508/#discussion_r749592033), and the comment doesn't clarify it much

* [ ]  The [stray semicolon](https://github.com/eclipse/omr/pull/5508#discussion_r749592610) is still there

• According to my colleague's work, they have splited the VPClassType and VPClass into two separate cases and now the summary optimization conditions are separated by the testing for different foldings. I have newly implemented the VPClassType part by using TR_FrontEnd::isInstanceOf(). A explanation has been replied right below the reviewer's comments for that part of code.
• All vlogAcquire()/vlogRelease() in IDT::Print has been replaced by TR_VerboseLog::CriticalSection().
• Name of IDTPreorderPriorityQueue has been changed to IDTPriorityQueue. A detailed comment has been added. This is an unused class for any future development. There has been some misunderstanding during the discussion and I will reply a further explanation below the reviewer's comment later for this problem. The further explanation has been added now.
• All unnecessary characters, typo and improper messages have been fixed.

[karimhamdanali]

hey @jdmpapin , I was wondering if you had a chance to look at the most recent changes that we have made to this PR. We would greatly appreciate your feedback so that we can move on to working on Phase 3 (and the final one) of the contribution. Thanks!

[jdmpapin]

Sorry for the delay! I'll start reviewing this today, though I won't necessarily finish today. You can expect to see my comments in the next few business days

[karimhamdanali]

Thanks @jdmpapin !

[jdmpapin]

The commit series in this PR has gotten to be quite a mess. It has lots of little commits changing this or that based on review feedback, and multiple merges from upstream. Furthermore, the following commits are unrelated to this PR and have been inadvertently included in it as a result of merging from the openj9-omr 0.36 release branch:

• bff33fc
• 2bb0413
• 625ff55
• f491bbf

My request is that you please clean this up before making any further changes (including in response to my current review comments). I don't think this PR needs to be more than a single commit. Here's how to do so:

1. Make sure you're on the jack-inliner-phase-2 branch, checking it out if necessary.
2. Check that your local branch is at the same commit as this PR. git rev-parse HEAD should show 517a7198155903f909e115de3d56756048db5616.
3. Run git reset --hard c766c4155; This is the most recent commit shared by your PR branch and the upstream master branch.
4. Run git checkout 517a71981 -- .; This updates your working tree and staging area with the exact file contents as they currently exist in this PR.
5. Create the new (clean) commit: git commit; At the end of the message, please list all prior contributors as co-authors using Co-authored-by trailer lines. Don't include any blank lines in the message after the co-authors. 68b4570 and 334f9c8 are examples of properly formatted multi-author commits.
6. Force-push the branch to update the PR and then post a comment. The force-push diff will be empty.

After that, please make any further changes using git commit --amend followed by a force-push to update the PR. And if there are any conflicts in the future, please refrain from merging from master. Instead, rebase (onto master, not a release branch) and force-push. If you merge, we'll have to go through this process again to get rid of the merge commit(s).

---

Now, on to the regular review comments.

---

I thought that we had agreed in discussions with [at]karimhamdanali and [at]dinolii that we would split the summary optimization conditions into positive and negative cases. This PR has been partially updated in a way that prevents the summary from expressing the possibility of folding instanceof to false. Is this distinction still coming? And supposing it is, will this PR be updated to make the distinction, or will that be separate follow-up work?

According to my colleague's work, they have splited the VPClassType and VPClass into two separate cases and now the summary optimization conditions are separated by the testing for different foldings. I have newly implemented the VPClassType part by using TR_FrontEnd::isInstanceOf(). A explanation has been replied right below the reviewer's comments for that part of code.

There are two related issues that we had discussed that required making additional distinctions. The first was to distinguish "type T" (possibly null, all that's needed for checkcast to succeed) from "type T and non-null" (needed for instanceof to succeed). And that distinction has indeed now been made in this PR.

However, I was asking about making a distinction between optimizations possible when a constraint is satisfied and those possible when a constraint is not satisfied. So far this PR only has the former. (In its original revision it treated type constraints as meaning both of them at once in a certain way, but I think we agreed that it shouldn't do that.)

So that leaves my question still unanswered:

Is this distinction still coming?

If not, i.e. if you mean to leave that for the OMR project to deal with after this contribution, I think I'm fine with that at this point, but either way I'd like to clarify what's going on explicitly.

[mingweiarthurli]

Hi @jdmpapin, the commit history has been cleaned up now and all commits has been squashed to one commit.

For the answer of:

Is this distinction still coming?

Actually the first contributor of this PR, Cijie Xia, who has written down if (yesNoMaybe != TR_maybe) was no longer in our team. I has asked him some questions about this PR, but his last commit was 3 year ago, and he cannot remember the details. So I don't know his original intention of why using != TR_maybe.

However, I found some clues about this question from our phase 3 code of OpenJ9 that should call TR::PotentialOptimizationVPPredicate::holdPartialOrderRelation().
The code of checkcast() and instanceof() at J9AbsInterpreter.cpp from line 2235 to 2366 are the functions should utilize holdPartialOrderRelation() to check if the compiler should unlock an optimization. This code actually is not belong to phase 3 but other branch, because is the latest change made by Cijie Xia and hasn't been synced to phase 3 branch yet. These two functions didn't use VPConstaint but a self-defined function TR::InstanceOfFoldingPredicate::predicate(), because this part hasn't been refactored to use VPConstaint before Cijie Xia leave. There was also a function called TR::CheckCastFoldingPredicate::predicate() just at the below of TR::InstanceOfFoldingPredicate::predicate(), but they have same implementation and Cijie Xia has only used TR::InstanceOfFoldingPredicate::predicate() for both checkcast() and instanceof() at J9AbsInterpreter.cpp.
If you see the self-defined function, it is very similar to the earliest version of holdPartialOrderRelation() committed by Cijie Xia, and both used != TR_maybe (this link will jump to the commit of phase 3, since the commit of phase 2 was disappeared due to force-push). I think this design is because Cijie Xia reused this predicate test for both checkcast() and instanceof(), and from line 2277 to 2294 of J9AbsInterpreter.cpp, the instanceOf folding return the boolean value immediately for both TR_yes case (positive case) and TR_no (negative case). However, for the checkcast folding, the code only handle the folding for TR_yes case.

Due to our prior discussion, we decided to test for checkcast folding and instanceOf folding separately. So, for the instanceOf folding test, we will apply optimization for both positive and negative predicate, but for the checkcast folding test, we will only apply optimization for the positive predicate.
There is a simple table of what optimizations our inliner will do (the ⟨null,⊤, ⊤⟩ and ⟨not null,⊤, Derived⟩ represent ⟨whether the value may be null, whether it is an array reference, its class type⟩):

• checkcast folding

  objRef belongs to	⟨null,⊤, ⊤⟩	⟨not null,⊤, Derived⟩
  True	null (objRef)	Derived
  False	N/A	N/A

• instanceOf folding

  objRef belongs to	⟨null,⊤, ⊤⟩	⟨not null,⊤, Derived⟩
  True	False	True
  False	N/A	False

In order to handle both positive and negative predicate, I modified test of instanceOf folding to return true if valueConstraint is null, or if valueConstraint is non-null and the result of fe()->isInstanceOf() is not TR_maybe.

I hope this can answer your question. : )

[karimhamdanali]

Hey @jdmpapin I was wondering if you had a chance to review the latest changes that @mingweiarthurli has made to this PR a couple of weeks ago?

[jdmpapin]

Taking a look now 👀

[mingweiarthurli]

Hi @jdmpapin , I see you are taking a look for this pull request. I just added a table in my last comment to show what optimizations our inliner will do for both the positive and negative cases of checkcast and instanceOf folding. Please refresh the page to let it show up. I hope this will make my comment clearer.

[jdmpapin]

Thanks for all of the updates - this is shaping up to look pretty good!

We have one more change going around for all of the file header comments: #6996. This one isn't merged yet, but it may very well be merged before this PR. So for the new files, the proper SPDX line is as follows:

SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 OR GPL-2.0-only WITH Classpath-exception-2.0 OR GPL-2.0-only WITH OpenJDK-assembly-exception-1.0

Sorry to ask for more changes yet again, but could you please update these SPDX lines?

Additionally, the commit message should describe the changes that it makes to the codebase - i.e. what's being contributed - rather than how those changes have been revised based on review feedback. Please rewrite the commit title and (the main part of the) message. (Your Co-authored-by lines are correct though, so please leave them as-is.)

[jdmpapin]

Jenkins build all

[mingweiarthurli]

Hi @jdmpapin, I have fixed the compilation failure on the ZOS platform. The Jenkins console showed the following message:

13:43:57  "/u/user1/workspace/Build/compiler/optimizer/abstractinterpreter/InliningMethodSummary.cpp", line 103.4: CCN6101 (W) A return value of type "const char *" is expected.

This was caused because we didn't set to return any string for the default switch case of const char* TR::PotentialOptimizationPredicate::getName(), but only rised an assertion. I have fixed this by return the string "Unknown Kind" if there isn't any switch cases match.

For the test failure on the Linux RISCV64 platform, the following message was returned:

...
14:18:57  29: Missing Implementation: Skipping test: ArraycmpLessThanVariableLen/1096
14:18:57  29:     
14:18:57  29: Missing Implementation: Skipping test: ArraycmpLessThanVariableLen/1097
14:18:57  29:     
14:18:57  29: Missing Implementation: Skipping test: ArraycmpLessThanVariableLen/1098
14:18:57  29:     
14:18:57  29: Missing Implementation: Skipping test: ArraycmpLessThanVariableLen/1099
...
14:18:58  97% tests passed, 1 tests failed out of 30
14:18:58  
14:18:58  Total Test time (real) = 2011.76 sec
14:18:58  
14:18:58  The following tests FAILED:
14:18:58  	 29 - comptest (Child killed)
14:18:58  Errors while running CTest

It looks like certain test wasn't implemented, and the I have found follwoing lines in ArrayTest.cpp:393:

TEST_P(ArraycmpNotEqualTest, ArraycmpLessThanVariableLen) {
    SKIP_ON_ARM(MissingImplementation);
    SKIP_ON_RISCV(MissingImplementation);
...

I assume this is a known issue and I will not fix it.

Another problem is I cannot apply those Jenkins checks again once I force pushed my changes. Could you please reapply the checks? I will fix any other problems if there would be any.

[jdmpapin]

Jenkins build all

[mingweiarthurli]

Hi @jdmpapin, sorry for the disturbing! I just would like to check if this pull request has been officially accepted, or do we need to make any futher change? Since I see this pull request is still open and is waiting for merging.

[jdmpapin]

No worries! Sorry for the delay