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 <t...@stellard.net> wrote: > > On Sat, Feb 06, 2016 at 01:15:42PM +0100, Marek Olšák wrote: > >> From: Marek Olšák <marek.ol...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
