RFR: JDK-8272574: Crashes in PhaseIdealLoop::build_loop_late_post_work [v2]

[王超]

On Wed, 18 Aug 2021 08:47:02 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> > We have two other bugs failing with the same assert: JDK-8272562 and JDK-8271954. Could they be related?
> 
> They are both regression from JDK-8252372 (split if optimization) and unrelated to this bug which is related to loop predication.

Thank you for your review and explanation.


> I'm not sure if this is too restrictive to not apply loop predication at all. It seems to me that this `248 RangeCheck` could indeed be moved out of the loop but this pin of `398 LoadI` does not allow it which is unfortunate. Can we fix that somehow? Are we too strict when setting the control input in `LoadNode::split_through_phi()`?
> 
> I'm currently looking at:
> 
> https://github.com/openjdk/jdk/blob/30b0f820cec12b6da62229fe78a528ab3ad0d134/src/hotspot/share/opto/memnode.cpp#L1627-L1629
> 
> 
> wondering if we could set the control input of the cloned load to the same control input as the memory input of the phi (if the control input of the old load was the region of the phi through which we split)? Something like this:
> ```
>       if (this->in(0) == region) {
>         if (mem->is_Phi() && (mem->in(0) == region) && mem->in(i)->in(0) != NULL) {
>           x->set_req(0, mem->in(i)->in(0)); // Use same control as memory
>         } else {
>           x->set_req(0, in);
>         }
>       } 
> ```

I agree with you and your suggested fix looks more elegant in preventing such pinned load. I will analyze and test it to see whether it has unexpected side effects.


What do you think about if the following guard is added to `IdealLoopTree::is_range_check_if`, which prevents loop predications float to predication point with dependency on the skipped predications(which is right after the predication point). When your suggested fix is applied, there is no pinned load currently. But I'm not sure whether it will occur again in the future.

  if (offset && phase->has_ctrl(offset)) {
    Node* offset_ctrl = phase->get_ctrl(offset);
    if (phase->get_loop(predicate_proj) == phase->get_loop(offset_ctrl) &&
        phase->is_dominator(predicate_proj, offset_ctrl)) {
      return false;
    }
  }

-------------

PR: https://git.openjdk.java.net/jdk/pull/5142