[Mesa-dev] [PATCH] nir/spirv/glsl450: rewrite atan2 to deal with denorms / infinities

[Juan A. Suarez Romero]

On Tue, 2017-01-17 at 15:07 -0800, Francisco Jerez wrote:
> "Juan A. Suarez Romero" <jasuarez at igalia.com> writes:
> 
> > On Tue, 2017-01-17 at 11:34 +0100, Juan A. Suarez Romero wrote:
> > > > The above does not necessarily sum to "we shouldn't fix it" but it probably does mean it's low-priority at best and we need to be careful.
> > > > 
> > > > Looking a bit into the math of atan2, I'm not convinced flushing denorms to zero is actually what we want to do.  For x, I think it's valid, but as y approaches 0, you get +/- pi/2 depending on whether y positive or negative.  As both approach zero, it's undefined.  As you approach infinity, it approaches something in the interval [-pi/2, pi/2] but it depends on the direction of approach.  If you do "man atan2" you'll see this all layed out in a horrifying amount of detail (somewhere around a dozen different cases).
> > > > 
> > > > 
> > > 
> > > Yes, originally I only flushed 'x'. But then also flushed 'y' to keep coherence. But didn't think about that corner case you mention. I think we can remove the flush to 0 in 'y'.
> > 
> > Actually, after doing  a couple of tests more, I don't think we need to flush 'x' any more.
> > 
> > The thing is that originally I added the flush to 0 to solve a couple of cases from Vulkan CTS atan2 tests. But later I modified the code to cope with infinities, which lead a refactoring of code. I think this refactoring made the flush to 0 not required any more.
> > 
> > I've run the tests again getting rid of both flushes, and seems results are exactly the same as with the flush. So maybe they are not required after all.
> > 
> 
> So what exactly from the many checks you added makes the tests pass?

Right now, the changes done to handle +/-INF as input. These changes
makes the tests that use +/-INF, and also denorms, to pass.

So we don't need the denorm_{x,y} changes.

> IOW, what property do the vulkan CTS tests take for granted that the
> current atan2 implementation doesn't fulfill?
> 

Vulkan CTS just computes in runtime the value of atan2() (in the CPU)
and then checks it with the returned value from our implementation. And
this is done with random values, plus corner cases: infinities,
denorms, 0, extreme values, etc.

So it is assuming that our implementation deals well with these corner
cases.

But it also assumes denorms are not flushed to 0, because when it runs
atan2() with those values in the CPU to compute the expected values, it
is not flushing to 0.

So the changes done make the tests dealing with denorms or infinities
to pass. But as I said, we can remove denorm_{x,y}, and tests dealing
with denorms or infinities still pass.

denorm_{x,y} is only required if we just revert to the original code
and don't deal with infinities. In this case, we need denorm_{x,y}
because otherwise using denorms as inputs returns a wrong result.


	J.A.