Re: [PULL 1/1] target/openrisc: Set EPCR to next PC on FPE exceptions

From: Stafford Horne <shorne@gmail.com>
To: Michael Tokarev <mjt@tls.msk.ru>
Cc: QEMU Development <qemu-devel@nongnu.org>,
	Richard Henderson <richard.henderson@linaro.org>
Subject: Re: [PULL 1/1] target/openrisc: Set EPCR to next PC on FPE exceptions
Date: Thu, 10 Aug 2023 20:50:29 +0100	[thread overview]
Message-ID: <ZNU/hbdfbAOzp95u@antec> (raw)
In-Reply-To: <a5dc02df-158e-4ff8-fce9-e2ec7df980ad@tls.msk.ru>

On Thu, Aug 10, 2023 at 09:35:18AM +0300, Michael Tokarev wrote:
> 09.08.2023 23:34, Stafford Horne пишет:
> > The architecture specification calls for the EPCR to be set to "Address
> > of next not executed instruction" when there is a floating point
> > exception (FPE).  This was not being done, so fix it by using the same
> > pattern as syscall.  Also, we move this logic down to be done for
> > instructions not in the delay slot as called for by the architecture
> > manual.
> > 
> > Without this patch FPU exceptions will loop, as the exception handling
> > will always return back to the failed floating point instruction.
> > 
> > This was not noticed in earlier testing because:
> > 
> >   1. The compiler usually generates code which clobbers the input operand
> >      such as:
> > 
> >        lf.div.s r19,r17,r19
> > 
> >   2. The target will store the operation output before to the register
> >      before handling the exception.  So an operation such as:
> > 
> >        float a = 100.0f;
> >        float b = 0.0f;
> >        float c = a / b;    /* lf.div.s r19,r17,r19 */
> > 
> >      Will first execute:
> > 
> >        100 / 0    -> Store inf to c (r19)
> >                   -> triggering divide by zero exception
> >                   -> handle and return
> > 
> >      Then it will execute:
> > 
> >        100 / inf  -> Store 0 to c  (no exception)
> > 
> > To confirm the looping behavior and the fix I used the following:
> > 
> >      float fpu_div(float a, float b) {
> > 	float c;
> > 	asm volatile("lf.div.s %0, %1, %2"
> > 		      : "+r" (c)
> > 		      : "r" (a), "r" (b));
> > 	return c;
> >      }
> 
> Is it a -stable material?  It applies cleanly to 8.0 and 7.2.
> Or maybe it is not needed on older versions, not being noticed before?

I would say no, it will work on 8.0 an 7.2 but this code path is not very useful
withouth the other 8.1 Floating Point Exception handling updates.

-Stafford