o3de / o3de-azslc

Amazon Shader Language (AZSL) Compiler
Other
23 stars 14 forks source link

Updating to hlsl6 6 #61

Closed siliconvoodoo closed 2 years ago

siliconvoodoo commented 2 years ago

It's not a complete update (missing payload PAQ) but it's a big overall refresh. List of changes:

1/ report that globallycoherent keyword is missing temporarily bypassable by using [verbatim("globallycoherent)"] workaround.

went to https://github.com/microsoft/DirectXShaderCompiler/blob/50c0199c17b0b7530cbc0bdddadc3697eaece695/tools/clang/include/clang/Basic/TokenKinds.def to find missing tokens

list is incomplete, need to crossref with doc: https://docs.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hlsl-appendix-keywords

missing and included:

center
export
globallycoherent
sampler_state
snorm
unorm
tbuffer
indices
vertices
payload

-> problem of reserving names. DXC uses "special identifier" semantic. in AntlR: it's harder (refer to 2/)

example from DXC repository: center_kwd.hlsl

  // RUN: %dxc -T ps_6_0 -Od -E main %s | FileCheck %s
  // CHECK: @main
  // make sure 'center' is allowed as an interpolation modifier
  float main(center float t : T) : SV_TARGET
  {
      // and also as an identifier
      float center = 10.0f;
      return center * 2;
  }

decided not to include yet:

min16float
min10float
min16int
min12int
min16uint
asm
asm_fragment

not to include probably ever:

compile_fragment
fxgroup
pixelfragment
vertexfragment
RenderTargetView, DepthStencilView, DepthStencilState
RasterizerState
VertexShader, ComputeShader, CompileShader, DomainShader, GeometryShader, HullShader, compile
technique, technique10, technique11

2/ Noticed while testing function declaration parsing, mistake that dates back May 2020: it is impossible to use "in out" or "const" or any qualifier other than static or inline on nameless function parameters, and return type.

found at random by changing parsing rules around qualifiers which caused a greedy parse and shifted param name into type. rule is:

qualifier* type name?

so "const int i" ok because named-param "const int" not ok because unnamed-param zealous semantic check/bug

there may be a solution to use a generic lexer match (Identifier) inside qualifier rule. using qualifier*? which causes the rule to be non-greedy. to investigate later.

in 1.7.35 this can't build:

static const int f()
{
    return 2;
}
// Semantic error #33: Functions can only have either static or inline modifiers.

--> fixed by inverting the logic (all passes except AZSL additions: options/rootconstant).

3/ mesh shaders

https://microsoft.github.io/DirectX-Specs/d3d/MeshShader.html#example-1-passthrough

example from document doesn't build:

[outputtopology("triangle")]
[numthreads(NUM_THREADS_X, NUM_THREADS_Y, NUM_THREADS_Z)]
void PassthroughMesh shader(
    in uint tid : SV_DispatchThreadID,
    in uint tig : SV_GroupIndex,
    out vertices MyOutputVertex verts[MAX_NUM_VERTS],
    out indices uint3 triangles[MAX_NUM_PRIMS])

PassthroughMesh isn't a keyword it's a typo. That was hard to find because of strong subconscious assumption that the doc is the truth.

Similarly: many official examples and tests have SV_RayPayload semantic.

https://github.com/microsoft/DirectXShaderCompiler/search?p=1&q=sv_raypayload

This is a fakenews semantic that doesn't exist. it's ignored. no codepath in DXC source can recognize it.

4/ Working mesh shader found in D3D12MeshShaders sample solution.

Surprise found: ConstantBuffer<MeshInfo> MeshInfo : register(b1);

This can't pass AZSLc because MeshInfo variable has the same name as MeshInfo type. message:

ConstantBuffer's generic type /u_/MeshInfo must be user defined, but seen as IsNotType

same problem for

struct A{};
A A;

redeclaration of /A with a different kind: Variable but was Struct, first seen line 1

Investigated what to do, but concluded against support. (c.f comment in code at commit 5a927ba62e7fab29df69fb74047)

5/ 16 bits types.

preamble: some person from Tencent added:

uint32_t
uint64_t

but not their vector, and matrix keyword expansions:

uint32_t3
uint32_t1x1
...

for 16bits: I remember we added -enable-16bit-types to the builder a long time ago. That said, there is no grammar support for int16_t doc: https://github.com/microsoft/DirectXShaderCompiler/wiki/16-Bit-Scalar-Types

the doc says "scalar type" but DXC actually swallows all the vector/matrix variants as well.

6/ I managed to improve the syntax error display from: syntax error #1: no viable alternative at input 'voidmain(payloadMyPayloadpayload' to: syntax error #1: no viable alternative at input 'void main(payload MyPayload payload'

by using a hidden channel and a comment channel on antlr lexer.

7/ noticed but not yet fixed: in SRGsemantic block: ShaderVariantFallback = stuff

the initializer "stuff" is not well checked and can be anything (string, bool...)

8/ fixed up launch_grun.bat (problem with CLASSPATH)

9/ Since the support of #line directive, we reported messages with 1 line off. This is due to a misunderstanding of #line behavior.

#line 1 "myprog"
error  // myprog:1:1: error: unknown type name 'error'

standard 16.4.3:

behave as if the following sequence of source lines begins with a source line that has a line number as specified

9.5/ refactoring: the output stream sill counts linefeeds, but now does it through an interface Streamable. the line emission system is entirely redone, and we don't emit unnecessary linefeeds anymore.

10/ there is an antlr 4.11 that could be interesting since huge changes happened in 4.10 on the C++ platform (lots of optimizations). we have 4.9.3 now. it seems the scheme to get it is through a git clone (called by cmake?) to an "external" o3de repository?

11/ unsigned int16_t g;

this crashes DXC.

Internal compiler error: access violation. Attempted to read from address 0x0000000000000008

reported at https://github.com/microsoft/DirectXShaderCompiler/issues/4648

12/ grammar: Because of the introduction of new int types, "unsigned" needed to move to the qualifiers rule, to lift the necessity of repetitions of 20 vector/matrix expansions x2 in the lexer, and allow arbitrary placement eg unsigned const int. This disrupted the AST possible structure, which now will tolerates qualifiers in type contexts. effect on code: -> refactoring: type qualifiers are no longer stored in VarInfo and FuncInfo but in ExtendedTypeInfo

example previsouly syntax errors, but now passing:

typedef const int UI;
ConstantBuffer< volatile Color >
typealias U = const T;
const float f();   // returning const types was not acceptable grammar in 1.7.35

previously semantic error but now pass unchecked:

ConstantBuffer< ConstantBuffer<A> > v;  // it was enforced to not allow that.
ConstantBuffer<A> func(ConstantBuffer<B> cbb)  // functions couldn't return CB nor have them as argument
{
  ConstantBuffer<A> cb = cbb;  // local variables with CB type were hard errors (only tolerated in SRG)
}

rationale: there is an opportunity for DXC to recognize references types as aliases (that have no physical existence) For example when used in selector Functions ConstantBuffer<S> Select(bool a_or_b) { return a_or_b ? g_cb1 : g_cb2; } Even though DXC sort of seem to build this, in some contexts it does trigger a complaint that this type shouldn't appear here. To be fair, the selector function can undecorate the viewtype (chameleon type in azslc parlance) by performing the access: S Select(bool a_or_b) { return a_or_b ? g_cb1 : g_cb2; } and be more guaranteed to be legal. But anyhow, I decided we don't need to care to validate that, let's reduce code complexity by offsetting burden of validation to downstream tools.

13/ Further improvement of syntax error reporting by detecting when the offending token is a keyword. this way, if someone gets a "no viable alternative" on a visibly valid code: void f(Payload payload){} // <- this is an error in AZSL 1.8 because payload is reserved! message now:

syntax error #1: no viable alternative at input 'void f(Payload payload' (payload is a keyword)

this will relax the frustration from surprising syntax errors with supercommon names: "indices, vertices, center, line, payload..."

14/ Syntax still remaining to support: PAQ (HLSL 6.6 payload qualifiers) eg: float4 irradiance : read(caller) : write(closesthit, miss);

15/ Improved "option for int must use range" error message with an example syntax to teach users the solution without google.

siliconvoodoo commented 2 years ago

all green. good to go.