8038953,Add sanity tests for BMI1 and LZCNT instructions
Anton Ivanov
anton.ivanov at oracle.com
Thu Apr 10 18:52:37 UTC 2014
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
>>>>
>>
--
Best regards,
Anton Ivanov
More information about the hotspot-compiler-dev
mailing list