RFR(S): 8077843: adlc: allow nodes that use TEMP inputs in expand rules.

[Vladimir Kozlov]

Okay. I see. Looks good then. I will push it.

Thanks,
Vladimir

On 4/17/15 12:25 AM, Lindenmaier, Goetz wrote:
> HI Vladimir,
>
> the node generated in the Expand has a match rule itself.
> I checked, all the TEMPs we use are in nodes with match rules.
> Anyways, you get a syntax error if not:
> "Syntax Error: <node>: TEMPs without match rule isn't supported"
>
> We use nodes with match rules in expands a lot, as the match rule
> is required for a lot of things. For example to generate the proper
> ideal opcode, get it in the array must_clone etc.
> If we don't want the nodes to be matched, we add predicate(false).
> In our internal VM, we use nodes with TEMPs in expands on ia64 a lot.
>
> For the code generated for the nodes I intend to contribute, see
> below.
>
> Best regards,
>    Goetz.
>
>
> // Card-mark for CMS garbage collection.
> // This cardmark does an optimization so that it must not always
> // do a releasing store. For this, it gets the address of
> // CMSCollectorCardTableModRefBSExt::_requires_release as input.
> // (Using releaseFieldAddr in the match rule is a hack.)
> instruct storeCM_CMS(memory mem, iRegLdst releaseFieldAddr, flagsReg crx) %{
>    match(Set mem (StoreCM mem releaseFieldAddr));
>    effect(TEMP crx);
>    predicate(false);
>    ins_cost(MEMORY_REF_COST);
>
>    // See loadConP.
>    ins_cannot_rematerialize(true);
>
>    format %{ "STB     #0, $mem \t// CMS card-mark byte (must be 0!), checking requires_release in [$releaseFieldAddr]" %}
>    ins_encode( enc_cms_card_mark(mem, releaseFieldAddr, crx) );
>    ins_pipe(pipe_class_memory);
> %}
>
> // Card-mark for CMS garbage collection.
> // This cardmark does an optimization so that it must not always
> // do a releasing store. For this, it needs the constant address of
> // CMSCollectorCardTableModRefBSExt::_requires_release.
> // This constant address is split off here by expand so we can use
> // adlc / matcher functionality to load it from the constant section.
> instruct storeCM_CMS_ExEx(memory mem, immI_0 zero) %{
>    match(Set mem (StoreCM mem zero));
>    predicate(UseConcMarkSweepGC);
>
>    expand %{
>      immL baseImm %{ 0 /* TODO: PPC port (jlong)CMSCollectorCardTableModRefBSExt::requires_release_address() */ %}
>      iRegLdst releaseFieldAddress;
>      flagsReg crx;
>      loadConL_Ex(releaseFieldAddress, baseImm);
>      storeCM_CMS(mem, releaseFieldAddress, crx);
>    %}
> %}
>
>
> MachNode* storeCM_CMSNode::Expand(State* state, Node_List& proj_list, Node* mem) {
>    Compile* C = Compile::current();
>    // Add projection edges for additional defs or kills
>    // TEMP crx
>    MachTempNode *def;
>    def = new MachTempNode(state->MachOperGenerator(FLAGSREG));
>    add_req(def);
>
>    return this;
> }
>
> MachNode* storeCM_CMS_ExExNode::Expand(State* state, Node_List& proj_list, Node* mem) {
>    Compile* C = Compile::current();
>    MachOper *op0 = new immLOper(
> #line 6571 "/sapmnt/home1/d045726/oJ/8077838-ppc-hs-comp/src/cpu/ppc/vm/ppc.ad"
> 0 /* TODO: PPC port (jlong)CMSCollectorCardTableModRefBSExt::requires_release_address() */
> #line 527 "../generated/adfiles/ad_ppc_64_expand.cpp"
> );
>    MachOper *op1 = new iRegLdstOper();
>    MachOper *op2 = new flagsRegOper();
>    MachNode *tmp0 = this;
>    MachNode *tmp1 = this;
>    MachNode *tmp2 = this;
>    MachNode *tmp3 = NULL;
>    MachNode *tmp4 = NULL;
>    MachNode *tmp5 = NULL;
>    unsigned num0 = 0;
>    unsigned num1 = opnd_array(1)->num_edges();
>    unsigned num2 = opnd_array(2)->num_edges();
>    unsigned idx0 = oper_input_base();
>    if (mem == (Node*)1) {
>      idx0--; // Adjust base because memory edge hasn't been inserted yet
>    }
>    unsigned idx1 = idx0 + num0;
>    unsigned idx2 = idx1 + num1;
>    unsigned idx3 = idx2 + num2;
>    MachNode *result = NULL;
>
>    loadConL_ExNode *n0 = new loadConL_ExNode();
>    n0->add_req(_in[0]);
>    ((MachTypeNode*)n0)->_bottom_type = bottom_type();
>    n0->set_opnd_array(0, state->MachOperGenerator(IREGLDST));
>    tmp4 = n0;
>    n0->set_opnd_array(1, op0->clone()); // baseImm
>    if(tmp3 != NULL)
>      n0->add_req(tmp3);
>    result = n0->Expand( state, proj_list, mem );
>
>    storeCM_CMSNode *n1 = new storeCM_CMSNode();
>    n1->add_req(_in[0]);
>    ((MachTypeNode*)n1)->_bottom_type = bottom_type();
>    n1->set_opnd_array(0, state->MachOperGenerator(UNIVERSE));
>    if (mem != (Node*)1) {
>      n1->add_req(_in[1]);	// Add memory edge
>    }
>    n1->set_opnd_array(1, opnd_array(1)->clone()); // mem
>    if(tmp1 == this) {
>      for(unsigned i = 0; i < num1; i++) {
>        n1->add_req(_in[i + idx1]);
>      }
>    }
>    else n1->add_req(tmp1);
>    tmp1 = n1;
>    n1->set_opnd_array(2, op1->clone()); // releaseFieldAddress
>    if(tmp4 != NULL)
>      n1->add_req(tmp4);
>    n1->set_opnd_array(3, op2->clone()); // crx
>    if(tmp5 != NULL)
>      n1->add_req(tmp5);
>    result = n1->Expand( state, proj_list, mem );
>
>
>    return result;
> }
>
>
>
> -----Original Message-----
> From: hotspot-dev [mailto:hotspot-dev-bounces at openjdk.java.net] On Behalf Of Vladimir Kozlov
> Sent: Freitag, 17. April 2015 02:28
> To: hotspot-dev at openjdk.java.net
> Subject: Re: RFR(S): 8077843: adlc: allow nodes that use TEMP inputs in expand rules.
>
> Here is what Tom R said during review of TEMP implementation:
>
>   >> 3. In formssel.cpp you restricted TEMP usage only to instruction
>   >> with match rule, why? What about effect()?:
>   >> + bool InstructForm::has_temps() {
>   >> +   if (_matrule) {
>   >
>   >TEMP isn't allowed for instructions which don't have match rules.  The
>   >reason is that if an instruct doesn't have a match rule the only way
>   >it can be constructed is by an expand rule.  expand rules look like
>   >explicit invocations of encodings but TEMPs are synthetic so you can't
>   >properly write an expand rule to work with TEMP.  I mean you could
>   >make it work but I decided not to support it.  I'll make this more
>   >explicit.
>
> So I am not sure that just removing the assert will be enough. Please,
> verify.
>
> Regards,
> Vladimir
>
> On 4/15/15 6:08 AM, Lindenmaier, Goetz wrote:
>> Hi,
>>
>> I have a tiny fix that allows nodes that use TEMP inputs / TEMP effect in
>> expand rules.
>>
>> Currently an assertion fires if you do so.
>> This is harmless, though, as the TEMP node is added in the Expand() of the used node,
>> which is called by the Expand() of the node being expanded.
>> Probably the assertion was meant for the node being expanded.
>>
>> The change simply removed the assertion.
>>
>> Please review this change.  I please need a sponsor.
>> http://cr.openjdk.java.net/~goetz/webrevs/8077843-adlcTEMP/webrev.01/
>>
>> I need this feature for a change I intend to do in the ppc.ad file.
>>
>> Best regards,
>>     Goetz.
>>