JIT code generation for Long/Integer.compare

[Aleksey Shipilev]

Hi Ian,

I filed:
  https://bugs.openjdk.java.net/browse/JDK-8137309

I can see some improvement with your patch, mostly when Longs are
random, and therefore branch profile is polluted, and current generated
code is not laid out well. Current Hotspot seems to generate multiple
redundant cmp-s, which looks like a generic issue that should be
addressed. See e.g. the very first disassembly here:
  http://cr.openjdk.java.net/~shade/8137309/BiasedCompareTo.java

In some other cases, patched version is regressing. It seems to be the
interaction with intrinsic not using any branch profile to figure out
which option is more frequent, and some minor regalloc/constant
propagation issues. See e.g.:
  http://cr.openjdk.java.net/~shade/8137309/PredictedCompareTo.java

With that, fixing the optimizer/codegen without resorting to point
intrinsics does look like a better course of action.

Thanks,
-Aleksey

On 09/28/2015 04:44 AM, Ian Rogers wrote:
> Thanks. I'm not disagreeing with anything here. is_x2logic is 45 lines
> long and doing less (at least half) pattern matching work than would be
> required for compare. Wrt implementation approach, ReverseBytesI is
> purely an intrinsic but could also be implemented by pattern matching,
> which fwiw is how its implemented in Jikes RVM as part of instruction
> selection. With hindsight a high/ideal level BURS matching phase would
> be nice. The intrinsic implementation is trivially correct and easy for
> developers to target, whereas pattern matching can be buggy, hard to
> test, and so on. Its inherently more LOC complex. I have long thought
> pattern matching is the more laudable approach, but there's also a
> certain amount of pragmatism when selecting between an intrinsic and
> pattern matching. The scale of the proposed pattern match is beyond the
> size of any equivalent in C2, so I have concerns around correctness and
> in the consistency of the codebase in implementing it this way.
> 
> If I may explain some of the background for this change, aside pure
> performance. There is a common bug pattern of:
> 
> class MyFoo implements Comparable<MyFoo> {
>   private long x;
>   int compareTo(MyFoo o) {
>     return (int)(o.x - x);
>   }
> }
> 
> As the int cast doesn't preserve the sign of the long subtract, subtract
> results that overflow 31-bits may produce incorrect sort orders.
> Rewriting the above code to use Long.compare is currently a performance
> loss, although far more correct. What's true of long is also somewhat
> true for ints as the subtract approach doesn't behave correctly around
> Integer.MIN_VALUE. Having Long.compare and Integer.compare flagged as
> intrinsics would go someway to remove developers reticence to prefer it
> over the pervasive subtract approach which is naively considered more
> performant.
> 
> Thanks,
> Ian
> 
> 
> On Sat, Sep 26, 2015 at 12:02 PM, John Rose <john.r.rose at oracle.com
> <mailto:john.r.rose at oracle.com>> wrote:
> 
>     On Sep 25, 2015, at 7:35 PM, Vladimir Kozlov
>     <vladimir.kozlov at oracle.com <mailto:vladimir.kozlov at oracle.com>> wrote:
>>
>>     For pattern matching I mean ideal transformation in ideal graph.
>>     See, fro example, is_x2logic() in cfgnode.cpp and other
>>     transformations for Phi node.
>>
>>     You will still need new CmpI3 node and changes in .ad file.
> 
>     +1  Ideally, we pattern-match the body of the method we know about and
>     generate the new IR, and then two good things happen:  Other expressions
>     we didn't know about also pattern match, and after looking at the
>     pattern
>     logic we discover straightforward generalizations that go beyond the
>     method
>     we knew about at first.
> 
>     — John
> 
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20150929/a10932c0/signature-0001.asc>