[Mesa-dev] [PATCH] radeonsi: enable denorms for 64-bit and 16-bit floats

[Tom Stellard]

On Mon, Feb 08, 2016 at 09:38:32PM +0100, Marek Olšák wrote:
> On Mon, Feb 8, 2016 at 5:08 PM, Tom Stellard <tom at stellard.net> wrote:
> > On Sat, Feb 06, 2016 at 01:15:42PM +0100, Marek Olšák wrote:
> >> From: Marek Olšák <marek.olsak at amd.com>
> >>
> >> This fixes FP16 conversion instructions for VI, which has 16-bit floats,
> >> but not SI & CI, which can't disable denorms for those instructions.
> >
> > Do you know why this fixes FP16 conversions?  What does the OpenGL
> > spec say about denormal handing?
> 
> Yes, I know why. The patch explain everything as far as I can see
> though. What isn't clear?
> 

I got it now.

> SI & CI: Don't support FP16. FP16 conversions are hardcoded to emit
> and accept FP16 denormals.
> VI: Supports FP16. FP16 denormal support is now configurable and
> affects FP16 conversions as well.(shared setting with FP64).
> 
> OpenGL doesn't require denormals. Piglit does. I think this is
> incorrect piglit behavior.
> 
> >
> >> ---
> >>  src/gallium/drivers/radeonsi/si_shader.c        | 14 ++++++++++++++
> >>  src/gallium/drivers/radeonsi/si_state_shaders.c | 18 ++++++++++++------
> >>  src/gallium/drivers/radeonsi/sid.h              |  3 +++
> >>  3 files changed, 29 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/src/gallium/drivers/radeonsi/si_shader.c b/src/gallium/drivers/radeonsi/si_shader.c
> >> index a4680ce..3f1db70 100644
> >> --- a/src/gallium/drivers/radeonsi/si_shader.c
> >> +++ b/src/gallium/drivers/radeonsi/si_shader.c
> >> @@ -4155,6 +4155,20 @@ int si_compile_llvm(struct si_screen *sscreen,
> >>
> >>       si_shader_binary_read_config(binary, conf, 0);
> >>
> >> +     /* Enable 64-bit and 16-bit denormals, because there is no performance
> >> +      * cost.
> >> +      *
> >> +      * If denormals are enabled, all floating-point output modifiers are
> >> +      * ignored.
> >> +      *
> >> +      * Don't enable denormals for 32-bit floats, because:
> >> +      * - Floating-point output modifiers would be ignored by the hw.
> >> +      * - Some opcodes don't support denormals, such as v_mad_f32. We would
> >> +      *   have to stop using those.
> >> +      * - SI & CI would be very slow.
> >> +      */
> >> +     conf->float_mode |= V_00B028_FP_64_DENORMS;
> >> +
> >
> > Do SI/CI support fp64 denorms?  If so, won't this hurt performance?
> 
> Yes, they do. Fp64 denorms don't hurt performance. Only fp32 denorms
> do on SI & CI.
> 
> >
> > We should tell the compiler we are enabling fp-64 denorms by adding
> > +fp64-denormals to the feature string.  It would also be better to
> > read the float_mode value from the config registers emitted by the
> > compiler.
> 
> Yes, I agree, but LLVM only sets these parameters for compute or even
> HSA-only kernels, not for graphics shaders. We need to set the mode
> for all users _now_, not in 6 months. Last time I looked,
> +fp64-denormals had no effect on graphics shaders.
> 

We should still add +fp64-denormals even if the backend doesn't do
anything with it now.  This will make it easier if we have to use this
feature string to enable fix a bug in the backend,
because we will just be able to update LLVM.

I don't have a problem hard-coding float_mode for now, but once LLVM is
emitting the correct thing, we should pull the value from LLVM.

-Tom

> Marek