RFR: 8261671: X86 I2L conversion can be skipped for certain masked positive values [v3]

[Vladimir Kozlov]

On Fri, 5 Mar 2021 22:02:03 GMT, Marcus G K Williams <github.com+168222+mgkwill at openjdk.org> wrote:

>>> compiler/intrinsics/bmi/TestBzhiI2L.java test failed:
>>> 
>>> ```
>>>  stdout: [For random generator using seed: -6986231732124296613
>>> To re-run test with same seed value please add "-Djdk.test.lib.random.seed=-6986231732124296613" to command line.
>>> ];
>>>  stderr: [Exception in thread "main" java.lang.UnsupportedOperationException
>>> 	at compiler.intrinsics.bmi.Expr.longExpr(Expr.java:97)
>>> 	at compiler.intrinsics.bmi.BMITestRunner$Executor.runUnaryLongRegTest(BMITestRunner.java:280)
>>> 	at compiler.intrinsics.bmi.BMITestRunner$Executor.runTests(BMITestRunner.java:219)
>>> 	at compiler.intrinsics.bmi.BMITestRunner$Executor.main(BMITestRunner.java:190)
>>> ]
>>>  exitValue = 1
>>> 
>>> java.lang.RuntimeException: Expected to get exit value of [0]
>>> 
>>> 	at jdk.test.lib.process.OutputAnalyzer.shouldHaveExitValue(OutputAnalyzer.java:474)
>>> 	at compiler.intrinsics.bmi.BMITestRunner.runTest(BMITestRunner.java:151)
>>> 	at compiler.intrinsics.bmi.BMITestRunner.runTests(BMITestRunner.java:87)
>>> 	at compiler.intrinsics.bmi.TestBzhiI2L.main(TestBzhiI2L.java:49)
>>> ```
>> 
>> Hi @vnkozlov - 
>> 
>> - I've updated BMI2.java with a reduced iterations of 30K and I tried to incorporate your feedback on code structure from #2811. My previous version was an attempt to match the code style and structure used in existing code. We can agree that those tests are less than optimal. Please point out further specifics on anything that could be improved.
>> 
>> - On TestBzhiI2L.java I did some digging. It appears there were some implementation issues with the underlying BmiIntrinsicBase.java and the BmiTestCase_x64 class. I believe I've fixed those issues.
>> 
>> However in my estimation the first version of code should not have hit Expr.java:97 longExpr below:
>>>  stderr: [Exception in thread "main" java.lang.UnsupportedOperationException
>>> 	at compiler.intrinsics.bmi.Expr.longExpr(Expr.java:97)
>> 
>> It should have hit intToLongExpr. This prompted me to see that my tests did not include a couple of key parts to the BMI Test running setup, including a BMIUnaryIntToLongExpr in Expr.java that is used instead of BMIUnaryLongExpr. The test seemed to be working without these but I'm guessing there are corner cases that this would cause problems, maybe explaining what was hit above. Though I don't have a good explanation why yet.
>> 
>> I've made the fixes I can see were needed. I've sourced an I7 to see if I can reproduce the failure. In the meantime, assuming pre-submit tests pass, can you run your tests again?
>> 
>> Thanks,
>> Marcus
>
>> > compiler/intrinsics/bmi/TestBzhiI2L.java test failed:
>> > ```
>> >  stdout: [For random generator using seed: -6986231732124296613
>> > To re-run test with same seed value please add "-Djdk.test.lib.random.seed=-6986231732124296613" to command line.
>> > ];
>> >  stderr: [Exception in thread "main" java.lang.UnsupportedOperationException
>> > 	at compiler.intrinsics.bmi.Expr.longExpr(Expr.java:97)
>> > 	at compiler.intrinsics.bmi.BMITestRunner$Executor.runUnaryLongRegTest(BMITestRunner.java:280)
>> > 	at compiler.intrinsics.bmi.BMITestRunner$Executor.runTests(BMITestRunner.java:219)
>> > 	at compiler.intrinsics.bmi.BMITestRunner$Executor.main(BMITestRunner.java:190)
>> > ]
>> >  exitValue = 1
>> > 
>> > java.lang.RuntimeException: Expected to get exit value of [0]
>> > 
>> > 	at jdk.test.lib.process.OutputAnalyzer.shouldHaveExitValue(OutputAnalyzer.java:474)
>> > 	at compiler.intrinsics.bmi.BMITestRunner.runTest(BMITestRunner.java:151)
>> > 	at compiler.intrinsics.bmi.BMITestRunner.runTests(BMITestRunner.java:87)
>> > 	at compiler.intrinsics.bmi.TestBzhiI2L.main(TestBzhiI2L.java:49)
>> > ```
>> 
>> Hi @vnkozlov -
>> 
>> * I've updated BMI2.java with a reduced iterations of 30K and I tried to incorporate your feedback on code structure from #2811. My previous version was an attempt to match the code style and structure used in existing code. We can agree that those tests are less than optimal. Please point out further specifics on anything that could be improved.
>> * On TestBzhiI2L.java I did some digging. It appears there were some implementation issues with the underlying BmiIntrinsicBase.java and the BmiTestCase_x64 class. I believe I've fixed those issues.
>> 
>> However in my estimation the first version of code should not have hit Expr.java:97 longExpr below:
>> 
>> > stderr: [Exception in thread "main" java.lang.UnsupportedOperationException
>> > at compiler.intrinsics.bmi.Expr.longExpr(Expr.java:97)
>> 
>> It should have hit intToLongExpr. This prompted me to see that my tests did not include a couple of key parts to the BMI Test running setup, including a BMIUnaryIntToLongExpr in Expr.java that is used instead of BMIUnaryLongExpr. The test seemed to be working without these but I'm guessing there are corner cases that this would cause problems, maybe explaining what was hit above. Though I don't have a good explanation why yet.
>> 
>> I've made the fixes I can see were needed. I've sourced an I7 to see if I can reproduce the failure. In the meantime, assuming pre-submit tests pass, can you run your tests again?
>> 
>> Thanks,
>> Marcus
> 
> P.S. I added some specific logic to skip TestBzhiI2L.java tests if BMI2 is not available and hopefully fixed the logic that should have stopped the tests from running before if cpuFeature was not present. Hopefully this solves the issue involving AMD EPYC and Aarch64.

Good.
Unfortunately we need to wait next week for testing results because we are updating our testing system.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2590