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

[Vladimir Kozlov]

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