Add support for ARB_gpu_shader_fp64 #1998

ShchchowAMD · 2019-11-26T15:38:51Z

GLSL Version : >= 150

Purpose:
Allow users to use features by enabling this extension, even in low versions.

Extension Name:
ARB_gpu_shader_fp64

Builtin-variables:
Nah

Builtin-functions:
functions overloaded for this extension, please check registry in reference.

Keywords:
Double

Features:
add support for type "double", also related functions/builtin functions
overloaded for DOUBLE. "double vector" input should be in
ARB_vertex_attrib_64bit

Reference:
https://www.khronos.org/registry/OpenGL/extensions/ARB/ARB_gpu_shader_fp64.txt

johnkslang · 2019-12-03T06:40:03Z

glslang/MachineIndependent/glslang.m4

@@ -1751,7 +1751,7 @@ type_specifier_nonarray
    }
 GLSLANG_WEB_EXCLUDE_ON
    | DOUBLE {
-        parseContext.doubleCheck($1.loc, "double");
+        parseContext.doubleCheck($1.loc, "double", parseContext.symbolTable.atBuiltInLevel());


Is adding an argument necessary? parseContext already has symbolTable, so it can check itself.

That is, it seems there is not much use now for the form that does not check atBuiltInLevel so, just leave all the calls the same, and only update the function. (Or, add a parseContext.doubleCheck() that checks the builtInLevel.)

Hi john,
The reason for this modification is that:

initialization (built-in functions and variables) happens before we update extensions status in state machine.

when I remove this builtin level check, glslang would report an error because it would treat "double" as an unavailable symbol in low versions during initialization when meets some overloaded functions with type "double".

if we add an builtin level check here:
3.1 initialization process would be okay as "doublecheck()" now knows they are overloaded functions.
3.2 users, still, can only use high core version or enabling extension if they need to use fp64.

For your suggestion, do you mean that I should add another doubleCheck() for builtinLevel symbols and keep calls same in .y file? Or maybe, try to do the builtinLevel check within doubleCheck()?

try to do the builtinLevel check within doubleCheck()?

Yes. Does that work?

Yes I tried before I committed in this version.

As if we put a breakpoint here, in doubleCheck(), we could know that: when a "double" symbol comes in, we have no extra information to know whether it is a built-in. We could also see that other check functions also use builtin flag to check types.

For built-in variables or functions, doubleCheck() is a part of initializing symbol table, we can't find that flag in context here as we have no context member variables. Otherwise, we still need to change doubleCheck's interface, or add context status, till here by other parameters.

I think for many types, like float16, their check functions already get this builtin flag with a default input parameter, may I know what suggestions for this part?

It seems like this adds this extra Boolean parameter to almost all (all?) calls to doubleCheck(), but with a query to the same object the method is in, so the method could more directly do the query itself.

FYI, this Boolean parameter has been removed. Built-in check has been moved when call this object. There's no difference in generated test results for latest commit.

I was trying to get one copy of the test instead of N copies.

johnkslang · 2019-12-03T06:44:16Z

glslang/MachineIndependent/Initialize.cpp


-        if (profile != EEsProfile) {
+    if (profile != EEsProfile && version >= 150) {  // ARB_gpu_shader_fp64
            commonBuiltins.append(
                "double fma(double, double, double);"


What are the impacts to overloaded function resolution under implicit type conversion? Version 400 has different rules than versions before 400.

I'll test for this and record here soon.

Thanks for reminding, implicit converting meets errors. I'll fix it.

@johnkslang Hi, John. I made some tests and find the difference is inside findFunction120 and findFunction400. Should "best match" rules be enabled in version 120-330 if I turned on type "double" with this commit?

Currently, I allow glslang to use findFunction400 when we turn on this extension, as type 'double' could be introduced with this extension.

Yes, that might be best, especially if formalized. Someone could change their shader behavior of unrelated code, just by enabling the extension, but if it's well specified that's how it works, it may be the best path.

GLSL Version : >= 150 Purpose: Allow users to use features by enabling this extension, even in low versions. Extension Name: ARB_gpu_shader_fp64 Builtin-variables: Nah Builtin-functions: functions overloaded for this extension, please check registry in reference. Keywords: Double Features: add support for type "double" Reference: https://www.khronos.org/registry/OpenGL/extensions/ARB/ARB_gpu_shader_fp64.txt Add support for implicit conversion 1. Remove builtin double vertex (this is introduced by vertex_attrib_64bit 2. Add extension check and implicit conversion as double has been introduced 3. Add test results.

ShchchowAMD · 2020-01-07T05:30:46Z

glslang/MachineIndependent/ParseHelper.cpp

@@ -6091,7 +6091,7 @@ const TFunction* TParseContext::findFunction(const TSourceLoc& loc, const TFunct
    if (isEsProfile() || version < 120)
        function = findFunctionExact(loc, call, builtIn);
    else if (version < 400)
-        function = findFunction120(loc, call, builtIn);
+        function = extensionTurnedOn(E_GL_ARB_gpu_shader_fp64) ? findFunction400(loc, call, builtIn) : findFunction120(loc, call, builtIn);


@johnkslang FYI, now when this extension turns on, we will use rules based on version 400.

I think the reason is we may still wanna remain the difference between old versions and versions larger than 400. And keep the change as small as possible (rules inside findFunction400 may be modified in the future).

When type "double" is introduced, a best match check would be enabled to allow extra implicit conversion, that's when those three rules work, to decide a best candidate.

When this extension is enabled, we have type "float", "int" and "double", same with version >= 400, which make no difference with what we do in latest version.

FYI, move builtin check to type symbol check. Avoid modifying interface doubleCheck().

glslang/MachineIndependent/glslang.m4

ShchchowAMD marked this pull request as ready for review November 26, 2019 16:49

johnkslang requested changes Dec 3, 2019

View reviewed changes

ShchchowAMD force-pushed the ARB_gpu_shader_fp64 branch from 155bed6 to a3c7a25 Compare January 6, 2020 03:09

ShchchowAMD commented Jan 7, 2020

View reviewed changes

Move symbol builtin check to grammar stage

ab6a588

FYI, move builtin check to type symbol check. Avoid modifying interface doubleCheck().

johnkslang reviewed Jan 8, 2020

View reviewed changes

glslang/MachineIndependent/glslang.m4 Show resolved Hide resolved

johnkslang merged commit 5181367 into KhronosGroup:master Jan 8, 2020

johnkslang mentioned this pull request May 4, 2020

Bogus implicit type conversion errors in 8.13.3743 #2206

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for ARB_gpu_shader_fp64 #1998

Add support for ARB_gpu_shader_fp64 #1998

ShchchowAMD commented Nov 26, 2019

johnkslang Dec 3, 2019

ShchchowAMD Dec 6, 2019

johnkslang Dec 24, 2019

ShchchowAMD Dec 25, 2019

johnkslang Jan 7, 2020

ShchchowAMD Jan 8, 2020

johnkslang Jan 8, 2020

johnkslang Dec 3, 2019

ShchchowAMD Dec 6, 2019

ShchchowAMD Dec 25, 2019

ShchchowAMD Dec 25, 2019

ShchchowAMD Jan 6, 2020

johnkslang Jan 7, 2020

ShchchowAMD Jan 7, 2020

Add support for ARB_gpu_shader_fp64 #1998

Add support for ARB_gpu_shader_fp64 #1998

Conversation

ShchchowAMD commented Nov 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment