RFR(L): 8029015: PPC64 (part 216): opto: trap based null and range checks

[Vladimir Kozlov]

On 11/25/13 5:24 AM, Lindenmaier, Goetz wrote:
> Hi Vladimir,
>
> I made a new webrev:
> http://cr.openjdk.java.net/~goetz/webrevs/8029015-1-trch/
>
>> First general question. Why you did not create specialized Mach node
>> like MachNullCheckNode to handle such cases and using existing code in
>> lcm.cpp? Or it would be more complicated changes?
> I think lcm is not a good place to create new nodes. The right place is the
> matcher. Also, I would have to implement a platform independent factory
> method to create the nodes, that is implemented on each platform.
> And to specify a node that  inherits from a shared MachNode, mayby
> MachTrapCheckNode, requires a rule with an idea opcode.
> Finally, I think the nodes checking ranges are not handles in lcm?

I thought that in lcm you will have more opportunity to generate trap 
checks. Anyway, it is not correctness issue but performance (and may be 
not).
I am fine with what you have now.

Also I was looking on predicates for these nodes in ppc.ad and I thought 
may be you can factor out repetitive checks to a separate method in IfNode:

   predicate(TrapBasedNullChecks &&
             _kids[0]->_leaf->as_Bool()->_test._test == BoolTest::ne &&
             _leaf->as_If()->_prob >= PROB_ALWAYS);

you will get more clean code there and you will not need next change:

+  AD.addInclude(AD._DFA_file, "opto/cfgnode.hpp");  // Use PROB_MAX in 
predicate.


>> To avoid changes in closed sources can you put TrapBasedNullChecks and
>> TrapBasedRangeChecks into opto/c2_globals.hpp (thay are used only by C2)
>> and set them to 'true' if FLAG_IS_DEFAULT() in
>> src/cpu/ppc/vm/vm_version_ppc.cpp ?
> It's in ARCH_FLAGS, so that it can be a develop flag on sparc/x86 and a
> product on ppc. But being in ARCH_FLAGS, closed sources should not need
> adaptions.  There is no ARCH_FLAGS for the c2_globals_<arch>.hpp files.

No, they can not be ARCH_FLAGS because they are referenced in C2 shared 
code (PhaseCFG::fixup_flow()):

+      if ((TrapBasedNullChecks || TrapBasedRangeChecks) &&

They are C2 flags. To compile this code for closed sources flags have to 
be defined. I asked to define them in opto/c2_globals.hpp because 
otherwise I will have to add them to ARCH_FLAGS in closed globals_<arc>.hpp

The example of strict ARCH_FLAGS is UseAddressNop which is used only in 
src/cpu/x86/vm/* files.

>> You moved _allowed_reasons code to Compile::Init() which is called for
>> stubs too. But you removed if (C->is_method_compilation()) check. Why?
> Did I?  It's still in gcm! And I also check it in branches_to_uncommon_trap().

I phrased my question not accurately. The code which set 
_allowed_reasons was done before only under (C->is_method_compilation()) 
check.
And I see that you added the check to Compile::Init() in this version so 
it is good.

>> Where branches_to_uncommon_trap() is used?
> In the predicates in ppc.ad, but also In our zArch/s390 port.

I don't see it in the ppc.ad version I have, that is why I asked:

http://hg.openjdk.java.net/ppc-aix-port/jdk8/hotspot/file/09f97b967480/src/cpu/ppc/vm/ppc.ad

Thanks,
Vladimir

>
>> output.cpp: can you replace next node to move is_TrapBasedCheckNode() to
>> MachNode class?:
> Done.  But it's the only typecheck method in MachNode now, all the others
> are in node, as the one used a few lines above: is_MachNullCheck.  But well, it's not tied
> to a node class as the others.
>
> Thanks for the review and best regards,
>    Goetz.
>
>> Thanks,
>> Vladimir
>
> On 11/22/13 6:41 AM, Lindenmaier, Goetz wrote:
>> Hi,
>>
>> I prepared a webrev for the trap based checks feature used on PPC:
>> http://cr.openjdk.java.net/~goetz/webrevs/8029015-0-trch/
>>
>> PPC has the tdi instruction that does a compare and raises SIGTRAP
>> if the compare is successful.
>> With this instruction conditional branches leading to uncommon
>> traps can be implemented very efficiently.
>> This is especially needed on AIX, where there are almost no
>> possibilities for ImplicitNullChecks as the zero page is not
>> protected.
>>
>> On linux, this accounts for about 2% jvm2008 performance.
>>
>> Possibilities for trap based range checks and trap based null checks
>> are recognized during matching. To support this, we added a method
>> branches_to_uncommon_trap() to be used in the predicate during
>> matching.
>> The computation of the final block layout must know about this,
>> as it must place the fallthrough properly and adapt the condition
>> in the tdi instruction.
>> We added a method is_TrapBasedCheckNode so these nodes
>> can be recogized in a platform independent way.
>>
>> Please review and test this change.
>>
>> Best regards,
>>     Goetz
>>