RFR(L): 8029015: PPC64 (part 216): opto: trap based null and range checks
Vladimir Kozlov
vladimir.kozlov at oracle.com
Wed Nov 27 12:40:59 PST 2013
Yes, it is acceptable because flags are in places where they should be.
I will deal with closed changes.
interp_masm_ppc_64.cpp diffs are empty.
load_heap_oop_with_trap_null_check() is commented.
In macroAssembler_ppc.inline.hpp TrapBasedNullChecks is global:
+ COMPILER2_PRESENT(assert(TrapBasedNullChecks, "sanity"));
c2_globals_sparc.hpp comment (x86): // Not needed on x86.
opto/c2_globals.hpp can you extend flag's comment explaining when it is
used.
globals.hpp: typo in comment 'don;t'
About allowed_deopt_reasons(), sorry I missed it before but it should be
calculated after we done parsing, better just before code_gen() call in
Compile constructor which compiles methods. Make it special Compile's
method.
The reason is too_many_traps() checks total number in all inlined
methods: trap_count(reason).
Regards,
Vladimir
On 11/27/13 7:00 AM, Lindenmaier, Goetz wrote:
> Hi Vladimir,
>
> I tried to move the flags to c2_globals, but it seems not
> appropriate for TrapBasedNullChecks.
> It's used wherever other platforms do an implicit null check --
> which is not always guarded by ImplicitNullCheck. E.g.
> MacroAssembler::null_check() (which is called null_check_throw
> on ppc.) and the loads following the call to it.
> Many of these are in the template interpreter, and as we intend
> to do a PPC port of the template interpreter, we should consider this.
>
> I prepared a new webrev where I moved TrapBasedNullChecks
> to globals.hpp, and TrapBasedRangeChecks to c2_globals.hpp.
> http://cr.openjdk.java.net/~goetz/webrevs/8029015-2-trch/
>
> Is this acceptable to you?
> I'm sorry if this means even more closed changes for you.
>
> Best regards,
> Goetz.
>
> PS: Did you intenionally only reply to me because you mention
> the closed sources?
>
> -----Original Message-----
> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
> Sent: Dienstag, 26. November 2013 18:00
> To: Lindenmaier, Goetz
> Subject: Re: RFR(L): 8029015: PPC64 (part 216): opto: trap based null and range checks
>
> On 11/26/13 12:50 AM, Lindenmaier, Goetz wrote:
>> Hi Vladimir,
>>
>>> I don't see it in the ppc.ad version I have, that is why I asked
>> The predicate should look like this:
>> predicate(TrapBasedNullChecks &&
>> _kids[0]->_leaf->as_Bool()->_test._test == BoolTest::ne &&
>> _leaf->as_If()->_prob >= PROB_LIKELY_MAG(4) &&
>> Matcher::branches_to_uncommon_trap(_leaf));
>> I added the last check to jdk8 about 4 weeks ago, when merging recent
>> improvements from our VM:
>> http://hg.openjdk.java.net/ppc-aix-port/jdk8/hotspot/rev/3f89fd0a071d
>
> Okay.
>
>>
>>> can factor out repetitive checks to a separate method in IfNode
>> You mean I should add something like this:
>> _leaf->as_If()->branches_unlikely() { return _prob >= PROB_LIKELY_MAG(4) }
>> Maybe also including the condition to test?
>> I don't think branches_to_uncommon_trap() should be called from within
>> an If node, as it's a graph pattern match that's got nothing to do in a node
>> implementation.
>> Having it in the ad file is more flexible. E.g., I can play around with the
>> probabilities.
>
> Okay, if it helps you.
>
>>
>> The ARCH_FLAGS issue:
>> I admit it's not intended to define ARCH_FLAGS that are used in
>> platform independent code. It works though, if they are
>> defined on all platforms using C2.
>> The advantage is that I can make the flag 'product' on PPC,
>> and 'debug' on the others.
>> I should be able to switch off these flags, as debugging is
>> impossible if the VM throws SIGTRAP all the time. So if
>> we have to debug a product VM, I must switch them off.
>
> I understand what you want to do - do not generated guarded code on other platforms without #ifdef.
> But it will not help since we moved code into separate method. Yes, inlined code will not be generated but separate
> method will be.
>
> I wish we had better way without polluting all globals_<arch>.hpp files.
> I would need to prepare closed changes, get reviews and test before the push.
>
> Thanks,
> Vladimir
>
>>
>> By the way, I have a flag UseSIGTRAP in the ppc ARCH_FLAGS.
>> If I move that to globals.hpp, I could protect the code I
>> changed in the os_linux.cpp file by UseSIGTRAP instead of
>> #ifdef PPC64. But it must be a product flag, too. Maybe this
>> does not matter in this case, as the added code in os_linux.cpp
>> should not be performance relevant.
>>
>> Best regards,
>> Goetz.
>>
>>
>>
>> -----Original Message-----
>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>> Sent: Tuesday, November 26, 2013 12:21 AM
>> To: Lindenmaier, Goetz; 'hotspot-dev at openjdk.java.net'; 'ppc-aix-port-dev at openjdk.java.net'
>> Subject: Re: RFR(L): 8029015: PPC64 (part 216): opto: trap based null and range checks
>>
>> 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
>>>>
More information about the ppc-aix-port-dev
mailing list