RFR(L): 8003854: PPC64 (part 115): Introduce lateExpand that expands nodes after register allocation.

[Vladimir Kozlov]

On 11/13/13 5:20 AM, Lindenmaier, Goetz wrote:
> Hi Vladimir,
>
> thanks for the review!
> I'm already working on your comments, more some later in a
> comprehensive mail.  For now: would postalloc_expand be fine,
> too (two ls)?

Yes, definitely.

Thanks,
Vladimir

>
> Best regards,
>    Goetz
>
>
> -----Original Message-----
> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
> Sent: Dienstag, 12. November 2013 21:53
> To: Lindenmaier, Goetz; hotspot-dev at openjdk.java.net; ppc-aix-port-dev at openjdk.java.net
> Subject: Re: RFR(L): 8003854: PPC64 (part 115): Introduce lateExpand that expands nodes after register allocation.
>
> Hi Goetz,
>
> It is good feature. Can you name it postaloc_expand to be clear when we
> do it?
>
> Names. I know it is picking but for methods and .ad statements names we
> use low case and underscore. Camel style is used for class names.
>
> I know this was written long time ago but we are trying now to not use
> separate enc_class unless it is used by several instructions. Can you
> also implement ability to write postaloc_expand inside mach instruction
> (As we do now for ins_encode)?  You can do it as separate changes later
> if it a lot of work.
>
> instruct divI_reg_reg_Ex(iRegI dst, iRegIsafe src1, iRegIsafe src2) %{
>     match(Set dst (DivI src1 src2));
>     predicate(UseNewCode);
>     ins_cost((2+71)*DEFAULT_COST);
>     format %{ "SRA     $src2,0,$src2\n\t"
>               "SRA     $src1,0,$src1\n\t"
>               "SDIVX   $src1,$src2,$dst" %}
>     postaloc_expand %{
>       MachNode *m1 = new (C) divI_reg_reg_SRANode();
>       MachNode *m2 = new (C) divI_reg_reg_SRANode();
>       MachNode *m3 = new (C) divI_reg_reg_SDIVXNode();
>       ...
>     %}
> %}
>
> Are you allow branches in late expand? I assume not because you don't
> have separate graph's blocks for that. It would be nice to have some
> verification code which enforces restrictions.
>
> Also it would be nice if in lateExpand rule you need only define nodes
> and connect them and the rest (opnds clonning, RA info patching and
> pushing result nodes) be done by adlc. But it will require more code in
> adlc so it may be for the future. I am worried that current code
> requires very careful coding: which operand to clone and to which assign
> it, which RA methods to use for resetting (set1(), set2(), set_pair()).
> It is very bugs prone.
>
>
> adlparse.cpp: // No pipe required for expands.
>
> +    // No pipe required for late expand.
> +    if (instr->expands() || instr->lateExpands()) {
>
> Will it be difficult to totally separate lateExpands from ins_encode?
> Your current code use _insencode + _is_lateExpand. Can you do one
> InsEncode* _lateExpand? Or it will be too intrusive?
>
>
> block.cpp:
> Node_List already has method contains(Node* n), use it in
> Block::contains() (you can define it in header file then).
>
> PhaseCFG::LateExpand():
>
> TraceLateExpand is develop flag so it will be true only for #ifdef
> ASSERT. It seems all #ifndef PRODUCT should be replaced by #ifdef ASSERT
> in this method.
>
> "These kills are not required any more after expanding" - what about
> flags kill Proj nodes? Disconnecting input nodes worries me. Do you
> clean graph after that to remove unused nodes?
>
> I need to look more on LateExpand() in next round.
>
> compile.cpp: Can you add TracePhase/TraceTime to get time spent in
> LateExpand.
>
> Why you need lateExpand() method in Node class? Generated mach nodes are
> based on MachNode so this method could be declared only in MachNode.
> (Putting new virtual method into top Node class will increase virtual
> tables for all ideal and mach nodes).
>
> Thanks,
> Vladimir
>
> On 11/12/13 8:32 AM, Lindenmaier, Goetz wrote:
>> Hi,
>>
>> I also updated this webrev to work with the latest staging repository.
>> http://cr.openjdk.java.net/~goetz/webrevs/8003854-lateExp/
>>
>> Best regards,
>>     Goetz.
>>
>> From: goetz.lindenmaier at sap.com
>> Sent: Mittwoch, 2. Oktober 2013 15:49
>> To: hotspot-dev at openjdk.java.net; ppc-aix-port-dev at openjdk.java.net; 'Vladimir Kozlov'
>> Subject: RFR(L): 8003854: PPC64 (part 115): Introduce lateExpand that expands nodes after register allocation.
>>
>> Hi,
>>
>> we extended C2 by what we call lateExpand. LateExpand expands nodes
>> after register allocation.
>> http://cr.openjdk.java.net/~goetz/webrevs/8003854-lateExp/
>>
>> Some nodes can not be expanded during matching. E.g., register
>> allocation might not be able to deal with the resulting pattern. To
>> allow better scheduling in such cases, we introduce lateExpand which
>> runs after register allocation. Whether and how nodes are expanded is
>> specified in the ad-file. See block.cpp for a detailed
>> documentation. We use this for some nodes on ppc, and extensively on
>> ia64.
>> For an example, see the webrev.
>>
>> We added some utility functions in node.hpp and block.hpp used by
>> PhaseCFG::LateExpand() or the Node::lateExpand functions.  Also,
>> MachConstantBaseNode gets the two new functions, as we need to
>> late expand it.
>>
>> To skip the walk over the IR if no lateExpand is needed, we use
>> Matcher::require_late_expand set in the ad file. This ends up in
>> ad_<cpu>.cpp, thus can not be evaluated by the C++ compiler.  If you
>> agree, I would use a "const bool EnableLateExpand" in
>> globalDefinitions_<cpu>.hpp.  (There is no suitable c2_<xxx>_<cpu>.hpp
>> file).
>>
>> We allready mailed a webrev with this change after last year's
>> JavaOne, but there was no discussion on it.
>> Again, this change is 'L', but the code is not used on other
>> platforms than PPC64 (so far).  So the impact on those platforms
>> should be minimal.
>>
>> Please review and test this change.
>>
>> Thanks and best regards,
>>     Goetz.
>>