Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updating to hlsl6 6 #61

Merged
merged 42 commits into from
Oct 7, 2022
Merged

Conversation

siliconvoodoo
Copy link
Contributor

@siliconvoodoo siliconvoodoo commented Sep 20, 2022

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 5a927ba)

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 microsoft/DirectXShaderCompiler#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.

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
…s recursively).

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
+ add re-emission of interfaces
+ add emission of inheritance lists
+ change base holding from set to vector (to preserve order)
+ fixup base lookup scope (it was target class's scope, must be enclosing)
+ add a typeof case to exploit inheritance lookup

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
…ed members, and also fully qualified idExpression that were bypassing lookup.

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
wip: advanced / verification of seenats not ready

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
… nameless.

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
…declared prior to a nested type, ended up extracted ABOVE the definition of their own parent type.

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
…ce correctly spawns a o3de#17 error.

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
…case (variable cannot have the same name as an already defined symbol in azsl, but valid in C/C++/HLSL)

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
…ifiers to be used with unnamed function parameters.

anti regression: copy function test to semantic check.

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
… answer on how to fix a compiler complaint.

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
center
export
globallycoherent
sampler_state
tbuffer
unorm
snorm
indices
vertices

+ improve syntax error report by preserving whitspaces

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
…ypes.

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
+ moved 'unsigned' as a type qualifier rather than fixed type

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
…e reemission

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
… and the virtual calls, compared to say, use template functions where stream objects are expected. To be judged.

This commit fixes the blank emission of attributes or other emitter code that used overloaded operator<< and Backend methods.

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
…pe resolving overload to char type

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
…roduced by new keywords indices and vertices.

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
…type, a functionType and the qualifiers.

+ this will rehabilitate the possibility to use "unsigned int" in contexts where type didnt have qualifiers allowed.
remainder: this possibility dissappeared when we moved unsigned to qualifiers.
+ remove overly strict 'constant buffer presence in SRG only' semantic check
we can let DXC do the validation. even if it's very weak at times on this matter.

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
…ed qualifiers in the type AST rule. Especially "in/out/inout"

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
Merge branch 'development' into updating-to-hlsl6_6

# Conflicts:
#	src/AzslcEmitter.cpp
#	src/AzslcEmitter.h
#	src/AzslcKindInfo.h
#	src/AzslcMain.cpp
#	src/AzslcSemanticOrchestrator.cpp

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
…ype emission function.

+ this fixes typedef emission.

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
src/AzslcMain.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@jeremyong-az jeremyong-az left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(will continue to review just leaving a few comments for now, thanks!)

src/AzslcBackend.cpp Show resolved Hide resolved
src/AzslcEmitter.cpp Outdated Show resolved Hide resolved
src/AzslcEmitter.cpp Show resolved Hide resolved
src/AzslcEmitter.h Show resolved Hide resolved
src/AzslcMain.cpp Show resolved Hide resolved
src/AzslcSemanticOrchestrator.cpp Show resolved Hide resolved
src/AzslcUtils.h Outdated Show resolved Hide resolved
src/PreprocessorLineDirectiveFinder.h Show resolved Hide resolved
src/StreamableInterface.h Show resolved Hide resolved
Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
…terns.

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
…n words in the vocabulary.

notably center, vertices and indices.

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
…e storage flags

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
@siliconvoodoo
Copy link
Contributor Author

all green. good to go.

@galibzon galibzon merged commit 727f031 into o3de:development Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants