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

[Lindenmaier, Goetz]

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?

> 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.

> output_h.cpp: since you touching the code can you add explicit != 0 for 
> all checks (ins_cost and ins_short_branch)?
Done.  Also inserted space after commas.

> block.cpp: Could you rename local 'bra' in no_flip_branch() and in new 
> code in fixup_flow()? It is not good name :) Use full name 'branch'.
Done.  Also fixed spaces and added {}.

> And, please, move new code in PhaseCFG::fixup_flow() into separate method.
Done, but it doesn't look good as I have to pass a lot of context in there.

> 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().

> matcher.cpp: we usually use C variable name for Compile object:
Fixed.  It's bad C is not available in the predicates.  It's quite useful
there.  You need it in post_store_load_barrier(), too.

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

> 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
>