[SPIR-V] add support for extension KHR_fragment_shading_barycentric (SV_Barycentrics)

[ShchchowAMD]

• [SPIR-V] Add support for SPV_KHR_fragment_shading_barycentrics (SV_Barycentrics)

  Implement latest SV_Barycentrics semantics in DXC, according to DXC wiki spec.

  For variant qualifier decorated baryCoord inputs, as KHR extension has no matched built-in, still mapped to Bary*AMD.

  Add support for using input as first parameter of GetAttributeAtVertex. This would expand input's dimension to meet extension spec.

  Merge some features from AMD_shader_explicit_parameter and current extension together for MSAA usage. Keep generated SPIR-V codes similar to GLSL as possible.

  With concern to HLSL/DXIL part, keep most type modification/variable decoration change in SPIRV trans unit.

Ref: https://github.com/microsoft/DirectXShaderCompiler/wiki/SV_Barycentrics

[AppVeyorBot]

:x: Build DirectXShaderCompiler 1.0.1965 failed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/41a5a9d062 by @ShchchowAMD)

[AppVeyorBot]

:x: Build DirectXShaderCompiler 1.0.1966 failed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/abb57c2aab by @ShchchowAMD)

[AppVeyorBot]

:x: Build DirectXShaderCompiler 1.0.2057 failed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/d851b5ca2b by @ShchchowAMD)

[AppVeyorBot]

:x: Build DirectXShaderCompiler 1.0.2058 failed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/84d43a3564 by @ShchchowAMD)

[AppVeyorBot]

:x: Build DirectXShaderCompiler 1.0.2059 failed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/0171587c60 by @ShchchowAMD)

[AppVeyorBot]

:white_check_mark: Build DirectXShaderCompiler 1.0.2060 completed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/f5f81c8400 by @ShchchowAMD)

[AppVeyorBot]

:white_check_mark: Build DirectXShaderCompiler 1.0.2065 completed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/0e2bae8ec0 by @ShchchowAMD)

[AppVeyorBot]

:white_check_mark: Build DirectXShaderCompiler 1.0.2066 completed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/0dfdbf3f07 by @ShchchowAMD)

[cassiebeckley]

In addition, this PR generates invalid SPIR-V for some inputs. For example, running dxc -T ps_6_1 -E main foo.hlsl -spirv on

enum VertexID {
    FIRST = 0,
    SECOND = 1,
    THIRD = 2
};


float3 main( float3 vBaryWeights : SV_Barycentrics,
    nointerpolation float3 Color : COLOR ) : SV_Target
{
    float3 vColor = GetAttributeAtVertex( Color, VertexID::SECOND );
    return Color * vColor.x;
}

returns the error message

fatal error: generated SPIR-V is invalid: Expected vector operand type to be equal to Result Type: VectorTimesScalar
  %17 = OpVectorTimesScalar %v3float %14 %16

note: please file a bug report on https://github.com/Microsoft/DirectXShaderCompiler/issues with source code if possible

[ShchchowAMD]

In addition, this PR generates invalid SPIR-V for some inputs. For example, running dxc -T ps_6_1 -E main foo.hlsl -spirv on

enum VertexID {
    FIRST = 0,
    SECOND = 1,
    THIRD = 2
};


float3 main( float3 vBaryWeights : SV_Barycentrics,
    nointerpolation float3 Color : COLOR ) : SV_Target
{
    float3 vColor = GetAttributeAtVertex( Color, VertexID::SECOND );
    return Color * vColor.x;
}

returns the error message

fatal error: generated SPIR-V is invalid: Expected vector operand type to be equal to Result Type: VectorTimesScalar
  %17 = OpVectorTimesScalar %v3float %14 %16

note: please file a bug report on https://github.com/Microsoft/DirectXShaderCompiler/issues with source code if possible

I do have concerns here too.

For current PR, we need to change input COLOR to an three-element array when generating SPIR-V code. The purpose is to set new decorator and change its type to meet new feature.

I tried to keep its influence scope small so this would only happen when COLOR is used as first parameter of GetAttributeAtVertex.

Which means, it would be correct when you use either GetAttributeAtVertex or Color here within a shader. But it will report error when you use them together, as the input has been changed to an array.

It is assumed here when you are using GetAttributeAtVertex, input COLOR is treated as pervertex data array.

[AppVeyorBot]

:white_check_mark: Build DirectXShaderCompiler 1.0.2139 completed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/3cff6ec831 by @ShchchowAMD)

[AntonTokarev]

Any updates on this? Missing this feature :)

[ShchchowAMD]

Any updates on this? Missing this feature :)

I'm waiting for review now. You can have a try with this PR, main features should have been finished.

[ShchchowAMD]

Fixed conflict in spvEmitter. Add comment to SV_BaryCentrics builtin cases in CapatibilityVisitor.

[AppVeyorBot]

:x: Build DirectXShaderCompiler 1.0.2393 failed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/039815a7ef by @ShchchowAMD)

[AppVeyorBot]

:white_check_mark: Build DirectXShaderCompiler 1.0.2395 completed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/93b01fa761 by @ShchchowAMD)

[AppVeyorBot]

:white_check_mark: Build DirectXShaderCompiler 1.0.2396 completed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/5fcd3d27b7 by @ShchchowAMD)

[ShchchowAMD]

@cassiebeckley Hi cassie, I've resolved conflicts, any further changes I need to make for this PR?

[AntonTokarev]

Hi! Any updates on this?

[Tobski]

@s-perron looks like this was assigned to you for review, are you able to take a look?

@ShchchowAMD can you rebase this again please?

[ShchchowAMD]

@s-perron looks like this was assigned to you for review, are you able to take a look?

@ShchchowAMD can you rebase this again please?

I'll resolve the conflict today. Thanks.

[ShchchowAMD]

Conflict resolved.

[AppVeyorBot]

:white_check_mark: Build DirectXShaderCompiler 1.0.2911 completed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/efb47174c7 by @ShchchowAMD)

[ShchchowAMD]

@cassiebeckley Hi cassie, current PR still needs your approval as there's a 'change requested' step from last review. Is it okay to merge with current impl?

[ShchchowAMD]

@cassiebeckley looks like there's still one 'change requested' needs your approval. Would you please help to verify and let me merge this PR?

[s-perron]

@llvm-beanz Could you please look at the changes to the AST? Cassie is not sure if there is a better way to implement this in the AST portion.

[s1sw]

Hi, any updates on this?

[llvm-beanz]

I'm a bit concerned about the implementation and testing here.

We have parsing changes, with no testing specifically focused on the parser. It looks like we're hooking into the parser to run call diagnostics, which... seems wrong. This also introduces a stateful-ness in the Sema object...

I'm going to need at least a day to wrap my head around what this change is trying to do.

[Tobski]

@ShchchowAMD in case you haven't seen this, can you look at/respond to @llvm-beanz's comments?

[ShchchowAMD]

@Tobski @llvm-beanz Sorry for the late response.

I'll refine current implementation and try to avoid problems beanz mentioned above. It may take some time.

Thanks very much for all the advice and notes beanz shared here. I'll try to keep new changes following those points and avoid introducing new issues in above modules.

BTW, in case I misunderstanding, should I try to keep most of changes in SPV level instead of storing data or pass info to AST and Sema? The reason of using current solution is that:

1. I found I must change type of first parameter (PervertexKHR decorated) to an array to meet spec for GetAttributeAt.

2. (If I understand right, I assumed that we will use KHR prefix built-ins to replace AMD built-ins after this change).

As an example, in test file semantic.barycentrics.ps.np-c.hlsl we could found this:

float4 main(noperspective centroid float3 bary : SV_Barycentrics) : SV_Target {
    return float4(bary, 1.0);
}

If bary is also used as the first parameter of GetAttributeAt, it should be an 'array', according to spec.

We could change type of 'bary' to an array in SPV level but this would against the 'return' statement (it is treated as a float3, non-array).

I think that's the reason for current implementation (I agree that I should not add too much things in other modules) to avoid spv level errors.

I'll try to find a new way to avoid those problems, my current idea is to add a new temp array input for this issue but not make sure this is correct. Any ideas about this?