Fixes for `fma` function #1580

parasyte · 2021-12-07T15:44:33Z

Using the mad intrinsic should be enough for HLSL because we only support f32 for now.
Adds support for fma function on GLSL, including trivial arithmetic transformation when necessary.
Adds a new test for WGSL functions, in the spirit of operators.wgsl. And one for WebGL output.
Closes [hlsl-out] fma function should use mad for single-precision floats #1579

- This should be enough because we only support f32 for now. - Adds a new test for WGSL functions, in the spirit of operators.wgsl. - Closes gfx-rs#1579

kvark · 2021-12-07T15:48:53Z

tests/in/functions.wgsl

@@ -0,0 +1,15 @@
+fn test_fma() -> vec2<f32> {


we should probably move a few things from operators.wgsl into here

kvark · 2021-12-07T15:50:10Z

You've got a fun SPIR-V error:

ERROR: 0:13: 'fma' : required extension not requested: Possible extensions include:
GL_EXT_gpu_shader5
GL_OES_gpu_shader5

So, the first approximation should probably just generate the multiply-add instructions as is.
Then we can add the check for one of those extensions and use the specific opcode.

parasyte · 2021-12-07T15:53:49Z

I was just looking into that, hoping it was going to be adding a shader model 5 extension to glslangValidator, but that doesn't appear to be the case.

- I think this is right. Just iterate all known expressions in all functions and entry points to locate any `fma` function call. Should not need to walk the statement DAG.

parasyte · 2021-12-07T17:42:00Z

I added the extension for GLES outputs when the fma function exists in the module. That fixes the test! I'm happy with it if you are.

parasyte · 2021-12-07T18:27:17Z

Hmm! That still isn't enough for WebGL 2. Old error, prior to this patch:

panicked at 'wgpu error: Validation Error

Caused by:
    In Device::create_render_pipeline
      note: label = `pixels_scaling_renderer_pipeline`
    Internal error in VERTEX shader: ERROR: 0:21: 'fma' : no matching overloaded function found
ERROR: 0:21: '=' : dimension mismatch
ERROR: 0:21: 'assign' : cannot convert from 'const mediump float' to 'highp 2-component vector of float'

New error:

panicked at 'wgpu error: Validation Error

Caused by:
    In Device::create_render_pipeline
      note: label = `pixels_scaling_renderer_pipeline`
    Internal error in VERTEX shader: The selected version doesn't support FMA

It may be better to output the separate multiply and add ops, but I don't know where to start with that.

kvark

Sorry, looks like we have one more thing to do here

src/back/glsl/features.rs

kvark · 2021-12-16T23:41:50Z

@parasyte is this still WIP?

parasyte · 2021-12-17T01:29:56Z

It is. I haven't attempted to do the transformation for GLSL.

parasyte · 2021-12-18T23:54:06Z

@kvark Ok, I think I got this one working. The GLSL writer can do the transformation and it works on WebGL 2.0.

kvark

Thank you!
Got a few notes here

src/back/glsl/features.rs

kvark · 2021-12-20T18:18:46Z

src/back/glsl/features.rs

@@ -33,6 +33,8 @@ bitflags::bitflags! {
        /// Arrays with a dynamic length
        const DYNAMIC_ARRAY_SIZE = 1 << 16;
        const MULTI_VIEW = 1 << 17;
+        /// Adds support for fused multiply-add
+        const FMA = 1 << 18;


Why do we need thus flag? If FMA isn't natively supported, we are emulating it anyway. So it seems to me that this flag isn't getting us anything.

For one thing, I'm following precedent with existing features that need extensions, and fma is only supported on GLES 3.1+ with:

naga/tests/out/glsl/functions.main.Compute.glsl

Lines 1 to 2 in 7454d1f

#version 310 es

#extension GL_EXT_gpu_shader5 : require

This PR does not emulate fma on GLSL in every case, but decides if it must emulate it or else it requests the extension when necessary. This fixes that unusual validation error you noted earlier: https://github.com/gfx-rs/naga/runs/4446174291?check_suite_focus=true

ERROR: 0:13: 'fma' : required extension not requested: Possible extensions include: GL_EXT_gpu_shader5 GL_OES_gpu_shader5

I chose to use the existing feature flag infrastructure to write this extension, rather than coming up with something unique just for this case. Is there something better I could have done here?

The FMA feature flag name is probably too narrow, honestly. GL_EXT_gpu_shader5 enables a lot more than just the fma function, and the feature flag can be used to support all of it on GLES.

The capability flags in GLSL backend are meant to be requirements. I.e. shader requires A, B, C, and we want to check if we can work with this shader at all.
The case for FMA is different. The backend always supports FMA instruction. The only thing different is a code path taken. Therefore, there is no case where GLSL backend would check for this capability and report it missing. It's not a real capability.

I think I understand what you mean. Let me try to rephrase it; The fma function from the frontend is always supported by the backend (GLSL) even if it has to fallback to an arithmetic transformation (i.e., "emulated"). This PR uses a feature flag (capability) in another sense, that it can enable the use of the GLSL fma function on particular versions of the backend. Which are not how feature flags are used elsewhere.

Would that be an accurate way to describe the situation?

I'll have to think on it if I need to use some other mechanism to enable the extension for GLES. I do see an extension enabled that is not controlled by feature flags:

naga/src/back/glsl/mod.rs

Lines 442 to 450 in 7c8bedc

// Write the additional extensions

if self

.options

.writer_flags

.contains(WriterFlags::TEXTURE_SHADOW_LOD)

{

// https://www.khronos.org/registry/OpenGL/extensions/EXT/EXT_texture_shadow_lod.txt

writeln!(self.out, "#extension GL_EXT_texture_shadow_lod : require")?;

}

I suppose that would be an appropriate place to put this?

src/back/glsl/mod.rs

* [hlsl-out] Write `mad` intrinsic for `fma` function - This should be enough because we only support f32 for now. - Adds a new test for WGSL functions, in the spirit of operators.wgsl. - Closes gfx-rs#1579 * Add FMA feature to glsl backend - I think this is right. Just iterate all known expressions in all functions and entry points to locate any `fma` function call. Should not need to walk the statement DAG. * Transform GLSL fma function into an airthmetic expression when necessary * Add tests for GLSL fma function tranformation * Remove the hazard comment from the webgl test input * Add helper method for fma function support checks * Address review comment

kvark · 2021-12-29T15:04:55Z

published in 0.8.1

* [hlsl-out] Write `mad` intrinsic for `fma` function - This should be enough because we only support f32 for now. - Adds a new test for WGSL functions, in the spirit of operators.wgsl. - Closes #1579 * Add FMA feature to glsl backend - I think this is right. Just iterate all known expressions in all functions and entry points to locate any `fma` function call. Should not need to walk the statement DAG. * Transform GLSL fma function into an airthmetic expression when necessary * Add tests for GLSL fma function tranformation * Remove the hazard comment from the webgl test input * Add helper method for fma function support checks * Address review comment

[hlsl-out] Write mad intrinsic for fma function

beddc71

- This should be enough because we only support f32 for now. - Adds a new test for WGSL functions, in the spirit of operators.wgsl. - Closes gfx-rs#1579

kvark approved these changes Dec 7, 2021

View reviewed changes

Add FMA feature to glsl backend

5cec686

- I think this is right. Just iterate all known expressions in all functions and entry points to locate any `fma` function call. Should not need to walk the statement DAG.

parasyte force-pushed the fix/hlsl-fma-f32 branch from f67e0bf to 5cec686 Compare December 7, 2021 17:38

kvark reviewed Dec 7, 2021

View reviewed changes

src/back/glsl/features.rs Outdated Show resolved Hide resolved

parasyte mentioned this pull request Dec 7, 2021

Revert Fused-Multiply-Add parasyte/pixels#241

Merged

parasyte marked this pull request as draft December 7, 2021 18:37

kvark mentioned this pull request Dec 17, 2021

Release v0.8 #1610

Merged

Transform GLSL fma function into an airthmetic expression when necessary

df97573

parasyte marked this pull request as ready for review December 18, 2021 23:34

parasyte added 2 commits December 18, 2021 15:50

Add tests for GLSL fma function tranformation

f4e2416

Remove the hazard comment from the webgl test input

7454d1f

parasyte changed the title ~~[hlsl-out] Write mad intrinsic for fma function~~ Fixes for fma function Dec 18, 2021

kvark suggested changes Dec 20, 2021

View reviewed changes

parasyte added 2 commits December 20, 2021 13:49

Add helper method for fma function support checks

4f9addb

Address review comment

7c8bedc

kvark approved these changes Dec 22, 2021

View reviewed changes

kvark merged commit 924ab17 into gfx-rs:master Dec 22, 2021

parasyte deleted the fix/hlsl-fma-f32 branch December 22, 2021 23:48

kvark added the can backport PR that can be back-ported to a release branch label Dec 29, 2021

kvark mentioned this pull request Dec 29, 2021

Release v0.8.1 #1635

Merged

kvark removed the can backport PR that can be back-ported to a release branch label Dec 29, 2021

teoxoy mentioned this pull request Jan 6, 2023

[glsl-out] use fma polyfill for versions below gles 320 #2197

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for `fma` function #1580

Fixes for `fma` function #1580

parasyte commented Dec 7, 2021 •

edited

Loading

kvark Dec 7, 2021

kvark commented Dec 7, 2021

parasyte commented Dec 7, 2021

parasyte commented Dec 7, 2021

parasyte commented Dec 7, 2021

kvark left a comment

kvark commented Dec 16, 2021

parasyte commented Dec 17, 2021

parasyte commented Dec 18, 2021

kvark left a comment

kvark Dec 20, 2021

parasyte Dec 20, 2021

kvark Dec 21, 2021

parasyte Dec 21, 2021

kvark commented Dec 29, 2021

	// Write the additional extensions
	if self
	.options
	.writer_flags
	.contains(WriterFlags::TEXTURE_SHADOW_LOD)
	{
	// https://www.khronos.org/registry/OpenGL/extensions/EXT/EXT_texture_shadow_lod.txt
	writeln!(self.out, "#extension GL_EXT_texture_shadow_lod : require")?;
	}

Fixes for fma function #1580

Fixes for fma function #1580

Conversation

parasyte commented Dec 7, 2021 • edited Loading

kvark Dec 7, 2021

Choose a reason for hiding this comment

kvark commented Dec 7, 2021

parasyte commented Dec 7, 2021

parasyte commented Dec 7, 2021

parasyte commented Dec 7, 2021

kvark left a comment

Choose a reason for hiding this comment

kvark commented Dec 16, 2021

parasyte commented Dec 17, 2021

parasyte commented Dec 18, 2021

kvark left a comment

Choose a reason for hiding this comment

kvark Dec 20, 2021

Choose a reason for hiding this comment

parasyte Dec 20, 2021

Choose a reason for hiding this comment

kvark Dec 21, 2021

Choose a reason for hiding this comment

parasyte Dec 21, 2021

Choose a reason for hiding this comment

kvark commented Dec 29, 2021

Fixes for `fma` function #1580

Fixes for `fma` function #1580

parasyte commented Dec 7, 2021 •

edited

Loading