[16] RFR(S): 8248552: C2 crashes with SIGFPE due to division by zero

[Tobias Hartmann]

+1

Best regards,
Tobias

On 24.07.20 23:41, Vladimir Kozlov wrote:
> Good.
> 
> Thanks,
> Vladimir K
> 
> On 7/24/20 4:44 AM, Christian Hagedorn wrote:
>> Hi Tobias
>>
>> Thank you for your review!
>>
>>> Please make sure to run performance testing.
>>
>> There is a repeated regression in the micros open crypto benchmark
>> openjdk.bench.javax.crypto.small.SecureRandomBench.nextBytes with these two settings:
>> - algorithm=SHA1PRNG-dataSize:64-provider:-shared:false
>> - algorithm=SHA1PRNG-dataSize:64-provider:-shared:true
>>
>> Repeated runs with these two settings resulted in a regression between 1 and 2%. I could trace it
>> back to the additional type filtering in PhiNode::Value() (webrev.02). This is only required for
>> the assertion code and not for the bailout fix itself. When running performance testing with
>> webrev.01, the regressions disappear.
>>
>> I therefore suggest to go with webrev.01 (without assertion code and type filtering) and file a
>> new RFE to investigate the usage of type filtering in PhiNode::Value() for iv phis and why we get
>> a performance regression in these two benchmark settings. In theory, I think it should be
>> beneficial to narrow the type range of iv phis.
>>
>>> cfgnode.cpp:1083
>>> - There's an extra whitespace before ","
>>>
>>> loopopts.cpp:84/86
>>> - No need for extra brackets
>>
>> These are not present anymore in webrev.01.
>>
>> http://cr.openjdk.java.net/~chagedorn/8248552/webrev.01/
>>
>> Best regards,
>> Christian
>>
>>
>> On 20.07.20 11:14, Tobias Hartmann wrote:
>>> Hi Christian,
>>>
>>> On 15.07.20 15:08, Christian Hagedorn wrote:
>>>> http://cr.openjdk.java.net/~chagedorn/8248552/webrev.02/
>>>
>>> Looks good to me.
>>>
>>> Some code style comments:
>>
>>
>>> Best regards,
>>> Tobias
>>>