8386163: C2 Vector API: assert(collect_unique_inputs(n, inputs) == 1) failed: not unary#31626
8386163: C2 Vector API: assert(collect_unique_inputs(n, inputs) == 1) failed: not unary#31626jatin-bhateja wants to merge 2 commits into
Conversation
… failed: not unary
|
/label add hotspot-compiler-dev |
|
👋 Welcome back jbhateja! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@jatin-bhateja |
|
The total number of required reviews for this PR has been set to 2 based on the presence of this label: |
|
@jatin-bhateja To determine the appropriate audience for reviewing this pull request, one or more labels corresponding to different subsystems will normally be applied automatically. However, no automatic labelling rule matches the changes in this pull request. In order to have an "RFR" email sent to the correct mailing list, you will need to add one or more applicable labels manually using the /label pull request command. Applicable Labels
|
Webrevs
|
| @@ -2721,13 +2721,10 @@ static uint collect_unique_inputs(Node* n, Unique_Node_List& inputs) { | |||
| if (is_vector_bitwise_op(n)) { | |||
| uint inp_cnt = n->is_predicated_vector() ? n->req()-1 : n->req(); | |||
| if (VectorNode::is_vector_bitwise_not_pattern(n)) { | |||
There was a problem hiding this comment.
Can VectorNode::is_vector_bitwise_not_pattern return the operand that is noted, so its caller does not need to find that information itself?
There was a problem hiding this comment.
I preferred fixing existing detection logic after the call to is_vector_bitwise_not_pattern which skip over both all ones operand. currently is_vector_bitwise_not_pattern returns a bool value, making the change you suggested will lead to a change (retValue != nullptr) at all the existing call sites of is_vector_bitwise_not_pattern.
|
FYI: just found this bug while reviewing ;) |
| @@ -2721,13 +2721,10 @@ static uint collect_unique_inputs(Node* n, Unique_Node_List& inputs) { | |||
| if (is_vector_bitwise_op(n)) { | |||
| uint inp_cnt = n->is_predicated_vector() ? n->req()-1 : n->req(); | |||
| if (VectorNode::is_vector_bitwise_not_pattern(n)) { | |||
There was a problem hiding this comment.
Should this code also check is_vectormask_bitwise_not_pattern()? It looks like VectorNode::make_mask_node() can create either kind, depending on platform support.
There was a problem hiding this comment.
collect_unique_inputs is only ever called on nodes for which is_vector_bitwise_op(n) holds, and that helper is restricted to Op_XorV / Op_AndV / Op_OrV / Op_MacroLogicV — it does not include the *VMask opcodes. So a genuine XorVMaskNode never enters the macro-logic cone optimization at all.
|
And found yet another bug in logic cones, while reviewing: JDK-8387204 I'm a bit disappointed in how many bugs I can find here. I think testing isn't very good for this optimization 😞 |
5 participants
This patch fixes an assertion failure seen in macro logic optimization caused by
collect_unique_inputsmishandling of the bitwise-not patternXorV(operand, -1)when the negated operand was itself an all-ones vector, i.e.XorV(-1, -1). It skipped all all-ones operands, collecting zero inputs instead of one, which tripped an assertion failure. The fix makes input collection select the single negated operand, so exactly one input is collected.Kindly review and share your feedback.
Best Regards,
Jatin
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/31626/head:pull/31626$ git checkout pull/31626Update a local copy of the PR:
$ git checkout pull/31626$ git pull https://git.openjdk.org/jdk.git pull/31626/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 31626View PR using the GUI difftool:
$ git pr show -t 31626Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/31626.diff
Using Webrev
Link to Webrev Comment