RFR(M): 8220376: C2: Int >0 not recognized as !=0 for div by 0 check

[Patric Hedlin]

Thanks for reviewing Vladimir.


On 18/11/2019 23:56, Vladimir Ivanov wrote:
>
>> Webrev: http://cr.openjdk.java.net/~phedlin/tr8220376/
>
> Looks good, Patric.
>
> Best regards,
> Vladimir Ivanov
>
> PS: some cleanup suggestions (feel free to ignore them if you don't 
> agree):
>
> src/hotspot/share/opto/ifnode.cpp:
>
> +//   \r1
> +//  r2\  eqT  eqF  neT  neF  ltT  ltF  leT  leF  gtT  gtF  geT geF
> +//  eq    t    f    f    t    f    -    -    f    f    -    - f
> +//  ne    f    t    t    f    t    -    -    t    t    -    - t
> +//  lt    f    -    -    f    t    f    -    f    f    -    f t
> +//  le    t    -    -    t    t    -    t    f    f    t    - t
> +//  gt    f    -    -    f    f    -    f    t    t    f    - f
> +//  ge    t    -    -    t    f    t    -    t    t    -    t f
> +//
> +Node* IfNode::simple_subsuming(PhaseIterGVN* igvn) {
> +  // Table encoding: N/A (na), True-branch (tb), False-branch (fb).
> +  static enum { na, tb, fb } s_subsume_map[6][12] = {
> +  /*rel: eq+T eq+F ne+T ne+F lt+T lt+F le+T le+F gt+T gt+F ge+T ge+F*/
> +  /*eq*/{ tb,  fb,  fb,  tb,  fb,  na,  na,  fb,  fb,  na,  na, fb },
> +  /*ne*/{ fb,  tb,  tb,  fb,  tb,  na,  na,  tb,  tb,  na,  na, tb },
> +  /*lt*/{ fb,  na,  na,  fb,  tb,  fb,  na,  fb,  fb,  na,  fb, tb },
> +  /*le*/{ tb,  na,  na,  tb,  tb,  na,  tb,  fb,  fb,  tb,  na, tb },
> +  /*gt*/{ fb,  na,  na,  fb,  fb,  na,  fb,  tb,  tb,  fb,  na, fb },
> +  /*ge*/{ tb,  na,  na,  tb,  fb,  tb,  na,  tb,  tb,  na,  tb, fb }};
>
> IMO you can dump the table from the comment: it mostly duplicates the 
> code. (Probably, you can use a different name for "N/A" or just refer 
> to it in numeric form (0?) to preserve clean structure of the table 
> from the comment.)
>
> ======================================
>
> +  if (is_If() && (cmp = in(1)->in(1))->Opcode() == Op_CmpP) {
> +    if (cmp->in(2) != NULL && // make sure cmp is not already dead
> +        cmp->in(2)->bottom_type() == TypePtr::NULL_PTR) {
>
> Merge nested ifs?
>
I to have problems with this part but for other reasons. What about: (?)

-  Node* cmp;
    int dist = 4;               // Cutoff limit for search
-  if (is_If() && (cmp = in(1)->in(1))->Opcode() == Op_CmpP) {
-    if (cmp->in(2) != NULL && // make sure cmp is not already dead
+  if (is_If() && in(1)->is_Bool()) {
+    Node* cmp = in(1)->in(1);
+    if (cmp->Opcode() == Op_CmpP &&
+        cmp->in(2) != NULL && // make sure cmp is not already dead

Best regards,
Patric
> ======================================
>
> Looks like extracting the following code into a helper function (along 
> with the enum and the table) can improve readability.
>
> +  int drel = subsuming_bool_test_encode(dom->in(1));
> +  int trel = subsuming_bool_test_encode(bol);
> +  int bout = pre->is_IfFalse() ? 1 : 0;
> +
> +  if (drel < 0 || trel < 0) {
> +    return NULL;
> +  }
> +  int br = s_subsume_map[trel][2*drel+bout];
> +  if (br == na) {
> +    return NULL;
> +  }
>
> New function can return intcon(0/1) or bol(or NULL?) and the caller 
> decides whether the update is needed.
>
>> On 12/11/2019 15:16, Patric Hedlin wrote:
>>> Dear all,
>>>
>>> I would like to ask for help to review the following change/update:
>>>
>>> Issue:  https://bugs.openjdk.java.net/browse/JDK-8220376
>>> Webrev: http://cr.openjdk.java.net/~phedlin/tr8220376/
>>>
>>> 8220376: C2: Int >0 not recognized as !=0 for div by 0 check
>>>
>>>     Adding a simple subsumption test to IfNode::Ideal to enable a local
>>>     short-circuit for (obviously) redundant if-nodes.
>>>
>>> Testing: hs-tier1-4, hs-precheckin-comp
>>>
>>>
>>> Best regards,
>>> Patric