Skip to content

Commit

Permalink
DYN-6527: Fix graph update for primitive input nodes that are first i…
Browse files Browse the repository at this point in the history
…nitialized to null (DynamoDS#14703)

* remove coreclr-ncalc references

* add failing test for dropdown node

* cleanup

* update tests

* attempt initial fix

* cleanup

* update test

* review comments

* add code comments
  • Loading branch information
aparajit-pratap committed Mar 14, 2024
1 parent 8f43cf7 commit c298d21
Show file tree
Hide file tree
Showing 5 changed files with 115 additions and 53 deletions.
58 changes: 8 additions & 50 deletions src/Engine/ProtoAssociative/CodeGen_SSA.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using ProtoCore.AST.AssociativeAST;
using ProtoCore.AST.AssociativeAST;
using ProtoCore.DSASM;
using ProtoCore.Utils;
using System.Collections.Generic;
Expand Down Expand Up @@ -275,7 +275,11 @@ private List<AssociativeNode> BuildSSA(List<AssociativeNode> astList, ProtoCore.
BinaryExpressionNode bnode = (node as BinaryExpressionNode);
int generatedUID = ProtoCore.DSASM.Constants.kInvalidIndex;

if (context.applySSATransform && core.Options.GenerateSSA)
// Skip SSA for input ASTs that are first assigned null such as this:
// a = null; (update "a") => a = <some primitive>;
// SSA would break up the null AST into two assignments, which then breaks update:
// a = null; (SSA) => temp = null; a = temp;
if (context.applySSATransform && core.Options.GenerateSSA && !bnode.IsInputExpression)
{
int ssaID = ProtoCore.DSASM.Constants.kInvalidIndex;
string name = ProtoCore.Utils.CoreUtils.GenerateIdentListNameString(bnode.LeftNode);
Expand Down Expand Up @@ -382,53 +386,6 @@ private List<AssociativeNode> BuildSSA(List<AssociativeNode> astList, ProtoCore.
return astList;
}

private void DfsSSAIeentList(AssociativeNode node, ref Stack<AssociativeNode> ssaStack, ref List<AssociativeNode> astlist)
{
if (node is IdentifierListNode)
{
IdentifierListNode listNode = node as IdentifierListNode;

bool isSingleDot = !(listNode.LeftNode is IdentifierListNode) && !(listNode.RightNode is IdentifierListNode);
if (isSingleDot)
{
BinaryExpressionNode bnode = BuildSSAIdentListAssignmentNode(listNode);
astlist.Add(bnode);
ssaStack.Push(bnode);
}
else
{
DfsSSAIeentList(listNode.LeftNode, ref ssaStack, ref astlist);

IdentifierListNode newListNode = node as IdentifierListNode;
newListNode.Optr = Operator.dot;

AssociativeNode leftnode = ssaStack.Pop();
Validity.Assert(leftnode is BinaryExpressionNode);

newListNode.LeftNode = (leftnode as BinaryExpressionNode).LeftNode;
newListNode.RightNode = listNode.RightNode;

BinaryExpressionNode bnode = BuildSSAIdentListAssignmentNode(newListNode);
astlist.Add(bnode);
ssaStack.Push(bnode);

}
}
else if (node is FunctionCallNode)
{
FunctionCallNode fcNode = node as FunctionCallNode;
for (int idx = 0; idx < fcNode.FormalArguments.Count; idx++)
{
AssociativeNode arg = fcNode.FormalArguments[idx];

Stack<AssociativeNode> ssaStack1 = new Stack<AssociativeNode>();
DFSEmitSSA_AST(arg, ssaStack1, ref astlist);
AssociativeNode argNode = ssaStack.Pop();
fcNode.FormalArguments[idx] = argNode is BinaryExpressionNode ? (argNode as BinaryExpressionNode).LeftNode : argNode;
}
}
}

private void DFSEmitSSA_AST(AssociativeNode node, Stack<AssociativeNode> ssaStack, ref List<AssociativeNode> astlist)
{
Validity.Assert(null != astlist && null != ssaStack);
Expand Down Expand Up @@ -488,7 +445,8 @@ private void DFSEmitSSA_AST(AssociativeNode node, Stack<AssociativeNode> ssaStac

var bnode = AstFactory.BuildAssignment(leftNode, rightNode);
bnode.isSSAAssignment = isSSAAssignment;
bnode.IsInputExpression = astBNode.IsInputExpression;
// TODO: SSA is not called for ASTs that are input expressions. Revisit this if there are any issues.
// bnode.IsInputExpression = astBNode.IsInputExpression;

astlist.Add(bnode);
ssaStack.Push(bnode);
Expand Down
3 changes: 2 additions & 1 deletion src/Engine/ProtoCore/DSASM/InstructionSet.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;
using System.Collections.Generic;
using System.Linq;
using ProtoCore.Exceptions;
Expand All @@ -10,6 +10,7 @@ namespace ProtoCore.DSASM
public enum Registers
{
RX,
// Register used to temporarily store primitive values for graph update cycles.
LX,
}

Expand Down
2 changes: 1 addition & 1 deletion src/Libraries/CoreNodeModels/Enum.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
Expand Down
53 changes: 53 additions & 0 deletions test/DynamoCoreWpfTests/NodeExecutionUITest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -328,5 +328,58 @@ public void TestSelectionNodeUpdate2()
AssertPreviewValue(tsn.GUID.ToString(), 0);

}

[Test]
public void TestDropdownNodeUpdate()
{
var model = GetModel();
var tdd = new TestDropdown
{
SelectedIndex = 0
};

var command = new DynamoModel.CreateNodeCommand(tdd, 0, 0, true, false);
model.ExecuteCommand(command);

AssertPreviewValue(tdd.GUID.ToString(), "one");

tdd.SelectedIndex = 1;
tdd.OnNodeModified();

AssertPreviewValue(tdd.GUID.ToString(), "two");

tdd.SelectedIndex = 2;
tdd.OnNodeModified();

AssertPreviewValue(tdd.GUID.ToString(), "three");

}

[Test]
public void TestDropdownNodeUpdate1()
{
var model = GetModel();
var tdd = new TestDropdown();

var command = new DynamoModel.CreateNodeCommand(tdd, 0, 0, true, false);
model.ExecuteCommand(command);

AssertPreviewValue(tdd.GUID.ToString(), null);

tdd.SelectedIndex = 0;
tdd.OnNodeModified();

AssertPreviewValue(tdd.GUID.ToString(), "one");

tdd.SelectedIndex = 1;
tdd.OnNodeModified();

AssertPreviewValue(tdd.GUID.ToString(), "two");

tdd.SelectedIndex = 2;
tdd.OnNodeModified();

AssertPreviewValue(tdd.GUID.ToString(), "three");
}
}
}
52 changes: 51 additions & 1 deletion test/TestUINodes/TestUINodes.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;
using System.Collections;
using System.Collections.Generic;
using System.IO;
Expand Down Expand Up @@ -140,4 +140,54 @@ protected override int GetModelObjectFromIdentifer(string id)
return id.Length;
}
}

[NodeName("Test Dropdown Node")]
[NodeCategory("TestUINodes")]
[NodeDescription("test dropdown node")]
[OutPortTypes("string")]
[IsDesignScriptCompatible]
[IsVisibleInDynamoLibrary(false)]
public class TestDropdown : DSDropDownBase
{
public TestDropdown() : base("TestDropdown") { }


public override IEnumerable<AssociativeNode> BuildOutputAst(List<AssociativeNode> inputAstNodes)
{
AssociativeNode node;
if (SelectedIndex < 0 || SelectedIndex >= Items.Count)
{
node = AstFactory.BuildNullNode();
return new[] { AstFactory.BuildAssignment(GetAstIdentifierForOutputIndex(0), node) };
}
else
{
// get the selected items name
var stringNode = AstFactory.BuildStringNode((string)Items[SelectedIndex].Name);

// assign the selected name to an actual enumeration value
var assign = AstFactory.BuildAssignment(GetAstIdentifierForOutputIndex(0), stringNode);

// return the enumeration value
return new List<AssociativeNode> { assign };
}
}

protected override SelectionState PopulateItemsCore(string currentSelection)
{
Items.Clear();

var symbols = new[] { "one", "two", "three" };


foreach (var symbol in symbols)
{

Items.Add(new DynamoDropDownItem(symbol, symbol));
}

return SelectionState.Restore;
}

}
}

0 comments on commit c298d21

Please sign in to comment.