8038953,Add sanity tests for BMI1 and LZCNT instructions
Vladimir Kozlov
vladimir.kozlov at oracle.com
Thu Apr 10 17:25:21 UTC 2014
Hi Anton,
You don't need to pass parameters to countCpuInstructions() which can access these new fields directly.
Also can you put each value on separate line in arrays initialization? It is easier to see that way. TZcntTestI and
LZcntTestI are fine as they are now but others:
instrMask = new byte[] {
(byte) 0xFF,
(byte) 0x1F,
(byte) 0x00,
(byte) 0xFF};
instrPattern = new byte[] {
(byte) 0xC4, // prefix for 3-byte VEX instruction
(byte) 0x02, // 00010 implied 0F 38 leading opcode bytes
(byte) 0x00,
(byte) 0xF2};
Otherwise looks good.
Thanks,
Vladimir
On 4/10/14 7:07 AM, Anton Ivanov wrote:
> http://cr.openjdk.java.net/~iignatyev/aaivanov/8038953/webrev.04/
>
> On 09.04.2014 0:25, Vladimir Kozlov wrote:
>> In BlsiTestI.java, BlsmskTestI.java, BlsrTestI.java, you check 5 elements but the scanned size is length - 4.
>>
>> On 4/8/14 3:30 AM, Anton Ivanov wrote:
>>>
>>>>> 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.
>>
>> Since countCpuInstructions have use the same code I asked for something like next in based class:
>>
>> protected int countCpuInstructions(byte[] nativeCode, int codeSize, byte[] mask, byte[] opcode) {
>> int n = 0;
>> for (int i = 0, size = nativeCode, length - codeSize; i < size; i++) {
>> int j = 0;
>> for (; j < codeSize; j++) {
>> if (nativeCode[i + j] & mask[j]) != opcode[j]) {
>> break;
>> }
>> }
>> if (j == codeSize) {
>> n++;
>> }
>> }
>> return n;
>> }
>>
>> For matching whole byte you use 0xFF mask. For bytes you don't want to compare mask = 0 and opcode = 0.
>>
>> Regards,
>> Vladimir
>>
>>>
>>>> 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
>>>
>
More information about the hotspot-compiler-dev
mailing list