8290169: adlc: Improve child constraints for vector unary operations #9534

shqking · 2022-07-18T07:46:17Z

As demonstrated in [1], the child constrait generated for predicated
vector unary operation is the super set of that generated for the
unpredicated version. As a result, there exists a risk for predicated
vector unary operaions to match the unpredicated rules by accident.

In this patch, we resolve this issue by generating one extra check
"rChild == NULL" ONLY for vector unary operations. In this way, the
child constraints for predicated/unpredicated vector unary operations
are exclusive now.

Following the example in [1], the dfa state generated for AbsVI is shown
below.

void  State::_sub_Op_AbsVI(const Node *n){
  if( STATE__VALID_CHILD(_kids[0], VREG) && STATE__VALID_CHILD(_kids[1], PREGGOV) &&
      ( UseSVE > 0 ) )
  {
    unsigned int c = _kids[0]->_cost[VREG]+_kids[1]->_cost[PREGGOV] + SVE_COST;
      DFA_PRODUCTION(VREG, vabsI_masked_rule, c)
  }
  if( STATE__VALID_CHILD(_kids[0], VREG) && _kids[1] == NULL &&  <---- 1
      ( UseSVE > 0) )
  {
    unsigned int c = _kids[0]->_cost[VREG] + SVE_COST;
    if (STATE__NOT_YET_VALID(VREG) || _cost[VREG] > c) {
      DFA_PRODUCTION(VREG, vabsI_rule, c)
    }
  }
  ...

We can see that the constraint at line 1 cannot be matched for
predicated AbsVI node now.

The main updates are made in adlc/dfa part. Ideally, we should only
add the extra check for affected platforms, i.e. AVX-512 and SVE. But we
didn't do that because it would be better not to introduce any
architecture dependent implementation here.

Besides, workarounds in both ~~aarch64_sve.ad~~aarch64_vector.ad and x86.ad are removed. 1)
Many "is_predicated_vector()" checks can be removed in ~~aarch64_sve.ad~~aarch64_vector.ad
file. 2) Default instruction cost is used for involving rules in x86.ad
file.

~~[1]. shqking@50ec9b19~~
[1]. shqking@f7d9621e2

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-8290169: adlc: Improve child constraints for vector unary operations

Reviewers

Eric Liu (@theRealELiu - Author) ⚠️ Review applies to 4be4fa37
Xiaohong Gong (@XiaohongGong - Committer) ⚠️ Review applies to f8bfb7be
Sandhya Viswanathan (@sviswa7 - Reviewer) ⚠️ Review applies to f8bfb7be

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9534

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

Using diff file

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

As demonstrated in [1], the child constrait generated for *predicated vector unary operation* is the super set of that generated for the *unpredicated* version. As a result, there exists a risk for predicated vector unary operaions to match the unpredicated rules by accident. In this patch, we resolve this issue by generating one extra check "rChild == NULL" ONLY for vector unary operations. In this way, the child constraints for predicated/unpredicated vector unary operations are exclusive now. Following the example in [1], the dfa state generated for AbsVI is shown below. ``` void State::_sub_Op_AbsVI(const Node *n){ if( STATE__VALID_CHILD(_kids[0], VREG) && STATE__VALID_CHILD(_kids[1], PREGGOV) && ( UseSVE > 0 ) ) { unsigned int c = _kids[0]->_cost[VREG]+_kids[1]->_cost[PREGGOV] + SVE_COST; DFA_PRODUCTION(VREG, vabsI_masked_rule, c) } if( STATE__VALID_CHILD(_kids[0], VREG) && _kids[1] == NULL && <---- 1 ( UseSVE > 0) ) { unsigned int c = _kids[0]->_cost[VREG] + SVE_COST; if (STATE__NOT_YET_VALID(VREG) || _cost[VREG] > c) { DFA_PRODUCTION(VREG, vabsI_rule, c) } } ... ``` We can see that the constraint at line 1 cannot be matched for predicated AbsVI node now. The main updates are made in adlc/dfa part. Ideally, we should only add the extra check for affected platforms, i.e. AVX-512 and SVE. But we didn't do that because it would be better not to introduce any architecture dependent implementation here. Besides, workarounds in both aarch64_sve.ad and x86.ad are removed. 1) Many "is_predicated_vector()" checks can be removed in aarch64_sve.ad file. 2) Default instruction cost is used for involving rules in x86.ad file. [1]. 50ec9b19

bridgekeeper · 2022-07-18T07:47:59Z

👋 Welcome back haosun! 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 · 2022-07-18T07:50:16Z

@shqking 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 · 2022-07-18T07:54:41Z

Webrevs

02: Full (a11766fd)
01: Full (f8bfb7be)
00: Full (4be4fa37)

e1iu · 2022-07-21T03:20:34Z

LGTM.

Resolve the conflicts.

shqking · 2022-08-24T00:38:57Z

Can anyone help to take a look at this PR? Thanks in advance.

TobiHartmann · 2022-09-08T11:20:57Z

Looks reasonable to me but I'm not an expert in that area. @jatin-bhateja, @sviswa7, @iwanowww could you have a look?

XiaohongGong

AArch64 part looks good to me. Thanks!

sviswa7

Looks good to me.

openjdk · 2022-09-13T18:59:54Z

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

8290169: adlc: Improve child constraints for vector unary operations

Reviewed-by: eliu, xgong, sviswanathan

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

7376c55: 8293769: RISC-V: Add a second temporary register for BarrierSetAssembler::load_at
d191e47: 8293768: Add links to JLS 19 and 20 from SourceVersion enum constants
a75ddb8: 8293122: (fs) Use file cloning in macOS version of Files::copy method
95c7c55: 8293402: hs-err file printer should reattempt stack trace printing if it fails
211fab8: 8291669: [REDO] Fix array range check hoisting for some scaled loop iv
7f3250d: 8293787: Linux aarch64 build fails after 8292591
2a38791: 8292755: Non-default method in interface leads to a stack overflow in JShell
8351b30: 8293771: runtime/handshake/SystemMembarHandshakeTransitionTest.java fails if MEMBARRIER_CMD_QUERY is unsupported
91f9c0d: 8293774: Improve TraceOptoParse to dump the bytecode name
1169a15: 8291657: Javac assertion when compiling a method call with switch expression as argument
... and 11 more: https://git.openjdk.org/jdk/compare/8bd79d3efdcab5997056675467aac70895903d33...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.

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 (@XiaohongGong, @sviswa7) 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).

shqking · 2022-09-14T07:34:48Z

@TobiHartmann Thanks for your comment.

As some shared code, i.e. share/adlc, gets changed in this patch, I think it would be better for this patch to pass the Oracle CI tests. I wonder if you could help to launch the test? Thanks.

TobiHartmann · 2022-09-14T11:49:15Z

Sure, I did already run testing. All passed.

shqking · 2022-09-15T00:40:43Z

@TobiHartmann Thanks for your testing.

/integrate

openjdk · 2022-09-15T00:41:59Z

@shqking
Your change (at version a11766f) is now ready to be sponsored by a Committer.

nsjian · 2022-09-15T01:36:30Z

/sponsor

openjdk · 2022-09-15T01:38:20Z

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

2057070: 8293815: P11PSSSignature.engineUpdate should not print debug messages during normal operation
7376c55: 8293769: RISC-V: Add a second temporary register for BarrierSetAssembler::load_at
d191e47: 8293768: Add links to JLS 19 and 20 from SourceVersion enum constants
a75ddb8: 8293122: (fs) Use file cloning in macOS version of Files::copy method
95c7c55: 8293402: hs-err file printer should reattempt stack trace printing if it fails
211fab8: 8291669: [REDO] Fix array range check hoisting for some scaled loop iv
7f3250d: 8293787: Linux aarch64 build fails after 8292591
2a38791: 8292755: Non-default method in interface leads to a stack overflow in JShell
8351b30: 8293771: runtime/handshake/SystemMembarHandshakeTransitionTest.java fails if MEMBARRIER_CMD_QUERY is unsupported
91f9c0d: 8293774: Improve TraceOptoParse to dump the bytecode name
... and 12 more: https://git.openjdk.org/jdk/compare/8bd79d3efdcab5997056675467aac70895903d33...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-09-15T01:38:37Z

@nsjian @shqking Pushed as commit eeb625e.

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

openjdk bot added the rfr Pull request is ready for review label Jul 18, 2022

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Jul 18, 2022

e1iu approved these changes Jul 21, 2022

View reviewed changes

Merge branch 'master' into 8290169-adlc

f8bfb7b

Resolve the conflicts.

XiaohongGong approved these changes Sep 9, 2022

View reviewed changes

shqking added 2 commits September 13, 2022 05:13

Merge branch 'master' into 8290169-adlc

28f5c07

Remove the "is_predicated_vector()" check introduced in JDK-8292587

a11766f

sviswa7 approved these changes Sep 13, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 13, 2022

openjdk bot added the sponsor Pull request is ready to be sponsored label Sep 15, 2022

openjdk bot added the integrated Pull request has been integrated label Sep 15, 2022

openjdk bot closed this Sep 15, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Sep 15, 2022

shqking deleted the 8290169-adlc branch September 15, 2022 01:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8290169: adlc: Improve child constraints for vector unary operations #9534

8290169: adlc: Improve child constraints for vector unary operations #9534

shqking commented Jul 18, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jul 18, 2022

openjdk bot commented Jul 18, 2022

mlbridge bot commented Jul 18, 2022 •

edited

Loading

e1iu commented Jul 21, 2022

shqking commented Aug 24, 2022

TobiHartmann commented Sep 8, 2022

XiaohongGong left a comment

sviswa7 left a comment

openjdk bot commented Sep 13, 2022 •

edited

Loading

shqking commented Sep 14, 2022

TobiHartmann commented Sep 14, 2022

shqking commented Sep 15, 2022

openjdk bot commented Sep 15, 2022

nsjian commented Sep 15, 2022

openjdk bot commented Sep 15, 2022

openjdk bot commented Sep 15, 2022

8290169: adlc: Improve child constraints for vector unary operations #9534

8290169: adlc: Improve child constraints for vector unary operations #9534

Conversation

shqking commented Jul 18, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jul 18, 2022

openjdk bot commented Jul 18, 2022

mlbridge bot commented Jul 18, 2022 • edited Loading

Webrevs

e1iu commented Jul 21, 2022

shqking commented Aug 24, 2022

TobiHartmann commented Sep 8, 2022

XiaohongGong left a comment

Choose a reason for hiding this comment

sviswa7 left a comment

Choose a reason for hiding this comment

openjdk bot commented Sep 13, 2022 • edited Loading

shqking commented Sep 14, 2022

TobiHartmann commented Sep 14, 2022

shqking commented Sep 15, 2022

openjdk bot commented Sep 15, 2022

nsjian commented Sep 15, 2022

openjdk bot commented Sep 15, 2022

openjdk bot commented Sep 15, 2022

shqking commented Jul 18, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Jul 18, 2022 •

edited

Loading

openjdk bot commented Sep 13, 2022 •

edited

Loading