[PING] RE: RFR(S): 8210152: Optimize integer divisible by power-of-2 check

[Vladimir Kozlov]

I finally have time to look on it and I agree with your changes.

The only comment I have is to add check for SubI on other branch (not only on True branch). Negation 
may occur on either branch since you accept all conditions for negation.

Thanks,
Vladimir

On 9/9/18 9:24 PM, Pengfei Li (Arm Technology China) wrote:
> Hi Dean / Vladimir / JDK experts,
> 
> Do you have any further questions or comments on this patch? Or should I make some modifications on it, such as adding some limitations to the matching condition?
> I appreciate your help.
> 
> --
> Thanks,
> Pengfei
> 
> 
>> -----Original Message-----
>> From: Pengfei Li (Arm Technology China)
>> Sent: Monday, September 3, 2018 13:50
>> To: 'dean.long at oracle.com' <dean.long at oracle.com>; 'Vladimir Kozlov'
>> <vladimir.kozlov at oracle.com>; hotspot-compiler-dev at openjdk.java.net;
>> hotspot-dev at openjdk.java.net
>> Cc: nd <nd at arm.com>
>> Subject: RE: RFR(S): 8210152: Optimize integer divisible by power-of-2 check
>>
>> Hi Vladimir, Dean,
>>
>> Thanks for your review.
>>
>>> I don't see where negation is coming from for 'X % 2 == 0' expression.
>>> It should be only 2 instructions: 'cmp (X and 1), 0'
>> The 'cmp (X and 1), 0' is just what we expected. But there's redundant
>> conditional negation coming from the possibly negative X handling in "X % 2".
>> For instance, X = -5, "X % 2" should be -1. So only "(X and 1)" operation is not
>> enough. We have to negate the result.
>>
>>> I will look on it next week. But it would be nice if you can provide small test
>> to show this issue.
>> I've already provided a case of "if (a%2 == 0) { ... }" in JBS description. What
>> code generated and what can be optimized are listed there.
>> You could see https://bugs.openjdk.java.net/browse/JDK-8210152 for details.
>> You could also see the test case for this optimization I attached below.
>>
>>> It looks like your matching may allow more patterns than expected. I was
>> expecting it to look for < 0 or >= 0 for the conditional negation, but I don't see
>> it.
>> Yes. I didn't limit the if condition to <0 or >= 0 so it will match more patterns.
>> But nothing is going wrong if this ideal transformation applies on more cases.
>> In pseudo code, if someone writes:
>> if ( some_condition ) { x = -x; }
>> if ( x == 0 ) { do_something(); }
>> The negation in 1st if-clause could always be eliminated whatever the
>> condition is.
>>
>> --
>> Thanks,
>> Pengfei
>>
>>
>> -- my test case attached below --
>> public class Foo {
>>
>>      public static void main(String[] args) {
>>          int[] dividends = { 0, 17, 1553, -90, -35789, 0x80000000 };
>>          for (int i = 0; i < dividends.length; i++) {
>>              int x = dividends[i];
>>              System.out.println(testDivisible(x));
>>              System.out.println(testModulo(x));
>>              testCondNeg(x);
>>          }
>>          return;
>>      }
>>
>>      public static int testDivisible(int x) {
>>          // Modulo result is only for zero check
>>          if (x % 4 == 0) {
>>              return 444;
>>          }
>>          return 555;
>>      }
>>
>>      public static int testModulo(int x) {
>>          int y = x % 4;
>>          if (y == 0) {
>>              return 222;
>>          }
>>          // Modulo result is used elsewhere
>>          System.out.println(y);
>>          return 333;
>>      }
>>
>>      public static void testCondNeg(int x) {
>>          // Pure conditional negation
>>          if (printAndIfNeg(x)) {
>>              x = -x;
>>          }
>>          if (x == 0) {
>>              System.out.println("zero!");
>>          }
>>      }
>>
>>      static boolean printAndIfNeg(int x) {
>>          System.out.println(x);
>>          return x <= 0;
>>      }
>> }