8038953,Add sanity tests for BMI1 and LZCNT instructions

Anton Ivanov anton.ivanov at oracle.com
Tue Apr 8 10:30:31 UTC 2014


>> 2. countCpuInstructions() moved to base class
> It still abstruct in base class:

countCpuInstructions() should be abstract. Concrete classes has their's 
own implementation depending on what instruction they are searching for, 
there is no sense to make this method non-abstract, it should be 
overwritten in every ancestor.

> protected abstract int countCpuInstructions(byte[] nativeCode);
>> 3. 0b001_11000 was a typo, thank you
> BlsmskTestI.java still have 0b001_11000.
fixed

webrev: http://cr.openjdk.java.net/~iignatyev/aaivanov/8038953/webrev.03 
<http://cr.openjdk.java.net/%7Eiignatyev/aaivanov/8038953/webrev.03>


> Thanks,
> Vladimir
>
>>
>>
>> On 03.04.2014 22:33, Vladimir Kozlov wrote:
>>> On 4/3/14 8:09 AM, Anton Ivanov wrote:
>>>> Here is the new webrev
>>>> http://cr.openjdk.java.net/~iignatyev/aaivanov/8038953/webrev.01
>>>> tests directory renamed to verifycode
>>>> skipping on not-target platform
>>>
>>> Why not to skip when !isIntrinsicSupported()? Otherwise you will get
>>> very long useless testing output and at the end:
>>> Java HotSpot(TM) 64-Bit Server VM warning: lzcnt instruction is not
>>> available on this CPU
>>>
>>> Just try to run on CPU which does not support this instructions.
>>>
>>> Do you plan to test Client VM (C1) in a future? If not we should skip
>>> whole testing if it is not Platform.isServer().
>>>
>>> May be report in checkEmittedCode() when we don't check instructions
>>> to know that that test passed without check.
>>> Also in checkEmittedCode() method verifyPositive() should be called
>>> only in scope where its result is used.
>>>
>>> We always get complain that tests run too long and your tests execute
>>> a lot of code with not used result.
>>>
>>> The check code in countCpuInstructions() looks the same and can be
>>> moved into common method.
>>>
>>> I see in one case you use 0b0011_1000 and in an other 0b001_11000, 
>>> typo?
>>>
>>> Thanks,
>>> Vladimir
>>>
>>>> negative check (for "Unexpected CPU instructions found") removed
>>>>
>>>>
>>>> On 01.04.2014 23:56, Vladimir Kozlov wrote:
>>>>> Okay, I see that you do use code from Filipp's changes (for example,
>>>>> TestAndnI.AndnIExpr).
>>>>>
>>>>> I think subdir name 'sanity' is not accurate and too general.
>>>>> Something like 'verifycode' or similar is more reasonable.
>>>>>
>>>>> Why you not bailing out (skip test) in main() (or in test()) when
>>>>> intrinsic is not used?
>>>>>
>>>>> An asm instruction could be used without intrinsic. It all depends on
>>>>> mach instructions matching rules in implementation in .ad file. So
>>>>> your  AssertionError "Unexpected CPU instructions found: " could be
>>>>> triggered. Also what if C1 also generate such code? I don't know why
>>>>> you have this assert.
>>>>>
>>>>> I think you need to add WB API to get info if a particular intrinsic
>>>>> was really used. Even if you pass UseCountTrailingZerosInstruction
>>>>> flag it could be overwritten by InlineNatives flag. There could be
>>>>> other legit reasons we don't use an intrinsic.
>>>>>
>>>>> On 4/1/14 10:51 AM, Anton Ivanov wrote:
>>>>>> Hi Vladimir,
>>>>>>
>>>>>> They test different things,
>>>>>> Fillip's tests - verify results to be the same for compiler and
>>>>>> interpreter (but imagine compiler do not use intrinsic at all, test
>>>>>> will
>>>>>> pass too, because they will do exactly the same operations)
>>>>>
>>>>> It is not true. Even when it is not intrinsic JIT still generates
>>>>> assembler code which could be problematic.
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>>> My tests verify only that certain asm instructions are presented in
>>>>>> native code emitted by compiler (but do not verify the 
>>>>>> correctness of
>>>>>> execution)
>>>>>> Combination of these 2 sets of tests makes sure that new 
>>>>>> instructions
>>>>>> are really used, and used correctly.
>>>>>> It also was not easy to modify Fillip's tests because they were
>>>>>> developed in parallel
>>>>>>
>>>>>>
>>>>>> combination of these 2 sets give us
>>>>>>
>>>>>> On 01.04.2014 21:03, Vladimir Kozlov wrote:
>>>>>>> I mean why do you need to create separate tests and not use (add 
>>>>>>> new
>>>>>>> code) existing one?
>>>>>>> For example, you added bmi/sanity/AddnTestI.java when there is
>>>>>>> already
>>>>>>> bmi/TestAndnI.java.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Vladimir
>>>>>>>
>>>>>>> On 4/1/14 9:57 AM, Vladimir Kozlov wrote:
>>>>>>>> Anton,
>>>>>>>>
>>>>>>>> How your changes relate to Filipp's?:
>>>>>>>>
>>>>>>>> http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/rev/15d507abfc7a
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Vladimir
>>>>>>>>
>>>>>>>> On 4/1/14 9:40 AM, Anton Ivanov wrote:
>>>>>>>>> Hi all,
>>>>>>>>>
>>>>>>>>> Please review the patch
>>>>>>>>>
>>>>>>>>> New tests for using of blsi,blsr,andn,blsmsk,lzcnt,tzcnt
>>>>>>>>> instructions were developed.
>>>>>>>>> They verify that instructions above are really presented in 
>>>>>>>>> emitted
>>>>>>>>> native code on supported hardware
>>>>>>>>>
>>>>>>>>> webrev:
>>>>>>>>> http://cr.openjdk.java.net/~iignatyev/aaivanov/8038953/webrev.00
>>>>>>>>> jbs: https://bugs.openjdk.java.net/browse/JDK-8038953
>>>>>>>>> testing: test/compiler/intrinsics/bmi/sanity
>>>>>>>>>
>>>>>>
>>>>
>>

-- 
Best regards,
Anton Ivanov

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20140408/84f1b8ab/attachment.html>


More information about the hotspot-compiler-dev mailing list