RFR: 8211100: hotspot C1 issue with comparing long numbers on x86 32-bit

[Dmitry Cherepanov]

Igor,

Updated version of original fix (with ifdef X86 added):
http://cr.openjdk.java.net/~dcherepanov/8211100/webrev.04/

Is it okay to push it?

Thanks,

Dmitry

> On Mar 13, 2019, at 5:24 AM, Igor Veresov <igor.veresov at oracle.com> wrote:
> 
> Dmitry,
> 
> After some digging around I think your original fix is ok. In addition to !_LP64 can you add ifdef X86?
> 
> igor
> 
> 
> 
>> On Mar 6, 2019, at 3:07 AM, Dmitry Cherepanov <dcherepanov at azul.com> wrote:
>> 
>> Igor,
>> 
>> Sorry for the delay in responding.
>> 
>> I updated comp_op (in c1_LIRAssembler_x86.cpp) to make use of tmp1 for this case. The changes: http://cr.openjdk.java.net/~dcherepanov/8211100/webrev.03/
>> 
>> For this change, I got assertion failed (from cpu_regnrLo, in c1_LIR.hpp). Sorry if this is an obvious question - Am I correctly understand that another part of this solution should be an additional change that would allocate tmp1? Or is there an existing code that should take care of it already and just need to enable the allocation of tmp1 for this case?
>> 
>> Another question: given that this is a major issue on x86 32bit system, would you mind if we proceed with the current minimal/low-risk fix (http://cr.openjdk.java.net/~dcherepanov/8211100/webrev.01/) and create new JBS issue to investigate more generic approach separately?
>> 
>> Thanks,
>> 
>> Dmitry
>> 
>>> On Oct 2, 2018, at 8:09 PM, Igor Veresov <igor.veresov at oracle.com> wrote:
>>> 
>>> Right, I forgot how it works. Sorry for the confusion. I think there is no way to explicitly describe a register kill in C1. I guess the only option is to just avoid clobbering opr1. So may be we should make use of tmp1 for lir_cmp to save/restore opr1? Again, tmp1 would have to be allocated only for this particular case.
>>> 
>>> igor
>>> 
>>> 
>>> 
>>>> On Oct 1, 2018, at 7:15 AM, Dmitry Cherepanov <dcherepanov at azul.com> wrote:
>>>> 
>>>> Hi Igor,
>>>> 
>>>> Thanks for the suggestions. I tried to make the opr1 a temporary
>>>> 
>>>> http://cr.openjdk.java.net/~dcherepanov/8211100/webrev.02/
>>>> 
>>>> but the generated code still has the problem. Looking into the log with -XX:TraceLinearScanLevel=4 (http://cr.openjdk.java.net/~dcherepanov/8211100/TraceLinearScanLevel.02.log) seems like the reason for this is that the opr1 (virtual register R165 in the log) is also an input operand and its range becomes wider and the shorter ranges (corresponding to the opr1 marked as temp) are merged to the single range. Can the input operand be temporary at the same time?
>>>> 
>>>> Dmitry
>>>> 
>>>>> On Sep 27, 2018, at 2:18 AM, Igor Veresov <igor.veresov at oracle.com> wrote:
>>>>> 
>>>>> Edit: It may be more consistent to check for is_double_cpu() instead of T_LONG. Although that’s semantically equivalent.
>>>>> 
>>>>>> On Sep 26, 2018, at 9:35 AM, Igor Veresov <igor.veresov at oracle.com> wrote:
>>>>>> 
>>>>>> It doesn’t seem to me like the proper way to fix it. The problem is that the cmp is destroying opr1 without telling the register allocator about it.
>>>>>> 
>>>>>> One possible solution would be to make opr1 also a temp (see LIR_OpVisitState::visit(LIR_Op* op) in c1_LIR.cpp), only for x86 32bit and only if the operand type is T_LONG. 
>>>>>> Another solution is to maintain a temporary register for lir_cmp and use it to save/restore opr1 when emitting the code in LIR_Assembler::comp_op(). Again, the temporary register has to be there only for x86 32bit and T_LONG.
>>>>>> 
>>>>>> igor
>>>>>> 
>>>>>> 
>>>>>>> On Sep 26, 2018, at 1:29 AM, Tobias Hartmann <tobias.hartmann at oracle.com> wrote:
>>>>>>> 
>>>>>>> Hi Dmitry,
>>>>>>> 
>>>>>>> this looks good to me but Igor (who implemented 8201447) should have a look as well.
>>>>>>> 
>>>>>>> Best regards,
>>>>>>> Tobias
>>>>>>> 
>>>>>>> On 26.09.2018 09:04, Dmitry Cherepanov wrote:
>>>>>>>> Hi Tobias,
>>>>>>>> 
>>>>>>>> Thanks for the review, updated patch avoids the additional move on x86_64 and includes the
>>>>>>>> regression test.
>>>>>>>> 
>>>>>>>> http://cr.openjdk.java.net/~dcherepanov/8211100/webrev.01/
>>>>>>>> <http://cr.openjdk.java.net/%7Edcherepanov/8211100/webrev.01/>
>>>>>>>> 
>>>>>>>> Dmitry
>>>>>>>> 
>>>>>>>>> On Sep 25, 2018, at 6:40 PM, Tobias Hartmann <tobias.hartmann at oracle.com
>>>>>>>>> <mailto:tobias.hartmann at oracle.com>> wrote:
>>>>>>>>> 
>>>>>>>>> Hi Dmitry,
>>>>>>>>> 
>>>>>>>>> Shouldn't this at least be guarded by an #ifndef _LP64 to avoid the additional move on x86_64?
>>>>>>>>> 
>>>>>>>>> Could you please add the regression test to the webrev? Or did this reproduce with other tests?
>>>>>>>>> 
>>>>>>>>> Thanks,
>>>>>>>>> Tobias
>>>>>>>>> 
>>>>>>>>> On 25.09.2018 16:00, Dmitry Cherepanov wrote:
>>>>>>>>>> Hello,
>>>>>>>>>> 
>>>>>>>>>> Please review a patch that resolves issue in x86 32bit builds. It slightly adjusts the fix for
>>>>>>>>>> JDK-8201447 (C1 does backedge profiling incorrectly) by creating a copy of the left operand and
>>>>>>>>>> using it for incrementing backedge counter.
>>>>>>>>>> 
>>>>>>>>>> JBS issue: https://bugs.openjdk.java.net/browse/JDK-8211100
>>>>>>>>>> webrev: http://cr.openjdk.java.net/~dcherepanov/8211100/webrev.00/
>>>>>>>>>> <http://cr.openjdk.java.net/%7Edcherepanov/8211100/webrev.00/>
>>>>>>>>>> 
>>>>>>>>>> Thanks,
>>>>>>>>>> 
>>>>>>>>>> Dmitry
>>>>>> 
>>>>> 
>>>> 
>>> 
>> 
>