Re-enable emission of #line directives and clean up output (#554)

This was based on feedback from Falcor users, who felt like changing the default to have no line directives didn't work out well. Since I'd only made them disabled by default based on what I perceived to be Falcor's needs, I'm happy to turn this back on by default.

I also added a few changes to clean up the output:

* Don't emit a directive for a sub-expression, since that breaks up the code too much. The only directives inside a function body will be on top-level instructions that didn't get folded into a use site.

* Add logic to emit a directive for top-level declarations (globals, functions, structs), and clean up their printing so that they put any extra space *after* the declaration rather than before (so the line numbers can be accurate)

* Don't emit the file path part of a directive if it would be the same as the previous directive. This makes the output less noisy, at the cost of having to work your way backward to find the file if you are looking directly at the output.

There are certainly more cleanups possible, but these make the output decent enough to be useful for working backwards from a downstream compiler error to the offending code.

1 files changed, 83 insertions, 88 deletions

diff --git a/source/slang/emit.cpp b/source/slang/emit.cpp
index c86634c02..9874e3ef3 100644
--- a/source/slang/emit.cpp
+++ b/source/slang/emit.cpp
@@ -534,92 +534,96 @@ struct EmitVisitor
         sprintf(buffer, "%llu", (unsigned long long)sourceLocation.line);
         emitRawText(buffer);
 
-        emitRawText(" ");
-
-        bool shouldUseGLSLStyleLineDirective = false;
-
-        auto mode = context->shared->entryPoint->compileRequest->lineDirectiveMode;
-        switch (mode)
+        // Only emit the path part of a `#line` directive if needed
+        if(sourceLocation.path != context->shared->loc.path)
         {
-        case LineDirectiveMode::None:
-        case LineDirectiveMode::Default:
-            SLANG_UNEXPECTED("should not be trying to emit '#line' directive");
-            return;
+            emitRawText(" ");
 
-        default:
-            // To try to make the default behavior reasonable, we will
-            // always use C-style line directives (to give the user
-            // good source locations on error messages from downstream
-            // compilers) *unless* they requested raw GLSL as the
-            // output (in which case we want to maximize compatibility
-            // with downstream tools).
-            if (context->shared->finalTarget == CodeGenTarget::GLSL)
-            {
-                shouldUseGLSLStyleLineDirective = true;
-            }
-            break;
+            bool shouldUseGLSLStyleLineDirective = false;
 
-        case LineDirectiveMode::Standard:
-            break;
+            auto mode = context->shared->entryPoint->compileRequest->lineDirectiveMode;
+            switch (mode)
+            {
+            case LineDirectiveMode::None:
+                SLANG_UNEXPECTED("should not be trying to emit '#line' directive");
+                return;
 
-        case LineDirectiveMode::GLSL:
-            shouldUseGLSLStyleLineDirective = true;
-            break;
-        }
+            case LineDirectiveMode::Default:
+            default:
+                // To try to make the default behavior reasonable, we will
+                // always use C-style line directives (to give the user
+                // good source locations on error messages from downstream
+                // compilers) *unless* they requested raw GLSL as the
+                // output (in which case we want to maximize compatibility
+                // with downstream tools).
+                if (context->shared->finalTarget == CodeGenTarget::GLSL)
+                {
+                    shouldUseGLSLStyleLineDirective = true;
+                }
+                break;
 
-        if(shouldUseGLSLStyleLineDirective)
-        {
-            auto path = sourceLocation.getPath();
+            case LineDirectiveMode::Standard:
+                break;
 
-            // GLSL doesn't support the traditional form of a `#line` directive without
-            // an extension. Rather than depend on that extension we will output
-            // a directive in the traditional GLSL fashion.
-            //
-            // TODO: Add some kind of configuration where we require the appropriate
-            // extension and then emit a traditional line directive.
+            case LineDirectiveMode::GLSL:
+                shouldUseGLSLStyleLineDirective = true;
+                break;
+            }
 
-            int id = 0;
-            if(!context->shared->mapGLSLSourcePathToID.TryGetValue(path, id))
+            if(shouldUseGLSLStyleLineDirective)
             {
-                id = context->shared->glslSourceIDCount++;
-                context->shared->mapGLSLSourcePathToID.Add(path, id);
-            }
+                auto path = sourceLocation.getPath();
 
-            sprintf(buffer, "%d", id);
-            emitRawText(buffer);
-        }
-        else
-        {
-            // The simple case is to emit the path for the current source
-            // location. We need to be a little bit careful with this,
-            // because the path might include backslash characters if we
-            // are on Windows, and we want to canonicalize those over
-            // to forward slashes.
-            //
-            // TODO: Canonicalization like this should be done centrally
-            // in a module that tracks source files.
+                // GLSL doesn't support the traditional form of a `#line` directive without
+                // an extension. Rather than depend on that extension we will output
+                // a directive in the traditional GLSL fashion.
+                //
+                // TODO: Add some kind of configuration where we require the appropriate
+                // extension and then emit a traditional line directive.
 
-            emitRawText("\"");
-            for(auto c : sourceLocation.getPath())
-            {
-                char charBuffer[] = { c, 0 };
-                switch(c)
+                int id = 0;
+                if(!context->shared->mapGLSLSourcePathToID.TryGetValue(path, id))
                 {
-                default:
-                    emitRawText(charBuffer);
-                    break;
+                    id = context->shared->glslSourceIDCount++;
+                    context->shared->mapGLSLSourcePathToID.Add(path, id);
+                }
 
-                // The incoming file path might use `/` and/or `\\` as
-                // a directory separator. We want to canonicalize this.
+                sprintf(buffer, "%d", id);
+                emitRawText(buffer);
+            }
+            else
+            {
+                // The simple case is to emit the path for the current source
+                // location. We need to be a little bit careful with this,
+                // because the path might include backslash characters if we
+                // are on Windows, and we want to canonicalize those over
+                // to forward slashes.
                 //
-                // TODO: should probably canonicalize paths to not use backslash somewhere else
-                // in the compilation pipeline...
-                case '\\':
-                    emitRawText("/");
-                    break;
+                // TODO: Canonicalization like this should be done centrally
+                // in a module that tracks source files.
+
+                emitRawText("\"");
+                for(auto c : sourceLocation.getPath())
+                {
+                    char charBuffer[] = { c, 0 };
+                    switch(c)
+                    {
+                    default:
+                        emitRawText(charBuffer);
+                        break;
+
+                    // The incoming file path might use `/` and/or `\\` as
+                    // a directory separator. We want to canonicalize this.
+                    //
+                    // TODO: should probably canonicalize paths to not use backslash somewhere else
+                    // in the compilation pipeline...
+                    case '\\':
+                        emitRawText("/");
+                        break;
+                    }
                 }
+                emitRawText("\"");
             }
-            emitRawText("\"");
         }
 
         emitRawText("\n");
@@ -648,11 +652,9 @@ struct EmitVisitor
         switch(mode)
         {
         case LineDirectiveMode::None:
-        case LineDirectiveMode::Default:
-            // Default behavior is to not emit line directives, since they
-            // don't help readability much for IR-based output.
             return;
 
+        case LineDirectiveMode::Default:
         default:
             break;
         }
@@ -3176,8 +3178,6 @@ struct EmitVisitor
         IRInst*         inst,
         IREmitMode      mode)
     {
-        advanceToSourceLocation(inst->sourceLoc);
-
         switch(inst->op)
         {
         case kIROp_IntLit:
@@ -4487,10 +4487,6 @@ struct EmitVisitor
     {
         auto resultType = func->getResultType();
 
-        // Put a newline before the function so that
-        // the output will be more readable.
-        emit("\n");
-
         // Deal with decorations that need
         // to be emitted as attributes
         auto entryPointLayout = asEntryPoint(func);
@@ -4553,11 +4549,11 @@ struct EmitVisitor
             emitIRStmtsForBlocks(ctx, func->getFirstBlock(), nullptr, nullptr);
 
             dedent();
-            emit("}\n");
+            emit("}\n\n");
         }
         else
         {
-            emit(";\n");
+            emit(";\n\n");
         }
     }
 
@@ -4646,7 +4642,7 @@ struct EmitVisitor
 
             emitIRParamType(ctx, paramType, paramName);
         }
-        emit(");\n");
+        emit(");\n\n");
     }
 
     EntryPointLayout* getEntryPointLayout(
@@ -4769,7 +4765,7 @@ struct EmitVisitor
         }
 
         dedent();
-        emit("};\n");
+        emit("};\n\n");
     }
 
     void emitIRMatrixLayoutModifiers(
@@ -5420,9 +5416,6 @@ struct EmitVisitor
             Emit("}\n");
         }
 
-        // Emit a blank line so that the formatting is nicer.
-        emit("\n");
-
         if (auto paramBlockType = as<IRUniformParameterGroupType>(varType))
         {
             emitIRParameterGroup(
@@ -5499,7 +5492,7 @@ struct EmitVisitor
             Emit("()");
         }
 
-        emit(";\n");
+        emit(";\n\n");
     }
 
     void emitIRGlobalConstantInitializer(
@@ -5556,6 +5549,8 @@ struct EmitVisitor
         EmitContext*    ctx,
         IRInst*         inst)
     {
+        advanceToSourceLocation(inst->sourceLoc);
+
         switch(inst->op)
         {
         case kIROp_Func: