Bug 226660

Summary: [Metal ANGLE] Shaders with reserved metal keywords do not translate, nor do shaders with struct and variable names that are the same except prefixed by an underscore
Product: WebKit Reporter: John Cunningham <johncunningham>
Component: WebGLAssignee: Kyle Piddington <kpiddington>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, ews-watchlist, kbr, kkinnunen, kondapallykalyan, kpiddington, markus, oliver, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Testcase
none
Patch
kpiddington: commit-queue+
Patch
none
Patch none

Description John Cunningham 2021-06-04 13:23:39 PDT
[Metal ANGLE] Shaders with reserved metal keywords do not translate, nor do shaders with struct and variable names that are the same except prefixed by an underscore
Comment 1 John Cunningham 2021-06-04 13:24:03 PDT
Created attachment 430604 [details]
Patch
Comment 2 John Cunningham 2021-06-04 13:30:35 PDT
Created attachment 430605 [details]
Testcase
Comment 3 Kenneth Russell 2021-06-04 14:44:24 PDT
Note that the WebGL variant of GLSL ES reserves some keyword prefixes:
https://www.khronos.org/registry/webgl/specs/latest/1.0/#4.3

so you could safely rename any of these variables which collide with Metal's reserved ones by prepending for example "_webgl_" to it after the initial parse and validation.
Comment 4 John Cunningham 2021-06-04 15:21:41 PDT
(In reply to Kenneth Russell from comment #3)
> Note that the WebGL variant of GLSL ES reserves some keyword prefixes:
> https://www.khronos.org/registry/webgl/specs/latest/1.0/#4.3
> 
> so you could safely rename any of these variables which collide with Metal's
> reserved ones by prepending for example "_webgl_" to it after the initial
> parse and validation.

Correct, we actually do a pass already of this in RewriteKeywords.cpp, with the keywords defined in GetMSLKeywords, however, in this case something is not being plumbed through correctly and the renaming is not consistent throughout the generated shader.
Comment 5 Alexey Proskuryakov 2021-06-04 15:26:36 PDT
Comment on attachment 430605 [details]
Testcase

View in context: https://bugs.webkit.org/attachment.cgi?id=430605&action=review

> LayoutTests/fast/canvas/webgl/shader-with-reserved-keyword-expected.txt:7
> +TEST COMPLETE

The test needs to be updated for this to be the last line.
Comment 6 John Cunningham 2021-06-04 16:57:40 PDT
Here's the fragment shader converted to MSL:

#define ANGLE_ALWAYS_INLINE __attribute__((always_inline))

ANGLE_ALWAYS_INLINE int ANGLE_int_clamp(int value, int minValue, int maxValue)
{
    return ((value < minValue) ?  minValue : ((value > maxValue) ? maxValue : value));
};

#define ANGLE_SAMPLE_COMPARE_GRADIENT_INDEX 0
#define ANGLE_SAMPLE_COMPARE_LOD_INDEX      1
#define ANGLE_RASTERIZATION_DISCARD_INDEX   2
#define ANGLE_COVERAGE_MASK_ENABLED_INDEX   3

constant bool ANGLE_UseSampleCompareGradient [[function_constant(ANGLE_SAMPLE_COMPARE_GRADIENT_INDEX)]];
constant bool ANGLE_UseSampleCompareLod      [[function_constant(ANGLE_SAMPLE_COMPARE_LOD_INDEX)]];
constant bool ANGLE_RasterizationDiscard     [[function_constant(ANGLE_RASTERIZATION_DISCARD_INDEX)]];
constant bool ANGLE_CoverageMaskEnabled      [[function_constant(ANGLE_COVERAGE_MASK_ENABLED_INDEX)]];

ANGLE_ALWAYS_INLINE void ANGLE_writeSampleMask(const uint mask,
                                               thread uint& gl_SampleMask)
{
    if (ANGLE_CoverageMaskEnabled)
    {
        gl_SampleMask = as_type<int>(mask);
    }
}

#define ANGLE_tensor metal::array
#pragma clang diagnostic ignored "-Wunused-value"
struct ANGLE_s
{
  float ANGLE_1_metal;
  char ANGLE_5_pad[12];
};

struct ANGLE_DepthRangeParams
{
  float ANGLE_near;
  float ANGLE_far;
  float ANGLE_diff;
  float ANGLE_reserved;
};

struct ANGLE_AngleUniforms
{
  metal::float4 ANGLE_viewport;
  metal::float2 ANGLE_halfRenderArea;
  metal::float2 ANGLE_flipXY;
  metal::float2 ANGLE_negFlipXY;
  uint ANGLE_clipDistancesEnabled;
  uint ANGLE_xfbActiveUnpaused;
  uint ANGLE_xfbVerticesPerDraw;
  metal::int4 ANGLE_xfbBufferOffsets;
  metal::uint4 ANGLE_acbBufferOffsets;
  ANGLE_DepthRangeParams ANGLE_depthRange;
  metal::float2x4 ANGLE_preRotation;
  metal::float2x4 ANGLE_fragRotation;
  uint coverageMask;
};

struct ANGLE_FragmentOut
{
  metal::float4 color [[color(0)]];
  uint gl_SampleMask [[sample_mask, function_constant(ANGLE_CoverageMaskEnabled)]];
};

ANGLE_s ANGLE_sb6c;

void ANGLE_helper(ANGLE_s ANGLE_s)
{
}

void ANGLE_0_main(thread ANGLE_FragmentOut & ANGLE_fragmentOut)
{
  ANGLE_fragmentOut.color = metal::float4(0.0f, 0.0f, 0.0f, 0.0f);
  ANGLE_s ANGLE_s = ANGLE_s{0.0f};
  ANGLE_helper(ANGLE_s);
  ANGLE_fragmentOut.color = metal::float4(0.0f, 0.80000001f, 0.0f, 1.0f);
}

fragment ANGLE_FragmentOut main0(constant ANGLE_AngleUniforms & ANGLE_angleUniforms [[buffer(17)]])
{
  ANGLE_FragmentOut ANGLE_fragmentOut;
  {
    ANGLE_0_main(ANGLE_fragmentOut);
    if (ANGLE_CoverageMaskEnabled)
    {
      ANGLE_writeSampleMask(ANGLE_angleUniforms.coverageMask, ANGLE_fragmentOut.gl_SampleMask);
    } else {}
  }
  return ANGLE_fragmentOut;;
}

If I compile this I get the following errors:

ANGLE_s ANGLE_sb6c;
        ^
shader.metal:77:28: error: expected ';' at end of declaration
  ANGLE_s ANGLE_s = ANGLE_s{0.0f};
                           ^
                           ;
shader.metal:77:21: warning: variable 'ANGLE_s' is uninitialized when used within its own initialization [-Wuninitialized]
  ANGLE_s ANGLE_s = ANGLE_s{0.0f};
          ~~~~~~~   ^~~~~~~
1 warning and 2 errors generated.
Comment 7 Kenneth Russell 2021-06-04 17:04:16 PDT
It looks to me like the name mangling scheme in the direct-to-Metal backend has to be changed to make sure that structs' names and variables' names don't collide.
Comment 8 Radar WebKit Bug Importer 2021-06-11 13:24:18 PDT
<rdar://problem/79213149>
Comment 9 Kyle Piddington 2021-06-15 18:03:03 PDT
Created attachment 431506 [details]
Patch
Comment 10 EWS Watchlist 2021-06-15 18:04:05 PDT
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment 11 John Cunningham 2021-06-15 18:14:57 PDT
Comment on attachment 431506 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=431506&action=review

LGTM. r+ with the expectation that you will fix the nits

> Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect/RewriteKeywords.cpp:137
> +            renamed->setQualifier(type.getQualifier());

is there a constructor that takes a qualifier? otherwise it's okay

> Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect/RewriteKeywords.cpp:158
> +        

not sure what happened here, maybe just formatting

> LayoutTests/fast/canvas/webgl/shader-with-reserved-keyword-expected.txt:15
> +TEST COMPLETE

Alexey's previous comment suggests this should be at the end.

> LayoutTests/fast/canvas/webgl/shader-with-reserved-keyword.html:33
> +    	description("Tests that program compiling/linking with a reserved keyword.");

indentation

> LayoutTests/fast/canvas/webgl/shader-with-reserved-keyword.html:38
> +            var fShaderSouce ='#version 300 es\n precision mediump float;\n out vec4 color;\n struct s { float metal; };\n float helper(s _s) { return _s.metal; }\n void main() { s _s; float a = helper(_s); color = vec4(a,0.8,0,1); }\n'

space after "=", same for each fShaderSouce. Also typo for Souce ;)
Comment 12 Dean Jackson 2021-06-15 18:19:02 PDT
Comment on attachment 431506 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=431506&action=review

> Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect/RewriteKeywords.cpp:159
>          renamed->setLayoutQualifier(type.getLayoutQualifier());
> +        
>  

Oops.

> LayoutTests/fast/canvas/webgl/shader-with-reserved-keyword-expected.txt:22
> +PASS successfullyParsed is true
> +
> +TEST COMPLETE
> +PASS getError was expected value: NO_ERROR : no error for using reserved keyword in struct
> +PASS getError was expected value: NO_ERROR : no error for using reserved keyword for struct name
> +PASS getError was expected value: NO_ERROR : no error for using reserved keyword for struct name with global struct
> +PASS getError was expected value: NO_ERROR : no error for using reserved keyword in type with global struct
> +PASS getError was expected value: NO_ERROR : no error for using reserved keyword for struct name  with global struct
> +PASS getError was expected value: NO_ERROR : no error for using reserved keyword for variable name with global struct
> +PASS getError was expected value: NO_ERROR : no error for using reserved keyword for variable name and struct name with global struct

I'm not sure why the test output is coming after the test complete message. I think it's because you're using the webgl test utils but also mixing it with the WebKit utils.

> LayoutTests/fast/canvas/webgl/shader-with-reserved-keyword.html:22
> +        console.log(gl.getShaderInfoLog(fragmentShader));

I think you left this in by accident.
Comment 13 Kyle Piddington 2021-06-16 13:41:43 PDT
Created attachment 431591 [details]
Patch
Comment 14 Kyle Piddington 2021-06-16 13:54:58 PDT
Created attachment 431597 [details]
Patch
Comment 15 John Cunningham 2021-06-16 14:08:26 PDT
Comment on attachment 431597 [details]
Patch

LGTM
Comment 16 John Cunningham 2021-06-16 14:08:27 PDT
Comment on attachment 431597 [details]
Patch

LGTM
Comment 17 EWS 2021-06-17 15:35:43 PDT
Committed r279016 (238941@main): <https://commits.webkit.org/238941@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431597 [details].
Comment 18 Kyle Piddington 2021-07-08 13:56:28 PDT
*** Bug 226865 has been marked as a duplicate of this bug. ***
Comment 19 Kimmo Kinnunen 2021-12-13 00:04:12 PST
*** Bug 234220 has been marked as a duplicate of this bug. ***