[14] RFR(S): 8233656: assert(d->is_CFG() && n->is_CFG()) failed: must have CFG nodes

[Vladimir Ivanov]

Hi Tobias,

>> My concerns is that other usages of PhaseGVN::is_dominator() may be affected the same way as well.
> 
> The method currently has two implementations:
> - PhaseIdealLoop::is_dominator
> - PhaseGVN/PhaseIterGVN:is_dominator -> PhaseGVN::is_dominator_helper
> 
> Both assert is_CFG() for the arguments so it's the callers responsibility to ensure that.
> 
>> It looks like PhaseGVN::is_dominator_helper() would benefit from additional checks:
>>
>> bool PhaseGVN::is_dominator_helper(Node *d, Node *n, bool linear_only) {
>>    if (d->is_top() || n->is_top()) {
>>      return false;
>>    }
>>    assert(d->is_CFG() && n->is_CFG(), "must have CFG nodes");
>>    ...
> 
> Do you mean simply converting the assert to a check or adding additional asserts?

(Assuming in(0) == NULL or TOP or Proj(_, TOP) are the only cases we 
need to care about.)

Currently, ConstraintCastNode::dominating_cast() does NULL check, 
PhaseGVN::is_dominator_helper() does TOP check, but Proj(TOP) is left 
uncovered and it reaches d->is_CFG() check.

Your fix covers both TOP and Proj(_, TOP) on 
ConstraintCastNode::dominating_cast() by doing is_CFG() check.

What I'm in favor of is to handle Proj(TOP) case explicitly and there 
are other places in the code base which do that. (It may sound too 
subtle, but it doesn't look right when the code performs in(0)->is_CFG() 
check outside of an assert.)

I mentioned InitializeNode::detect_init_independence() as an example how 
control info processing can be done [1]. It covers NULL, TOP, and 
Proj(TOP) cases, but without is_CFG() check.

Considering current shape of ConstraintCastNode::dominating_cast() and 
that PhaseGVN::is_dominator_helper() already assumes non-NULL inputs, 
putting control normalization before TOP checks should solve the problem 
as well:

   bool PhaseGVN::is_dominator_helper(Node *d, Node *n, bool linear_only) {
   + if (d->is_Proj())  d = d->in(0);
   + if (n->is_Proj())  n = n->in(0);
     if (d->is_top() || n->is_top()) {
       return false;
     }

Best regards,
Vladimir Ivanov

[1]

     Node* ctl = n->in(0);
     if (ctl != NULL && !ctl->is_top()) {
       if (ctl->is_Proj())  ctl = ctl->in(0);
       ...
       if (!MemNode::all_controls_dominate(n, this))

bool MemNode::all_controls_dominate(Node* dom, Node* sub) {
   if (dom == NULL || dom->is_top() || sub == NULL || sub->is_top())
     return false; // Conservative answer for dead code

>> As an example, InitializeNode::detect_init_independence() does some control normalization first:
> 
> Yes but that method processes data and control edges.
> 
> Thanks,
> Tobias
>