o3de / o3de-azslc

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

Emitted HLSL contains many unneeded new lines #57

Closed jeremyong-az closed 1 year ago

jeremyong-az commented 2 years ago
// HLSL emission by AZSL Compiler 1.7.35 Win64
#line 14 "D:/o3de-atom-sampleviewer/user/AssetProcessorTemp/JobTemp-KiBTnM/StandardPBR_ForwardPass.azsl.dx12.prepend"

static const float4 s_AzslDebugColor = float4 ( 16.0 / 255.0 , 124.0 / 255.0 , 16.0 / 255.0 , 1 ) ;
#line 11 "D:/o3de/Gems/Atom/Feature/Common/Assets/ShaderLib/Atom/Features/SrgSemantics.azsli"
#line 13 "D:/o3de/Gems/Atom/Feature/Common/Assets/ShaderResourceGroups/Decals/ViewSrg.azsli"

For example ^.

The new lines here corresponded to stripped comments, but instead of emitting new lines, we should just be adjusting the first argument of those #line directives.

siliconvoodoo commented 2 years ago

Wouldn't it be better to preserve the comments? That would give some landmarks to authors who can navigate their code by familiarity, or leave anchors to search for. Especially in case of wrong line reporting, because of potential miss in the line directives (not unheard-of). I don't remember why we chose to remove them, it's possible that the reason is as simple as they just get skipped by AntlR's lexer, and we never made the effort of recovering them.

jeremyong-az commented 2 years ago

@siliconvoodoo There are two reasons I can think of for stripping comments.

  1. If we cache the preprocessed HLSL as an intermediate asset (something @santorac is prototyping), cached shader binaries would not be affected by either dead code (stripped via pound-def) or comments. If we strip unused functions from HLSL in the future, the comments associated with those functions would persist also.
  2. Really fat shaders sometimes don't work well with tools that do source analysis (e.g. Pix or Nsight), but I've noticed this has improved quite a bit in recent years

It may be useful to have an option to preserve them however.

siliconvoodoo commented 2 years ago

fair!

btw I found the likely culprit for the empty lines, I suspect commit 396d653f

Also I'm noting bugs in line directive generation (that we should be relying on instead of empty lines btw). image This might relate to bug https://github.com/o3de/o3de-azslc/issues/39

siliconvoodoo commented 1 year ago

Ok I have a fix. (commit b3518ad1adacd87e4f4676992da00d8bac6f5796) Before: image

In between (change to a line emission system rather than line feeds) image

After improvement with "line economy" (only reemit when out of sync) image

Example2: image

siliconvoodoo commented 1 year ago

Submitted in #61