8269775: compiler/codegen/ClearArrayTest.java failed with "assert(false) failed: bad AD file" #199
Conversation
…se) failed: bad AD file"
|
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to fix x86_32.ad
file too.
src/hotspot/cpu/x86/x86_64.ad
Outdated
match(Set dummy (ClearArray cnt base)); | ||
effect(USE_KILL cnt, USE_KILL base, TEMP tmp, KILL zero, KILL cr); | ||
ins_cost(125); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need it here. There are no other ClearArray conflicting instructions for small copy and AVX <= 2.
src/hotspot/cpu/x86/x86_64.ad
Outdated
predicate(!((ClearArrayNode*)n)->is_large() && | ||
UseAVX > 2 && | ||
!n->in(2)->bottom_type()->is_long()->is_con()); | ||
predicate(!((ClearArrayNode*)n)->is_large() && UseAVX > 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add ()
around UseAVX > 2
as in other place.
@sviswa7 Please, file separate RFE to improve this ClearArrayTest test which should include all cases we have in .ad file:
|
I verified that with this fix |
RFE for test filed: https://bugs.openjdk.java.net/browse/JDK-8269789 |
@vnkozlov I have implemented all your review comments. Please take a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
I am currently running tests. I will approve when they pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing passed.
@sviswa7 This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be:
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 1 new commit pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the
|
@vnkozlov Please let me know if I can go ahead and integrate. |
Wait Dean's review. It is fine if you push tomorrow. |
Thanks @sviswa7 , I missed putting this change in my last check-in, attaching instruction costs will resolve conflicts and select instruction with minimum cost in case of multiple matches in this BURS based instruction selection. |
Thanks a lot @vnkozlov @dean-long for the review and all the help. |
/integrate |
Going to push as commit 6f0e8e7.
Your commit was automatically rebased without conflicts. |
The following test failed in JDK17
compiler/codegen/ClearArrayTest.java
with assert(false) failed: bad AD file
The problem is that no match is found for platforms not supporting avx512vlbw for constant input.
Per analysis from Dean Long and Vladimir Kozlov:
This is due to rep_stos rules for small clear array which use !n->in(2)->bottom_type()->is_long()->is_con().
This prevents the rule from matching with a constant --> register conversion.
Removing !is_con() from rep_stos for small clear array fixes the issue.
We also add appropriate "ins_cost" to all ClearArray rules, so that in the case of multiple matches, we break the tie based on ins_cost.
Best Regards,
Sandhya
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk17 pull/199/head:pull/199
$ git checkout pull/199
Update a local copy of the PR:
$ git checkout pull/199
$ git pull https://git.openjdk.java.net/jdk17 pull/199/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 199
View PR using the GUI difftool:
$ git pr show -t 199
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk17/pull/199.diff