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

[Volker Simonis]

Hi Dmitry,

sorry, but I don’t understand how the regression test works. Can you please
explain what’s the expected result of the test without and with your fix?

Thanks,
Volker

Dmitry Cherepanov <dcherepanov at azul.com> schrieb am Mi. 13. März 2019 um
13:44:

> 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
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20190316/eb9398ff/attachment.html>