8281122: [IR Framework] Cleanup IR matching code in preparation for JDK-8280378

[chhagedorn]

This patch does some restructurings and refactorings of the IRMatcher.java class in preparation for adding IR matching support on different compile phases with JDK-8280378. There are no semantic changes of how IR matching is eventually done on regexes and how the results are checked with the failOn and counts constraints provided by the user. The only user-visible change is an improved output format of matching failures.

The old format also did not report a counts constraint failure correctly. It wrongly used the format of failOn failures which was misleading. This is also fixed by this patch.

Example:

@Test
@IR(counts = {IRNode.STORE_F, "!= 1"}) // fails
@IR(counts = {IRNode.STORE_F, "2", IRNode.LOAD_F, "> 1"}) // both constraints fail
public void test1() {
    fFld = 4.2f;
}

@Test
@IR(failOn = IRNode.ALLOC, counts = {IRNode.STORE_F, ">= 2"}) // failOn and counts fail
public void test2() {
    fFld = 4.2f;
    o = new Object();
}

Failure outputs:

Old format

Failed IR Rules (3)
------------------
- Method "public void ir_framework.tests.Testi.test1()":
  * @IR rule 1: "@compiler.lib.ir_framework.IR(applyIf={}, applyIfAnd={}, failOn={}, applyIfOr={}, counts={"(\\d+(\\s){2}(StoreF.*)+(\\s){2}===.*)", "!= 1"}, applyIfNot={})"
    - counts: Graph contains wrong number of nodes:
        Regex 1: (\d+(\s){2}(StoreF.*)+(\s){2}===.*)
        Expected 1 but found 1 node:
          25  StoreF  ===  5  7  24  21  [[ 16 ]]  @ir_framework/tests/Testi+12 *, name=fFld, idx=4;  Memory: @ir_framework/tests/Testi:NotNull+12 *, name=fFld, idx=4; !jvms: Testi::test1 @ bci:3 (line 48)
  * @IR rule 2: "@compiler.lib.ir_framework.IR(applyIf={}, applyIfAnd={}, failOn={}, applyIfOr={}, counts={"(\\d+(\\s){2}(StoreF.*)+(\\s){2}===.*)", "2", "(\\d+(\\s){2}(LoadF.*)+(\\s){2}===.*)", "> 1"}, applyIfNot={})"
    - counts: Graph contains wrong number of nodes:
        Regex 1: (\d+(\s){2}(StoreF.*)+(\s){2}===.*)
        Expected 2 but found 1 node:
          25  StoreF  ===  5  7  24  21  [[ 16 ]]  @ir_framework/tests/Testi+12 *, name=fFld, idx=4;  Memory: @ir_framework/tests/Testi:NotNull+12 *, name=fFld, idx=4; !jvms: Testi::test1 @ bci:3 (line 48)
        Regex 2: (\d+(\s){2}(LoadF.*)+(\s){2}===.*)
        Expected 1 but found 0 nodes.

- Method "public void ir_framework.tests.Testi.test2()":
  * @IR rule 1: "@compiler.lib.ir_framework.IR(applyIf={}, applyIfAnd={}, failOn={"(.*precise .*\\R((.*(?i:mov|xorl|nop|spill).*|\\s*|.*LGHI.*)\\R)*.*(?i:call,static).*wrapper for: _new_instance_Java)"}, applyIfOr={}, counts={"(\\d+(\\s){2}(StoreF.*)+(\\s){2}===.*)", ">= 2"}, applyIfNot={})"
    - failOn: Graph contains forbidden nodes:
        Regex 1: (.*precise .*\R((.*(?i:mov|xorl|nop|spill).*|\s*|.*LGHI.*)\R)*.*(?i:call,static).*wrapper for: _new_instance_Java)
        Matched forbidden node:
          1a5     movq    RSI, precise java/lang/Object: 0x00007fb188007318:Constant:exact *	# ptr
    1af     call,static  wrapper for: _new_instance_Java
    - counts: Graph contains wrong number of nodes:
        Regex 1: (\d+(\s){2}(StoreF.*)+(\s){2}===.*)
        Expected 2 but found 1 node:
          25  StoreF  ===  5  7  24  21  [[ 19  236  36  179  220 ]]  @ir_framework/tests/Testi+12 *, name=fFld, idx=4;  Memory: @ir_framework/tests/Testi:NotNull+12 *, name=fFld, idx=4; !jvms: Testi::test2 @ bci:3 (line 55)

New format

Failed IR Rules (3) of Methods (2)
----------------------------------
1) Method "public void ir_framework.tests.Testi.test1()" - [Failed IR rules: 2]:
   * @IR rule 1: "@compiler.lib.ir_framework.IR(applyIf={}, applyIfAnd={}, failOn={}, applyIfOr={}, counts={"(\\d+(\\s){2}(StoreF.*)+(\\s){2}===.*)", "!= 1"}, applyIfNot={})"
     - counts: Graph contains wrong number of nodes:
       * Regex 1: (\d+(\s){2}(StoreF.*)+(\s){2}===.*)
         - Failed comparison: [found] 1 != 1 [given]
         - Matched node:
           * 25  StoreF  ===  5  7  24  21  [[ 16 ]]  @ir_framework/tests/Testi+12 *, name=fFld, idx=4;  Memory: @ir_framework/tests/Testi:NotNull+12 *, name=fFld, idx=4; !jvms: Testi::test1 @ bci:3 (line 48)
   * @IR rule 2: "@compiler.lib.ir_framework.IR(applyIf={}, applyIfAnd={}, failOn={}, applyIfOr={}, counts={"(\\d+(\\s){2}(StoreF.*)+(\\s){2}===.*)", "2", "(\\d+(\\s){2}(LoadF.*)+(\\s){2}===.*)", "> 1"}, applyIfNot={})"
     - counts: Graph contains wrong number of nodes:
       * Regex 1: (\d+(\s){2}(StoreF.*)+(\s){2}===.*)
         - Failed comparison: [found] 1 = 2 [given]
         - Matched node:
           * 25  StoreF  ===  5  7  24  21  [[ 16 ]]  @ir_framework/tests/Testi+12 *, name=fFld, idx=4;  Memory: @ir_framework/tests/Testi:NotNull+12 *, name=fFld, idx=4; !jvms: Testi::test1 @ bci:3 (line 48)
       * Regex 2: (\d+(\s){2}(LoadF.*)+(\s){2}===.*)
         - Failed comparison: [found] 0 > 1 [given]
         - No nodes matched!

2) Method "public void ir_framework.tests.Testi.test2()" - [Failed IR rules: 1]:
   * @IR rule 1: "@compiler.lib.ir_framework.IR(applyIf={}, applyIfAnd={}, failOn={"(.*precise .*\\R((.*(?i:mov|xorl|nop|spill).*|\\s*|.*LGHI.*)\\R)*.*(?i:call,static).*wrapper for: _new_instance_Java)"}, applyIfOr={}, counts={"(\\d+(\\s){2}(StoreF.*)+(\\s){2}===.*)", ">= 2"}, applyIfNot={})"
     - failOn: Graph contains forbidden nodes:
       * Regex 1: (.*precise .*\R((.*(?i:mov|xorl|nop|spill).*|\s*|.*LGHI.*)\R)*.*(?i:call,static).*wrapper for: _new_instance_Java)
         - Matched forbidden node:
           * 1a5     movq    RSI, precise java/lang/Object: 0x00007fd3c4007318:Constant:exact *	# ptr
             1af     call,static  wrapper for: _new_instance_Java
     - counts: Graph contains wrong number of nodes:
       * Regex 1: (\d+(\s){2}(StoreF.*)+(\s){2}===.*)
         - Failed comparison: [found] 1 >= 2 [given]
         - Matched node:
           * 25  StoreF  ===  5  7  24  21  [[ 19  236  36  179  220 ]]  @ir_framework/tests/Testi+12 *, name=fFld, idx=4;  Memory: @ir_framework/tests/Testi:NotNull+12 *, name=fFld, idx=4; !jvms: Testi::test2 @ bci:3 (line 55)

New class structure:

The old IRMatcher class mixed different tasks such as parsing, IR annotation verification, IR matching and failure reporting together. This made it difficult to extend the existing code to add IR matching support on different compile phases. Therefore, the class was split to separate the different tasks:

• parser classes (IREncodingParser, HotSpotPidFileParser, IRMethodParser etc.)
• entity classes for the different parts of an @IR annotation (IRMethod, IRRule, Counts, FailOn etc.)
• match result classes for each entity class to provide a formatted failure message (IRMethodMatchResult, IRRuleMatchResult etc.)

The main structure of the new classes will be kept in JDK-8280378 but will be further improved to match the new needs.

Testing:

• Normal tier testing including tier5 and 6 where the IR framework tests are executed.
• Testing of this patch in Valhalla with tier1-3 + some Valhalla-specific stress testing

Thanks,
Christian

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8281122: [IR Framework] Cleanup IR matching code in preparation for JDK-8280378

Reviewers

• Tobias Hartmann (@TobiHartmann - Reviewer)
• Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7533/head:pull/7533
$ git checkout pull/7533

Update a local copy of the PR:
$ git checkout pull/7533
$ git pull https://git.openjdk.java.net/jdk pull/7533/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7533

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7533.diff

[bridgekeeper]

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

@chhagedorn 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 (873a8282)

[TobiHartmann]

Great work! It's hard to review though so I'm trusting your expertise in this area. Approved.

[openjdk]

@chhagedorn 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:

8281122: [IR Framework] Cleanup IR matching code in preparation for JDK-8280378

Reviewed-by: thartmann, kvn

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 441 new commits pushed to the master branch:

• eac8021: 8282320: Remove case conversion for debugging log in SSLCipher
• b86a8c0: 8282150: Drop redundant
  elements from tables in java.desktop HTML files
• 941e97c: 8281738: Create a regression test for checking the 'Space' key activation of focused Button
• 8fec7b8: 8281548: Add escape analysis tracing flag
• b03d66c: 8282452: Use of Preview API in preview methods should not trigger preview warning for the enclosing class
• 76398c8: 8279573: compiler/codecache/CodeCacheFullCountTest.java fails with "RuntimeException: the value of full_count is wrong."
• 31b61f9: 8282311: Fix a typo in javax.lang.model.type.NullType
• 2c5d266: 8282045: When loop strip mining fails, safepoints are removed from loop anyway
• 341c8bd: 8267834: Refactor G1CardSetAllocator and BufferNode::Allocator to use a common base class
• fcce24c: 8281811: assert(_base == Tuple) failed: Not a Tuple after JDK-8280799
• ... and 431 more: https://git.openjdk.java.net/jdk/compare/a4d201909c8919b7465dee72594d718252c6344e...master

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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[chhagedorn]

Thanks a lot for your review Tobias! Yes, unfortunately, it is quite an extensive cleanup which makes it hard to review.

[vnkozlov]

Good.

[chhagedorn]

Thanks Vladimir for your review!

[chhagedorn]

/integrate

[openjdk]

Going to push as commit 2da6777.
Since your change was applied there have been 454 commits pushed to the master branch:

• d0eb6fa: 8281569: Create tests for Frame.setMinimumSize() method
• 02aa7ce: 8282515: More clean up on NativeLibraries just for JNI library use
• 1485883: 8281628: KeyAgreement : generateSecret intermittently not resetting
• b6c35ae: 8209784: Include hsdis in the JDK
• ce18ff8: 8282551: Properly initialize L32X64MixRandom state
• 234c17e: 8274788: Support archived heap objects in ParallelGC
• fc918a7: 8281234: The -protected option is not always checked in keytool and jarsigner
• d80f697: 8282523: Fix 'hierachy' typo
• f12200c: 8267796: vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/TestDescription.java fails with NoClassDefFoundError
• 12a822a: 8282381: Parallel: Remove unnecessary PCReferenceProcessor
• ... and 444 more: https://git.openjdk.java.net/jdk/compare/a4d201909c8919b7465dee72594d718252c6344e...master

Your commit was automatically rebased without conflicts.

[openjdk]

@chhagedorn Pushed as commit 2da6777.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.