RFR(S): 8244086: Following 8241492, strip mined loop may run extra iterations

[Doerr, Martin]

Hi Roland,

my idea was rather to check if the trip counter is already checked before the loop.
Check before loop should look like this (stride > 0 example):
CmpINode c = CountedLoop->in(1) -> IfTrue->in(0) -> If->in(1) -> Bool->in(1) -> CompI

(Maybe there's an easier way to find it where it gets generated.)

Comparison of start value:
c->in(1) == Phi(trip counter)->in(1)
with limit:
c->in(2) == CmpI(trip counter)->in(2)

If this matches we should be safe.
I haven't checked if such patterns match often enough. Just as an idea.

Best regards,
Martin


> -----Original Message-----
> From: Roland Westrelin <rwestrel at redhat.com>
> Sent: Donnerstag, 30. April 2020 13:49
> To: Doerr, Martin <martin.doerr at sap.com>; Pengfei Li
> <Pengfei.Li at arm.com>; hotspot-compiler-dev at openjdk.java.net
> Cc: nd <nd at arm.com>
> Subject: RE: RFR(S): 8244086: Following 8241492, strip mined loop may run
> extra iterations
> 
> 
> > Maybe another option would be to recognize loops which are affected by
> this problem and live with having a safepoint in the body (skip LSM).
> > AFAICS "for" loops have the check at the beginning and should not be
> affected.
> 
> That requires pattern matching to find a dominating check. That pattern
> matching has to be robust enough. I gave it a try and it's
> straightforward for a simple:
> 
> for (int i = start; i < stop; i++) {
> 
> with:
> 
>   {
>     bool found = false;
>     Node* temp_cmp = cmp->clone();
>     int limit_edge = cmp->find_edge(limit);
>     assert(limit_edge == 1 || limit_edge == 2, "");
>     temp_cmp->set_req(limit_edge == 1 ? 2 : 1, init_trip);
>     Node* dom_cmp = _igvn.hash_find(temp_cmp);
>     if (dom_cmp != NULL) {
>       Node* temp_test = test->clone();
>       temp_test->set_req(1, dom_cmp);
>       Node* dom_cmp = _igvn.hash_find(temp_test);
>       if (dom_cmp != NULL) {
>         temp_cmp->destruct();
>         temp_test->destruct();
>         for (DUIterator_Fast imax, i = dom_cmp->fast_outs(imax); i < imax &&
> !found; i++) {
>           Node* u = dom_cmp->fast_out(i);
>           if (u->is_If()) {
>             ProjNode* proj = u->as_If()->proj_out(back_control->as_Proj()-
> >_con);
>             proj->dump();
>             if (is_dominator(proj, x)) {
>               found = true;
>             }
>           }
>         }
>       }
>     }
>   }
> 
> But that fails for:
> 
>     public static int hashCode(byte[] value) {
>         int h = 0;
>         for (byte v : value) {
> 
> So it feels like quite a bit of extra complexity and it could backfire
> if the pattern matching is not robust enough.
> 
> Roland.