Here is my current thinking after talking to @Adrian_Prantl, @vedantk, and @dcci .
From the point of view of the compiler, there should be one concept of a debug location that includes both the debug scope and line info. I'll call that SILDebugLocation. (Adrian already added that type, we just need to convert all our APIs to start using it).
There are various inliner-like passes that need to modify the scope without changing the line info. There should be a utility for doing that independent from the SILBuilder. In general it's easy to build pass-specific builders that do extra book-keeping.
We're trying to solve two problems by changing SILBuilder API:
-
When creating multiple instructions, we may forget to update the debug location.
-
It's easy to set an entirely incorrect location or a location that is inconsistent with the scope.
Let's look at these problem in the context of a strawman proposal:
class SILBuilder {
protected:
SILBasicBlock::iterator insertPosition;
SILDebugLocation debugLocation;
public:
// Basic constructor.
SILBuilder(SILBasicBlock::iterator insertPosition, SILDebugLocation debugLocation);
// Most common convenience constructor for inheriting SILDebugLocation.
explicit SILBuilder(SILInstruction *insertBefore);
LoadInst *createLoad(<load operands only>);
StoreInst *createStore(<store operands only>);
protected:
SILBuilder &operator =(const SILBuilder &);
};
This stripped down builder class makes the reuse problem (#1) easier to identify, and defines away problem #2 wherever it is used. Code that reuses a basic SILBuilder object is explicitly relinquishing control of debug info and insertion point--it is impossible to change either from this interface. It should only be reused in very limited situations in which all the instructions to be created are obviously part of the same location.
Protecting the assignment operator compensates for the fact that we don't declare it's methods const and we're not good about passing around const reference. The point is that the position/location is immutable (without downcasting).
Whenever the compiler expands, inserts, or clones instructions in the same scope, the common convenience constructor for inheriting SILDebugLocation is exactly what the code wants:
SILBuilder(SILInstruction *insertBefore);
The most common usage would be:
SILBuilder(oldInstruction).createLoad(address);
But it could also be used as an abstract interface:
// This utility generates a snippet of code for a single debug location.
// It is unable to change the insertion point or debug information.
void genNewInstruction(const SILBuilder &builder, SILValue address) {
builder.createLoad(address);
}
// When the builder is constructed, we know we're trying to replace an instruction.
SILBuilder B(oldInstruction);
genNewInstruction(B);
In the much less common case of moving code by recreating it while preserving its old location, the basic constructor can be called directly:
SILBuilder(SILBasicBlock::iterator insertPosition, SILDebugLocation debugLocation);
The basic constructor is also useful for inserting implementation instructions that don't correspond to source expressions as "at line 0".
Now comes the question of how to handle existing code that reuses the same SILBuilder for multiple locations and updates the insertion point and debug location as it goes. That code falls into two categories:
For lowering, a simple wrapper around SILBuilder can provide convenience and safety:
class SILGenBuilder {
SILBasicBlock::iterator currentPosition;
SILDebugScope *currentScope;
SILLocation currentExprLocation;
// Conceptual interface. In practice, these would be RAII wrappers
// that maintain a stack.
pushScope(scope);
popScope();
pushExpression(expr);
popExpression();
SILBuilder builder() {
return SILBuilder(currentPosition, SILDebugLocation(currentScope, currentExprLocation));
}
LoadInst *createLoadCopy(<load operands only>) const {
// Handle the copy...
builder().createLoad(<...>);
}
}
This solves another serious issue with the SILGenBuilder inheriting SILBuilder's API. We don't actually want code in SILGen accidentally calling the low-level builder, it's very important to know at the call site which API is being used. It's currently nearly impossible tell which API is being invoked at the call site making the SILGenBuilder implementation essentially unreadable.
The second category of code that arbitrarily reuses a SILBuilder to track the current insertion point and debug location is the most dangerous. If the reused SILBuilder is already part of another object's state, then there's actually no reason to reuse it.
For example, given code that currently looks like this:
void genNewInstruction(SILBuilder &builder, SILValue address) {
builder.createLoad(address);
}
class CodeGenState {
SILBuilder B;
void genCode(SILValue address) {
genNewInstruction(B, address)
B.setInsertionPoint(somewherElse);
}
};
The fact the the code is tracking and mutating the insertion point and debug location should be made explicit. That can be done with a convenient wrapper around the insertion point and debug location:
struct InsertionPoint {
SILBasicBlock::iterator insertPosition;
SILDebugLocation debugLocation;
// For convenience.
InsertionPoint(SILInstruction *inheritFrom);
};
void genNewInstruction(const SILBuilder &builder, SILValue address) {
builder.createLoad(address);
}
class CodeGenState {
InsertionPoint insertionPoint;
void genCode(SILValue address) {
genNewInstruction(SILBuilder(insertionPoint), address);
insertionPoint = InsertionPoint(somewhereElse);
}
};
If there is still enough code in the compiler that needs to manually adjust the insertion point and isn't already part of some other object's state, we could still have a mutable-insertion-point SILBuilder:
class SILBuilderPosition : SILBuilder {
public:
// Raw insertion point and debug location.
void setInsertionPoint(InsertionPoint &pt);
// Inherited insertion point and debug location.
void setInsertionPoint(SILInstruction *insertAfter);
};
Usage:
void genCode(SILBuilderPosition &buildAtPos, SILValue address) {
buildAtPos.createLoad(address);
buildAtPos.setInsertionPoint(somewhereElse);
}
There are two important points that the design should adhere to:
-
This "mutable position" SILBuilderPosition type cannot be the same type as the base SILBuilder because that would completely defeat the original motivation. We need to be able to isolate the code that is allowed to change the debug location and insertion point. i.e. If I'm editing a utility that that takes SILBuilder, I need to know whether I can change the location, or whether someone else might be doing it.
-
For sanity, the setInsertionPoint() semantics should be identical to just constructing a new SILBuilder with that position. Converting the code from this to a normal SILBuilder should be a purely mechanical (albeit time consuming) process.
Note that the exactly same thing could be also accomplished better by:
class SILBuilderPosition final : SILBuilder {
public:
SILBuilderPosition &operator =(const SILBuilderPosition &);
}
void genCode(SILBuilderPosition &builAtPos, SILValue address) {
buildAtPos.createLoad(address);
buildAtPos = SILBuilderPosition(newInsertionPoint);
}
The first SILBuilderPosition hack might just allow slightly easier migration. However, since it's likely no one will want to rewrite the vast amount of compiler code using SILBuilder, we will need to keep around a LegacySILBuilder with the old interface anyway for a while.
