MM Rework for use by Kyle #2
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 30 commits
December 13, 2014 09:57
This results in 211 regressions.
…n the right place. I stumbled across this long-standing bug after disabling returnRecordsByRefArguments(). Apparently that routine covered at least some cases that must be handled by returnStartTuplesByRefArgs() when it is disabled. If there is an early return in the routine, it will jump to the exit label. As written, returnStarTuplesByRefArgs() was putting the write-back to the return variable before the exit label. This works fine for the straight-line path, but fails to write out the result when the early return fires. This results in the caller using uninitialized memory, which is generally bad. The fix is a one-liner. The call to insertBeforeReturn() that inserts the write-back was changed to insertBeforeReturnAfterLabel(). That puts the write-back in the right place.
This still completes with 50 regressions, so has no impact on the output of the compiler.
It may be possible to remove some of these calls after the optimized insertion of
autoDestroys is working, so this lets me track those places. The TODOs are flagged with
TODO AMM for "Automatic Memory Management".
diff --git a/compiler/AST/build.cpp b/compiler/AST/build.cpp
index 87d86ee..53e9829 100644
--- a/compiler/AST/build.cpp
+++ b/compiler/AST/build.cpp
@@ -664,6 +664,7 @@ handleArrayTypeCase(FnSymbol* fn, Expr* indices, Expr* iteratorExpr, Expr* expr)
thenStmt->insertAtTail(new DefExpr(domain));
// note that we need the below autoCopy until we start reference
// counting domains within runtime array types
+ // TODO: Check if the explicit insertion of an autoCopy is necessary here.
thenStmt->insertAtTail(new CallExpr(PRIM_MOVE, domain,
new CallExpr("chpl__autoCopy",
new CallExpr("chpl__ensureDomainExpr",
diff --git a/compiler/passes/normalize.cpp b/compiler/passes/normalize.cpp
index 0e9f207..ef8f12c 100644
--- a/compiler/passes/normalize.cpp
+++ b/compiler/passes/normalize.cpp
@@ -949,11 +949,15 @@ static void init_array_alias(VarSymbol* var, Expr* type, Expr* init, Expr* stmt)
if (!type) {
partial = new CallExpr("newAlias", gMethodToken, init->remove());
// newAlias is not a method, so we don't set the methodTag
+ // TODO: newAlias should return a constructed object, which would make
+ // this autoCopy redundant.
stmt->insertAfter(new CallExpr(PRIM_MOVE, var, new CallExpr("chpl__autoCopy", partial)));
} else {
partial = new CallExpr("reindex", gMethodToken, init->remove());
partial->partialTag = true;
partial->methodTag = true;
+ // TODO: Does reindex return a constructed object? If so, the autoCopy
+ // call is redundant.
stmt->insertAfter(new CallExpr(PRIM_MOVE, var, new CallExpr("chpl__autoCopy", new CallExpr(partial, type->remove()))));
}
}
diff --git a/compiler/resolution/functionResolution.cpp b/compiler/resolution/functionResolution.cpp
index 6fb3507..316b150 100644
--- a/compiler/resolution/functionResolution.cpp
+++ b/compiler/resolution/functionResolution.cpp
@@ -2945,6 +2945,8 @@ static void setFlagsForConstAccess(CallExpr* call, FnSymbol* resolvedFn)
// Report an error when storing a sync or single variable into a tuple.
// This is because currently we deallocate memory excessively in this case.
+// TODO: Review and see if this is still needed after comprehensive automatic
+// memory management is in place.
static void checkForStoringIntoTuple(CallExpr* call, FnSymbol* resolvedFn)
{
// Do not perform the checks if:
@@ -3751,8 +3753,9 @@ insertFormalTemps(FnSymbol* fn) {
// The copy in is needed for "inout", "in" and "const in" intents.
// The copy out is needed for "inout" and "out" intents.
// Blank intent is treated like "const", and normally copies the formal through
-// chpl__autoCopy.
+// chpl__autoCopy. (Check that this is still true.)
// Note that autoCopy is called in this case, but not for "inout", "in" and "const in".
+// TODO: Check that this special behavior is correct and necessary.
// Either record-wrapped types are always passed by ref, or some unexpected
// behavior will result by applying "in" intents to them.
static void addLocalCopiesAndWritebacks(FnSymbol* fn, SymbolMap& formals2vars)
@@ -3835,6 +3838,7 @@ static void addLocalCopiesAndWritebacks(FnSymbol* fn, SymbolMap& formals2vars)
!ts->hasFlag(FLAG_ITERATOR_CLASS) &&
!ts->hasFlag(FLAG_ITERATOR_RECORD) &&
!getSyncFlags(ts).any()) {
+ // Todo, remove this special case and let parallel handle it.
if (fn->hasFlag(FLAG_BEGIN)) {
// autoCopy/autoDestroy will be added later, in parallel pass
// by insertAutoCopyDestroyForTaskArg()
@@ -5313,6 +5317,8 @@ insertValueTemp(Expr* insertPoint, Expr* actual) {
// Currently, FLAG_DONOR_FN is only relevant when placed on
// chpl__autoCopy().
//
+// TODO: Would like to simplify this, so it returns true if the function
+// returns a record or tuple and false otherwise.
FnSymbol*
requiresImplicitDestroy(CallExpr* call) {
if (FnSymbol* fn = call->isResolved()) {
@@ -7857,8 +7863,9 @@ static void replaceInitPrims(Vec<BaseAST*>& asts)
VarSymbol* tmp = newTemp("_runtime_type_tmp_", runtimeTypeToValueFn->retType);
call->getStmtExpr()->insertBefore(new DefExpr(tmp));
call->getStmtExpr()->insertBefore(new CallExpr(PRIM_MOVE, tmp, runtimeTypeToValueCall));
- INT_ASSERT(autoCopyMap.get(tmp->type));
- call->replace(new CallExpr(autoCopyMap.get(tmp->type), tmp));
+ FnSymbol* autoCopy = autoCopyMap.get(tmp->type);
+ INT_ASSERT(autoCopy);
+ call->replace(new CallExpr(autoCopy, tmp));
} else if (rt->symbol->hasFlag(FLAG_HAS_RUNTIME_TYPE)) {
//
// This is probably related to a comment that used to handle
diff --git a/compiler/resolution/wrappers.cpp b/compiler/resolution/wrappers.cpp
index f54766e..2025c27 100644
--- a/compiler/resolution/wrappers.cpp
+++ b/compiler/resolution/wrappers.cpp
@@ -282,6 +282,9 @@ buildDefaultWrapper(FnSymbol* fn,
{
Symbol* copyTemp = newTemp("wrap_arg");
wrapper->insertAtTail(new DefExpr(copyTemp));
+ // TODO: Insertion of this autocopy can probably be left to
+ // ownership flow analysis. Since the LHS is a structure member,
+ // the copy is required, but only if the RHS object is not owned.
wrapper->insertAtTail(new CallExpr(PRIM_MOVE, copyTemp, new CallExpr("chpl__autoCopy", temp)));
wrapper->insertAtTail(
new CallExpr(PRIM_SET_MEMBER, wrapper->_this,
Also, use size_t in place of int, for closer compatibility with std::bitset and the boost dynamic_bitset. Added a copy-constructor and the copy operation to support + (set union) and - (set difference). Updated bb.cpp, copyPropagation.cpp and loopInvariantCodeMotion.cpp, clients of BitVec, because now the size it returns and the ndata field are unsigned (size_t).
Added filters in extractSymbols() to ignore symbols that are not VarSymbols or ArgSymbols, extern symbols and symbols not of a record type. Filtering out symbols of class type also filters out references. Maybe this is too clever, but at least fits well with the style of programming currently in place. If the representation of reference types changes in the future, this is one place where the code will have to be amended. At least that case is covered by a helpful comment. Moved the initialization of the alias table up into extractSymbols() because the expectation that each symbol is initialized before being copied (or copied into) through a move is currently false. Split off the isConstructor() test as a predicate. Added a test in processMove(), so we can flag when an uninitialized symbol is copied. This would probably result in a large number of false positives before flow analysis is carried out, because each block starts with an empty gen set. We now filter by the members of the symbolIndex, so we ignore symbols that are not local (including module-level and extern symbols) and not variables or arguments. Had to add a test to skip degenerate basic blocks. Why do we still have degenerate basic blocks? We skip functions that are prototypes (including extern functions) altogether. Re-enabled insertGlobalAutoDestroyCalls() and activated the new insertAutoDestroyCalls() mini-pass.
If alias lists are the same, do not merge them again. The RHS symbol of a move is not necessarily a local symbol. It can be a module-level or extern symbol. Now down to 55 regresssions.
Down to 46 regressions.
I thought I would need these but I don't. But this follows the "single point of definition" ROT, so I kept it.
Added a backward flow pass to propagate disharge-of-ownership constraints from IN sets back across the gap to OUT sets. The OUT sets of terminal blocks are set to all zeroes to force all locals to be autodestroyed on exit. Added code so that we only add an autoDestroy for one member of an alias clique.
Down to 1475 regressions.
Rewrite header comments for improved accuracy. Factor out AliasVectorMap manipulations into a class. Factor out common code between gen/kill set creation and autoCopy insertion. Move predicates up toward top of file. Renamed predicates to better indicate how they relate to ownership rather than toward code structure. Moved autoCopy insertion pass up, right after forward flow analysis. [Tested with hello.chpl]
…re inserted. In isConsumed(), added cases for PRIM_MOVE to a global and PRIM_SET_MEMBER. Use temporaries to store the result of an autocopy call, to keep returnStarTuplesByRefArgs happy. 1361 failures. W00t!
…reference counted types. Some tests involving internally ref-counted types were failing due to the reference count going negative. This turned out to be due to record-wrapped types (the same set) being inserted into tuples without their copy-constructor (autoCopy) being called. The destructor for a tuple calls autoDestroy on its elements, so these autoCopy calls are required. It is probably the case that destructor calls were omitted for record-wrapped elements in a tuple. But since I'm coding the insertion of destructor calls from scratch and trying to keep the implementation as uniform as possible, tuple destructors contain destructors for all of their record elements, and are called wherever a tuple local needs to be destroyed. Adding more autoCopies exposed that we may have RWT objects that are empty (i.e. contain a nil class object) and autoCopy should be robust to that. 291 failures remaining
…RHS is global. Removed the commented-out clause that prevented autoCopy insertion for ref-counted types (no change in functionality). Updated some test output because the copy constructor is called for members of tuples much more often now. In users/ferguson/refcnt, the output changed because autodestroy calls have been moved closer to the end of the routine. This is good, because it shows that in the new implementation there are more opportunities for an owned variable to be consumed before it goes out of scope.
This results in 211 regressions.
…n the right place. I stumbled across this long-standing bug after disabling returnRecordsByRefArguments(). Apparently that routine covered at least some cases that must be handled by returnStartTuplesByRefArgs() when it is disabled. If there is an early return in the routine, it will jump to the exit label. As written, returnStarTuplesByRefArgs() was putting the write-back to the return variable before the exit label. This works fine for the straight-line path, but fails to write out the result when the early return fires. This results in the caller using uninitialized memory, which is generally bad. The fix is a one-liner. The call to insertBeforeReturn() that inserts the write-back was changed to insertBeforeReturnAfterLabel(). That puts the write-back in the right place.
This still completes with 50 regressions, so has no impact on the output of the compiler.
It may be possible to remove some of these calls after the optimized insertion of
autoDestroys is working, so this lets me track those places. The TODOs are flagged with
TODO AMM for "Automatic Memory Management".
diff --git a/compiler/AST/build.cpp b/compiler/AST/build.cpp
index 87d86ee..53e9829 100644
--- a/compiler/AST/build.cpp
+++ b/compiler/AST/build.cpp
@@ -664,6 +664,7 @@ handleArrayTypeCase(FnSymbol* fn, Expr* indices, Expr* iteratorExpr, Expr* expr)
thenStmt->insertAtTail(new DefExpr(domain));
// note that we need the below autoCopy until we start reference
// counting domains within runtime array types
+ // TODO: Check if the explicit insertion of an autoCopy is necessary here.
thenStmt->insertAtTail(new CallExpr(PRIM_MOVE, domain,
new CallExpr("chpl__autoCopy",
new CallExpr("chpl__ensureDomainExpr",
diff --git a/compiler/passes/normalize.cpp b/compiler/passes/normalize.cpp
index 0e9f207..ef8f12c 100644
--- a/compiler/passes/normalize.cpp
+++ b/compiler/passes/normalize.cpp
@@ -949,11 +949,15 @@ static void init_array_alias(VarSymbol* var, Expr* type, Expr* init, Expr* stmt)
if (!type) {
partial = new CallExpr("newAlias", gMethodToken, init->remove());
// newAlias is not a method, so we don't set the methodTag
+ // TODO: newAlias should return a constructed object, which would make
+ // this autoCopy redundant.
stmt->insertAfter(new CallExpr(PRIM_MOVE, var, new CallExpr("chpl__autoCopy", partial)));
} else {
partial = new CallExpr("reindex", gMethodToken, init->remove());
partial->partialTag = true;
partial->methodTag = true;
+ // TODO: Does reindex return a constructed object? If so, the autoCopy
+ // call is redundant.
stmt->insertAfter(new CallExpr(PRIM_MOVE, var, new CallExpr("chpl__autoCopy", new CallExpr(partial, type->remove()))));
}
}
diff --git a/compiler/resolution/functionResolution.cpp b/compiler/resolution/functionResolution.cpp
index 6fb3507..316b150 100644
--- a/compiler/resolution/functionResolution.cpp
+++ b/compiler/resolution/functionResolution.cpp
@@ -2945,6 +2945,8 @@ static void setFlagsForConstAccess(CallExpr* call, FnSymbol* resolvedFn)
// Report an error when storing a sync or single variable into a tuple.
// This is because currently we deallocate memory excessively in this case.
+// TODO: Review and see if this is still needed after comprehensive automatic
+// memory management is in place.
static void checkForStoringIntoTuple(CallExpr* call, FnSymbol* resolvedFn)
{
// Do not perform the checks if:
@@ -3751,8 +3753,9 @@ insertFormalTemps(FnSymbol* fn) {
// The copy in is needed for "inout", "in" and "const in" intents.
// The copy out is needed for "inout" and "out" intents.
// Blank intent is treated like "const", and normally copies the formal through
-// chpl__autoCopy.
+// chpl__autoCopy. (Check that this is still true.)
// Note that autoCopy is called in this case, but not for "inout", "in" and "const in".
+// TODO: Check that this special behavior is correct and necessary.
// Either record-wrapped types are always passed by ref, or some unexpected
// behavior will result by applying "in" intents to them.
static void addLocalCopiesAndWritebacks(FnSymbol* fn, SymbolMap& formals2vars)
@@ -3835,6 +3838,7 @@ static void addLocalCopiesAndWritebacks(FnSymbol* fn, SymbolMap& formals2vars)
!ts->hasFlag(FLAG_ITERATOR_CLASS) &&
!ts->hasFlag(FLAG_ITERATOR_RECORD) &&
!getSyncFlags(ts).any()) {
+ // Todo, remove this special case and let parallel handle it.
if (fn->hasFlag(FLAG_BEGIN)) {
// autoCopy/autoDestroy will be added later, in parallel pass
// by insertAutoCopyDestroyForTaskArg()
@@ -5313,6 +5317,8 @@ insertValueTemp(Expr* insertPoint, Expr* actual) {
// Currently, FLAG_DONOR_FN is only relevant when placed on
// chpl__autoCopy().
//
+// TODO: Would like to simplify this, so it returns true if the function
+// returns a record or tuple and false otherwise.
FnSymbol*
requiresImplicitDestroy(CallExpr* call) {
if (FnSymbol* fn = call->isResolved()) {
@@ -7857,8 +7863,9 @@ static void replaceInitPrims(Vec<BaseAST*>& asts)
VarSymbol* tmp = newTemp("_runtime_type_tmp_", runtimeTypeToValueFn->retType);
call->getStmtExpr()->insertBefore(new DefExpr(tmp));
call->getStmtExpr()->insertBefore(new CallExpr(PRIM_MOVE, tmp, runtimeTypeToValueCall));
- INT_ASSERT(autoCopyMap.get(tmp->type));
- call->replace(new CallExpr(autoCopyMap.get(tmp->type), tmp));
+ FnSymbol* autoCopy = autoCopyMap.get(tmp->type);
+ INT_ASSERT(autoCopy);
+ call->replace(new CallExpr(autoCopy, tmp));
} else if (rt->symbol->hasFlag(FLAG_HAS_RUNTIME_TYPE)) {
//
// This is probably related to a comment that used to handle
diff --git a/compiler/resolution/wrappers.cpp b/compiler/resolution/wrappers.cpp
index f54766e..2025c27 100644
--- a/compiler/resolution/wrappers.cpp
+++ b/compiler/resolution/wrappers.cpp
@@ -282,6 +282,9 @@ buildDefaultWrapper(FnSymbol* fn,
{
Symbol* copyTemp = newTemp("wrap_arg");
wrapper->insertAtTail(new DefExpr(copyTemp));
+ // TODO: Insertion of this autocopy can probably be left to
+ // ownership flow analysis. Since the LHS is a structure member,
+ // the copy is required, but only if the RHS object is not owned.
wrapper->insertAtTail(new CallExpr(PRIM_MOVE, copyTemp, new CallExpr("chpl__autoCopy", temp)));
wrapper->insertAtTail(
new CallExpr(PRIM_SET_MEMBER, wrapper->_this,
Added filters in extractSymbols() to ignore symbols that are not VarSymbols or ArgSymbols, extern symbols and symbols not of a record type. Filtering out symbols of class type also filters out references. Maybe this is too clever, but at least fits well with the style of programming currently in place. If the representation of reference types changes in the future, this is one place where the code will have to be amended. At least that case is covered by a helpful comment. Moved the initialization of the alias table up into extractSymbols() because the expectation that each symbol is initialized before being copied (or copied into) through a move is currently false. Split off the isConstructor() test as a predicate. Added a test in processMove(), so we can flag when an uninitialized symbol is copied. This would probably result in a large number of false positives before flow analysis is carried out, because each block starts with an empty gen set. We now filter by the members of the symbolIndex, so we ignore symbols that are not local (including module-level and extern symbols) and not variables or arguments. Had to add a test to skip degenerate basic blocks. Why do we still have degenerate basic blocks? We skip functions that are prototypes (including extern functions) altogether. Re-enabled insertGlobalAutoDestroyCalls() and activated the new insertAutoDestroyCalls() mini-pass.
If alias lists are the same, do not merge them again. The RHS symbol of a move is not necessarily a local symbol. It can be a module-level or extern symbol. Now down to 55 regresssions.
Down to 46 regressions.
added 24 commits
January 8, 2015 09:32
I thought I would need these but I don't. But this follows the "single point of definition" ROT, so I kept it.
Added a backward flow pass to propagate disharge-of-ownership constraints from IN sets back across the gap to OUT sets. The OUT sets of terminal blocks are set to all zeroes to force all locals to be autodestroyed on exit. Added code so that we only add an autoDestroy for one member of an alias clique.
Down to 1475 regressions.
Rewrite header comments for improved accuracy. Factor out AliasVectorMap manipulations into a class. Factor out common code between gen/kill set creation and autoCopy insertion. Move predicates up toward top of file. Renamed predicates to better indicate how they relate to ownership rather than toward code structure. Moved autoCopy insertion pass up, right after forward flow analysis. [Tested with hello.chpl]
…re inserted. In isConsumed(), added cases for PRIM_MOVE to a global and PRIM_SET_MEMBER. Use temporaries to store the result of an autocopy call, to keep returnStarTuplesByRefArgs happy. 1361 failures. W00t!
…reference counted types. Some tests involving internally ref-counted types were failing due to the reference count going negative. This turned out to be due to record-wrapped types (the same set) being inserted into tuples without their copy-constructor (autoCopy) being called. The destructor for a tuple calls autoDestroy on its elements, so these autoCopy calls are required. It is probably the case that destructor calls were omitted for record-wrapped elements in a tuple. But since I'm coding the insertion of destructor calls from scratch and trying to keep the implementation as uniform as possible, tuple destructors contain destructors for all of their record elements, and are called wherever a tuple local needs to be destroyed. Adding more autoCopies exposed that we may have RWT objects that are empty (i.e. contain a nil class object) and autoCopy should be robust to that. 291 failures remaining
…RHS is global. Removed the commented-out clause that prevented autoCopy insertion for ref-counted types (no change in functionality). Updated some test output because the copy constructor is called for members of tuples much more often now. In users/ferguson/refcnt, the output changed because autodestroy calls have been moved closer to the end of the routine. This is good, because it shows that in the new implementation there are more opportunities for an owned variable to be consumed before it goes out of scope.
Add some debug prinout clauses. Down to 62 regressions.
Conflicts: compiler/adt/bitVec.cpp compiler/include/bitVec.h compiler/passes/insertAutoCopyAutoDestroy.cpp Converted debug sections into asserts and inserted additional asserts.
52 regressions remaining (all execution-time)
… that return owned values. Down to 50 regressions.
…ds being autoCopied now.
Added cleanup traversal to remove empty blocks. (Also control verification with the fVerify flag now.) Revised the verify function to lock this behavior in. The removal of empty blocks cannot be done until BB analysis is complete, because some of the threading is delayed. We have to wait for threading to finish before we can identify blocks that have no predecessor. Updated the signature of initInSets in copyPropagation.cpp.
…ains is nil. Also, update test/types/records/sungeun/classInRecord.chpl and test/types/records/sungeun/destructor3.good because there are different numbers of calls to chpl__autoCopy and chpl__autoDestroy.
… autoCopy callse. This test basically devolves to a test on whether the members of a tuple are passed by reference or value. In the current version of the grand autoCopy/autoDestroy rework, a tuple owns and destroys record members that are passed by value. The output of this test is consistent with that definition. In the previous implementation, MM in tuples was something of a hybrid. (Record) values copied when the fields in a tuple were created and destroyed when done, but (inconsistently) the members of a tuple are not copied when a tuple is assigned or when one of its members is read out. This is the root cause of the return-record bug this work is intended to fix. However, this solution is not necessarily permanent. It may be possible to avoid these copies by handling tuples specially. In that special implementation, tuple members would always be treated as references. But if a member of a tuple is used to determine the type of a result, members that were originally values (i.e. not references) would have value and not reference type. There are some semantical issues related to this choice that may need to be worked through (or experimented with). Down to 43 regressions.
Down to 45 regressions.
Nested calls to initializeClass() result in the creation of temporaries which are then piped through chpl__autoCopy to initialize fields in the containing record or class. When the field type is an extern type, the initializeClass() does nothing. This can be bad in cases where an initial value (such as nil) is expected due to the class (or record) invariants. It turns out that we don't need initializeClass() anyway, since it is used only in specializations of the compiler-supplied all-fields constructor. These specializations should ensure that all fields are initialized, so code generated by initializeClass() should be moot. (Testing indicates that this is the case, but I have not yet extensively studied the generated code.) I tried earlier to remove the PRIM_INIT_FIELDS primitive entirely, but apparently it is still needed to establish the type of _this within the specialized all-fields constructor. I reverted the change to IO.chpl, since it did not actually resolve the problem. Down to 30 regressions.
Too many autodestroy calls were being inserted because one temp contributing to an alias clique might lie along one path while another lies along a distinct path. Backward flow analysis then considers them both to be dead upon entry to the join node. The constraint that each of these temps be dead on exit from their respective blocks causes autodestroy calls to be inserted. The temps themselves are dead as required, but the other members of the clique get deleted (because they are all aliases) even though their respective bits in the OUT set may be true. This mismatch causes us to get the wrong answer, assuming that one of those other clique members is alive when it is actually dead. To address this, I moved the population of the alias map out to a separate traversal. Now, any manipulation of the flow sets affects all members of an alias clique in concert, whether or not their equivalence has yet been established (through a bitwise copy). The result is that this mode of premature deletion has been eliminated. This change allows the insertion of autoDestroys for internally reference-counted types to be enabled. Doing so, only cause one additional regression to appear. Now at 31 regressions.
Kyle-B
pushed a commit
that referenced
this pull request
Sep 18, 2015
Artifact from the past. (back in Web Docs Iteration #2 - internal repo)
Kyle-B
pushed a commit
that referenced
this pull request
Sep 18, 2015
Artifact from the past. (back in Web Docs Iteration #2 - internal repo)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This should make your branch compatible with the SHA1 72bfb0e I sent to you yesterday.