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