[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-compiler-dev
Subject:    Re: RFR: 8317721: RISC-V: Implement CRC32 intrinsic [v5]
From:       urniming <duke () openjdk ! org>
Date:       2024-02-28 7:18:46
Message-ID: 1ciFje_z8G3aP849ZeIfhtu890U1omYhobbRaxuQ3HI=.a000a953-c7f0-4604-8ffc-537af4c58328 () github ! com
[Download RAW message or body]

On Mon, 19 Feb 2024 17:39:18 GMT, ArsenyBochkarev <duke@openjdk.org> wrote:

> > Hi everyone! Please review this port of \
> > [AArch64](https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp#L4224) \
> > `_updateBytesCRC32`, `_updateByteBufferCRC32` and `_updateCRC32` intrinsics. This \
> > patch introduces only the plain (non-vectorized, no Zbc) version. 
> > ### Correctness checks
> > 
> > Tier 1/2 tests are ok.
> > 
> > ### Performance results on T-Head board
> > 
> > #### Results for enabled intrinsic:
> > 
> > Used test is `test/micro/org/openjdk/bench/java/util/TestCRC32.java`
> > 
> > > Benchmark                                             |  (count) |  Mode | Cnt  \
> > > | Score |   Error |  Units |
> > > --- | ---- | ----- | --- | ---- | --- | ---- |
> > > CRC32.TestCRC32.testCRC32Update  |     64  | thrpt     | 24 | 3730.929 | 37.773 \
> > > | ops/ms |
> > > CRC32.TestCRC32.testCRC32Update  |    128 |  thrpt    | 24 | 2126.673 |  2.032 \
> > > | ops/ms |
> > > CRC32.TestCRC32.testCRC32Update  |    256 | thrpt    |  24 | 1134.330 |  6.714 \
> > > | ops/ms |
> > > CRC32.TestCRC32.testCRC32Update  |    512 | thrpt    |  24 |  584.017 |  2.267 \
> > > | ops/ms |
> > > CRC32.TestCRC32.testCRC32Update  |   2048 |  thrpt   |   24 |  151.173 |  0.346 \
> > > | ops/ms |
> > > CRC32.TestCRC32.testCRC32Update  |   16384 | thrpt |  24 |   19.113 |  0.008 | \
> > > ops/ms |
> > > CRC32.TestCRC32.testCRC32Update  |  65536 | thrpt  | 24  |   4.647 | 0.022 | \
> > > ops/ms |
> > 
> > #### Results for disabled intrinsic:
> > 
> > > Benchmark                                            | (count)  |  Mode | Cnt | \
> > > Score  |  Error   | Units     |
> > > --------------------------------------------------- | ---------- | --------- | \
> > > ---- | ----------- | --------- | ---------- | 
> > > CRC32.TestCRC32.testCRC32Update |      64    |  thrpt   | 15  | 798.365 | \
> > > 35.486 | ops/ms |
> > > CRC32.TestCRC32.testCRC32Update |     128   |  thrpt   | 15  | 677.756 | 46.619 \
> > > | ops/ms |
> > > CRC32.TestCRC32.testCRC32Update |     256   |  thrpt   | 15  | 552.781 | 27.143 \
> > > | ops/ms |
> > > CRC32.TestCRC32.testCRC32Update |     512   |  thrpt   | 15  | 429.304 | 12.518 \
> > > | ops/ms |
> > > CRC32.TestCRC32.testCRC32Update |    2048  |  thrpt   | 15  | 166.738 |  0.935  \
> > > | ops/ms |
> > > CRC32.TestCRC32.testCRC32Update |   16384 |  thrpt   | 15  |  25.060  | 0.034   \
> > > | ops/ms |
> > > CRC32.TestCRC32.testCRC32Update |   65536 |  thrpt   | 15  |   6.196   | 0.030  \
> > > | ops/ms |
> 
> ArsenyBochkarev has updated the pull request incrementally with three additional \
> commits since the last revision: 
> - Re-selected register for tmp's in kernel_crc32
> - Use shNadd in update_byte_crc32
> - Optimize by1_loop

src/hotspot/cpu/riscv/macroAssembler_riscv.hpp line 1247:

> 1245:         bool upper);
> 1246:   void update_byte_crc32(Register crc, Register val, Register table);
> 1247: 

Hi, one thing I'm confused about here, why do these three functions need to be in the \
`COMPILER2` macro ?

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/17046#discussion_r1505452747


[prev in list] [next in list] [prev in thread] [next in thread]