RFR(L) 8073583: C2 support for CRC32C on SPARC

John Rose john.r.rose at oracle.com
Tue May 5 00:39:23 UTC 2015


On May 4, 2015, at 2:30 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
> 
> http://cr.openjdk.java.net/~kvn/8073583/webrev.00/ <http://cr.openjdk.java.net/~kvn/8073583/webrev.00/>
> https://bugs.openjdk.java.net/browse/JDK-8073583 <https://bugs.openjdk.java.net/browse/JDK-8073583>
> 
> Contributed by: James Cheng

I took a quick look at the assembly code.  I have a few comments.

The magic constant 128 (size of parallel decomposition chunk) needs naming; suggest "chunk length" or something like that.  When the constant is defined, it needs to be explained also.

I'm glad to see newer sparc instructions getting used, especially xmulx to reassociate crc.  The magic constants used to shift the CRC codes by 1024 should be named and explained.

The code is unnecessarily monolithic.  It will be more readable and maintainable if some easy factorizations are applied.  The factorizations themselves can add value beyond readability of this new code.

Specifically, the sethi/or3/or3 should be replaced by set64 macros.  Forget about the clever hoisting of 1<<32 into G1; just improve set64 to cover 34-bit constants in three instructions:  sethi/sllx/or3.  The sllx shifts up by 3 to make room for a simm13.

(If we don't want to disturb set64, then add a new macro set34.  But I say go for it.  I wish the set64 macro got a little more love.)

A couple of places use 'sethi' instead of the 'set' macro; I'd use 'set' to avoid puzzling the reader with details of micro-optimization, although it's a matter of style.

The long sequences which perform byte reversal should use a macro; you can name it 'reverse_bytes_32'.  If they are truly optimal then the macro will hide the noise.  But if they are not, it will be much easier to swap in better instructions.  The C2 intrinsics called ReverseBytes use ASI_PRIMARY_LITTLE to a stack temp, which is probably better.  In any case, the decision needs to be hidden in a macro, not spread around in unrelated code.

There are some minor things that would tidy the code.  The first two instructions sllx/srlx should be replaced by clruwu (srl 0).  The uses of cc's in the "subcc" instructions are all useless and slightly misleading; should be "sub" or (better) "dec".

I personally find the use of cmp_and_brx_short peculiar with the constant 32, just because I know 32 doesn't actually work with the intended instruction.  But I suppose it is OK, since the macro expands to something that works.

— John
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20150504/55deb4d8/attachment.html>


More information about the hotspot-compiler-dev mailing list