More fixes for Falcor IR support (shader-slang#317)

* Fix: try to improve float literal formatting

An earlier change switched to using the C++ stdlib to format floats, but I neglected to check that it would correctly print values that happen to be integral with a decimal place (e.g., print `1.0` instead of just `1`). That causes some obscure cases to fail (e.g., when you write `1.0.xxxx` in HLSL, and we print out `1.xxxx`).

I've tried to resolve the issue by using the "fixed" output format, but that may also create problems for extremely large or small values (which really need to use scientific notation). However, this at least puts us back where we were before...

* Add support for source locations to the IR

- Add a `sourceLocation` field to all `IRValue`s
- Add some logic to associate locations with operations while lowering (using a slightly "clever" RAII approach)
- Make sure that when cloning instructions, we also clone the location
- Make the emit logic use the existing support for source locations when emitting code from the IR

A nice cleanup to this work would be to have the source locations for IR values not be hyper-specific about both line and column; it is probably enough to just emit on the correct line.

Author: Tim Foley

source/slang/emit.cpp

@@ -578,6 +578,7 @@ struct EmitVisitor
 
         std::ostringstream stream;
         stream.imbue(std::locale::classic());
+        stream.setf(std::ios::fixed,std::ios::floatfield);
         stream.precision(20);
         stream << value;
 
@@ -5322,6 +5323,9 @@ emitDeclImpl(decl, nullptr);
         IRValue*        value)
     {
         IRInst* inst = (IRInst*) value;
+
+        advanceToSourceLocation(inst->sourceLoc);
+
         switch(value->op)
         {
         case kIROp_IntLit:
@@ -5617,6 +5621,8 @@ emitDeclImpl(decl, nullptr);
             return;
         }
 
+        advanceToSourceLocation(inst->sourceLoc);
+
         switch(inst->op)
         {
         default:
@@ -5780,6 +5786,8 @@ emitDeclImpl(decl, nullptr);
 
             // Now look at the terminator instruction, which will tell us what we need to emit next.
 
+            advanceToSourceLocation(terminator->sourceLoc);
+
             switch (terminator->op)
             {
             default:

source/slang/ir-insts.h

@@ -343,6 +343,8 @@ struct SharedIRBuilder
     Dictionary<IRConstantKey,   IRConstant*>    constantMap;
 };
 
+struct IRBuilderSourceLocRAII;
+
 struct IRBuilder
 {
     // Shared state for all IR builders working on the same module
@@ -366,6 +368,8 @@ struct IRBuilder
     IRGlobalValueWithCode*  getFunc() { return curFunc; }
     IRBlock*                getBlock() { return curBlock; }
 
+    IRBuilderSourceLocRAII* sourceLocInfo = nullptr;
+
     void addInst(IRBlock* block, IRInst* inst);
     void addInst(IRInst* inst);
 
@@ -559,6 +563,33 @@ struct IRBuilder
     IRLayoutDecoration* addLayoutDecoration(IRValue* value, Layout* layout);
 };
 
+// Helper to establish the source location that will be used
+// by an IRBuilder.
+struct IRBuilderSourceLocRAII
+{
+    IRBuilder*  builder;
+    SourceLoc   sourceLoc;
+    IRBuilderSourceLocRAII* next;
+
+    IRBuilderSourceLocRAII(
+        IRBuilder*  builder,
+        SourceLoc   sourceLoc)
+        : builder(builder)
+        , sourceLoc(sourceLoc)
+        , next(nullptr)
+    {
+        next = builder->sourceLocInfo;
+        builder->sourceLocInfo = this;
+    }
+
+    ~IRBuilderSourceLocRAII()
+    {
+        assert(builder->sourceLocInfo == this);
+        builder->sourceLocInfo = next;
+    }
+};
+
+
 //
 
 // Interface to IR specialization for use when cloning target-specific

source/slang/ir.cpp

@@ -244,6 +244,32 @@ namespace Slang
         addInst(parent, inst);
     }
 
+    static void maybeSetSourceLoc(
+        IRBuilder*  builder,
+        IRValue*    value)
+    {
+        if(!builder)
+            return;
+
+        auto sourceLocInfo = builder->sourceLocInfo;
+        if(!sourceLocInfo)
+            return;
+
+        // Try to find something with usable location info
+        for(;;)
+        {
+            if(sourceLocInfo->sourceLoc.getRaw())
+                break;
+
+            if(!sourceLocInfo->next)
+                break;
+
+            sourceLocInfo = sourceLocInfo->next;
+        }
+
+        value->sourceLoc = sourceLocInfo->sourceLoc;
+    }
+
     static IRValue* createValueImpl(
         IRBuilder*  /*builder*/,
         UInt        size,
@@ -273,7 +299,7 @@ namespace Slang
     // arguments *after* the type (which is a mandatory
     // argument for all instructions).
     static IRInst* createInstImpl(
-        IRBuilder*      /*builder*/,
+        IRBuilder*      builder,
         UInt            size,
         IROp            op,
         IRType*         type,
@@ -291,6 +317,8 @@ namespace Slang
 
         inst->type = type;
 
+        maybeSetSourceLoc(builder, inst);
+
         auto operand = inst->getArgs();
 
         for( UInt aa = 0; aa < fixedArgCount; ++aa )
@@ -858,6 +886,7 @@ namespace Slang
             this,
             kIROp_Func,
             nullptr);
+        maybeSetSourceLoc(this, rsFunc);
         addGlobalValue(getModule(), rsFunc);
         return rsFunc;
     }
@@ -870,6 +899,7 @@ namespace Slang
             this,
             kIROp_global_var,
             ptrType);
+        maybeSetSourceLoc(this, globalVar);
         addGlobalValue(getModule(), globalVar);
         return globalVar;
     }
@@ -3112,7 +3142,8 @@ namespace Slang
             }
         }
 
-        // TODO: implement this
+        // We will also clone the location here, just because this is a convenient bottleneck
+        clonedValue->sourceLoc = originalValue->sourceLoc;
     }
 
     struct IRSpecContext : IRSpecContextBase

source/slang/ir.h

@@ -9,6 +9,8 @@
 
 #include "../core/basic.h"
 
+#include "source-loc.h"
+
 namespace Slang {
 
 class   Decl;
@@ -136,6 +138,9 @@ struct IRValue
 
     Type* getType() { return type; }
 
+    // Source location information for this value, if any
+    SourceLoc sourceLoc;
+
     // The linked list of decorations attached to this value
     IRDecoration* firstDecoration = nullptr;
 

source/slang/lower-to-ir.cpp

@@ -998,6 +998,7 @@ struct ExprLoweringVisitorBase : ExprVisitor<Derived, LoweredValInfo>
     // as the visitor itself.
     LoweredValInfo lowerSubExpr(Expr* expr)
     {
+        IRBuilderSourceLocRAII sourceLocInfo(getBuilder(), expr->loc);
         return this->dispatch(expr);
     }
 
@@ -1667,6 +1668,8 @@ LoweredValInfo lowerLValueExpr(
     IRGenContext*   context,
     Expr*           expr)
 {
+    IRBuilderSourceLocRAII sourceLocInfo(context->irBuilder, expr->loc);
+
     LValueExprLoweringVisitor visitor;
     visitor.context = context;
     return visitor.dispatch(expr);
@@ -1676,6 +1679,8 @@ LoweredValInfo lowerRValueExpr(
     IRGenContext*   context,
     Expr*           expr)
 {
+    IRBuilderSourceLocRAII sourceLocInfo(context->irBuilder, expr->loc);
+
     RValueExprLoweringVisitor visitor;
     visitor.context = context;
     return visitor.dispatch(expr);
@@ -2380,6 +2385,8 @@ void lowerStmt(
     IRGenContext*   context,
     Stmt*           stmt)
 {
+    IRBuilderSourceLocRAII sourceLocInfo(context->irBuilder, stmt->loc);
+
     StmtLoweringVisitor visitor;
     visitor.context = context;
     return visitor.dispatch(stmt);
@@ -2617,6 +2624,8 @@ struct DeclLoweringVisitor : DeclVisitor<DeclLoweringVisitor, LoweredValInfo>
         //
         for (auto decl : declGroup->decls)
         {
+            IRBuilderSourceLocRAII sourceLocInfo(context->irBuilder, decl->loc);
+
             // Note: I am directly invoking `dispatch` here,
             // instead of `ensureDecl` just to try and
             // make sure that we don't accidentally
@@ -3493,6 +3502,8 @@ LoweredValInfo lowerDecl(
     IRGenContext*   context,
     DeclBase*       decl)
 {
+    IRBuilderSourceLocRAII sourceLocInfo(context->irBuilder, decl->loc);
+
     DeclLoweringVisitor visitor;
     visitor.context = context;
     return visitor.dispatch(decl);