RFR: 8224828: aarch64: save and restore the rflags register in push_CPU_state and pop_CPU_state
Yangfei (Felix)
felix.yang at huawei.com
Wed May 29 10:10:41 UTC 2019
> On 28/05/2019 14:50, Andrew Haley wrote:
> > Great catch! I'm somewhat appalled that flags are live across
> > safepoint polls. I know that they are not live on x86.
>
> I was thinking exactly the same thing.
Yes, I see the x86_64 SafePoint instruction has the effect of killing cr.
> I am not really clear how the cmp and cset instructions got separated in
> the instruction scheduling. It would be good to understand how this
> arose and see if it has any implications for other rules, encodings or
> even masm method definitions.
We found this bug will not trigger when C2 instruction scheduling is turned off.
As the aarch64 SafePoint instruction does not have the effect of killing cr, the data dependence between cmp/cset and the SafePoint instruction is not there.
So I think C2 instruction scheduling has the reason of moving instruction like that.
> Felix, were you able to identify what happened during graph lowering
> that caused this interleaving of the cmp, safepoint and cset
> instructions? Are you able to provide any details of the C2 graph that
> produced this code?
Currently, we only got the JIT code for the erroneous Java method.
As we were treating this as an instruction scheduling issue, we didn't analyze the C2 graph.
We found the erroneous Java method is C2 compiled for three times, and the bad JIT code was generated by the first C2 compile.
Also this happens randomly. I haven't figured out how to look into the C2 scheduling phase during a hadoop terasort run :-(
> > I don't think you should do this. Instead, please apply
> >
> > effect(KILL cr);
> >
> > to instruct safePoint in aarch64.ad.
> Yes, that is a far better fix.
I find x86 handles all registers including rflags in push_CPU_state/pop_CPU_state.
Was rflags intentionally neglected in push_CPU_state/ pop_CPU_state for the aarch64 port?
If true, then I would suggest renaming push_CPU_state/ pop_CPU_state to some other name.
Thanks,
Felix
More information about the hotspot-runtime-dev
mailing list