8038953,Add sanity tests for BMI1 and LZCNT instructions

Vladimir Kozlov vladimir.kozlov at oracle.com
Thu Apr 10 18:58:47 UTC 2014


Looks good.

Thanks,
Vladimir

On 4/10/14 11:52 AM, Anton Ivanov wrote:
> On 10.04.2014 21:25, Vladimir Kozlov wrote:
>> 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:
> agree. here is the new wevrev:
> http://cr.openjdk.java.net/~iignatyev/aaivanov/8038953/webrev.05/
>
>
>>
>>         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