IVGCVSW-3581 Fix AddCopyLayers and associated tests

Take a copy of the MemoryStrategies for a layer before inserting
new connections. Use the copy when looking up the original MemoryStrategies
during the graph transformation.

Fix the unit tests for AddCopyLayers to have cases where copies are needed.
Fix the validation for clarity and correctness - was previously comparing
Layers by pointer when it should have been by name (as it was comparing
with a cloned graph).

Change-Id: Ie282dc11913e977b8151ce1ad8bfba5e11617d40
Signed-off-by: Matthew Bentham <Matthew.Bentham@arm.com>

4 files changed, 47 insertions, 17 deletions

diff --git a/src/armnn/Graph.cpp b/src/armnn/Graph.cpp
index e521623737..9e00f5ec01 100644
--- a/src/armnn/Graph.cpp
+++ b/src/armnn/Graph.cpp
@@ -285,12 +285,13 @@ void Graph::AddCopyLayers(std::map<BackendId, std::unique_ptr<IBackendInternal>>
         {
             OutputSlot& srcOutputSlot = srcLayer->GetOutputSlot(srcOutputIndex);
             const std::vector<InputSlot*> srcConnections = srcOutputSlot.GetConnections();
+            const std::vector<MemoryStrategy> srcMemoryStrategies = srcOutputSlot.GetMemoryStrategies();
             for (unsigned int srcConnectionIndex = 0; srcConnectionIndex < srcConnections.size(); srcConnectionIndex++)
             {
                 InputSlot* dstInputSlot = srcConnections[srcConnectionIndex];
                 BOOST_ASSERT(dstInputSlot);
 
-                auto strategy = srcOutputSlot.GetMemoryStrategyForConnection(srcConnectionIndex);
+                MemoryStrategy strategy = srcMemoryStrategies[srcConnectionIndex];
                 BOOST_ASSERT_MSG(strategy != MemoryStrategy::Undefined,
                                  "Undefined memory strategy found while adding copy layers for compatibility");
 
@@ -339,8 +340,19 @@ void Graph::AddCopyLayers(std::map<BackendId, std::unique_ptr<IBackendInternal>>
                         copyOutputSlot.SetTensorHandleFactory(ITensorHandleFactory::LegacyFactoryId);
                     }
 
+                    // The output strategy of a copy layer is always DirectCompatibility.
                     copyOutputSlot.SetMemoryStrategy(0, MemoryStrategy::DirectCompatibility);
-                    srcOutputSlot.SetMemoryStrategy(srcConnectionIndex, MemoryStrategy::DirectCompatibility);
+
+                    // Recalculate the connection index on the previous layer as we have just inserted into it.
+                    const std::vector<InputSlot*>& newSourceConnections = srcOutputSlot.GetConnections();
+                    long newSrcConnectionIndex = std::distance(newSourceConnections.begin(),
+                                                               std::find(newSourceConnections.begin(),
+                                                                         newSourceConnections.end(),
+                                                                         &copyLayer->GetInputSlot(0)));
+
+                    // The input strategy of a copy layer is always DirectCompatibilty.
+                    srcOutputSlot.SetMemoryStrategy(boost::numeric_cast<unsigned int>(newSrcConnectionIndex),
+                                                    MemoryStrategy::DirectCompatibility);
                 }
             }
         }
diff --git a/src/armnn/Layer.hpp b/src/armnn/Layer.hpp
index 1ddbc00bc7..b90d040475 100644
--- a/src/armnn/Layer.hpp
+++ b/src/armnn/Layer.hpp
@@ -123,6 +123,7 @@ public:
     void Disconnect(InputSlot& slot);
 
     const std::vector<InputSlot*>& GetConnections() const { return m_Connections; }
+    const std::vector<MemoryStrategy>& GetMemoryStrategies() const { return m_MemoryStrategies; }
 
     bool ValidateTensorShape(const TensorShape& shape) const;
 
diff --git a/src/armnn/NetworkUtils.cpp b/src/armnn/NetworkUtils.cpp
index 66940e4eb5..a3760a9c6b 100644
--- a/src/armnn/NetworkUtils.cpp
+++ b/src/armnn/NetworkUtils.cpp
@@ -86,7 +86,7 @@ std::vector<DebugLayer*> InsertDebugLayerAfter(Graph& graph, Layer& layer)
     debugLayers.reserve(layer.GetNumOutputSlots());
 
     // Connect a DebugLayer to each output slot of the layer
-    for (auto&& outputSlot = layer.BeginOutputSlots(); outputSlot != layer.EndOutputSlots(); ++outputSlot)
+    for (auto outputSlot = layer.BeginOutputSlots(); outputSlot != layer.EndOutputSlots(); ++outputSlot)
     {
         const std::string debugName = std::string("DebugLayerAfter") + layer.GetNameStr();
 
@@ -94,6 +94,7 @@ std::vector<DebugLayer*> InsertDebugLayerAfter(Graph& graph, Layer& layer)
             graph.InsertNewLayer<DebugLayer>(*outputSlot, debugName.c_str());
 
         // Sets output tensor info for the debug layer.
+        BOOST_ASSERT(debugLayer->GetInputSlot(0).GetConnectedOutputSlot() == &(*outputSlot));
         TensorInfo debugInfo = debugLayer->GetInputSlot(0).GetConnectedOutputSlot()->GetTensorInfo();
 
         debugLayer->GetOutputSlot().SetTensorInfo(debugInfo);
diff --git a/src/armnn/test/GraphTests.cpp b/src/armnn/test/GraphTests.cpp
index ae5ff2232b..7950ec49f4 100644
--- a/src/armnn/test/GraphTests.cpp
+++ b/src/armnn/test/GraphTests.cpp
@@ -342,13 +342,14 @@ static void TestGraphAfterAddingCopyLayers(const armnn::Graph& graph, const armn
             if (srcLayer == nullptr || dstLayer == nullptr)
             {
                 BOOST_ERROR("At least one of the two ends of a new edge (" << edge.first << ", " << edge.second << ") "
-                            "introduced after adding copy layers to a graph correspond is not known to the graph");
+                            "introduced after adding copy layers to a graph "
+                            "correspond to a layer not known to the graph");
                 continue;
             }
 
             // One and only one of the two layers referenced by the edge should be present in the original graph.
-            const bool srcLayerInOrigGraph = GraphHasNamedLayer(origGraph, edge.first->GetNameStr());
-            const bool dstLayerInOrigGraph = GraphHasNamedLayer(origGraph, edge.second->GetNameStr());
+            const bool srcLayerInOrigGraph = GraphHasNamedLayer(origGraph, srcLayer->GetNameStr());
+            const bool dstLayerInOrigGraph = GraphHasNamedLayer(origGraph, dstLayer->GetNameStr());
 
             if (srcLayerInOrigGraph == dstLayerInOrigGraph)
             {
@@ -363,7 +364,7 @@ static void TestGraphAfterAddingCopyLayers(const armnn::Graph& graph, const armn
                 continue;
             }
 
-            const armnn::Layer* copyLayer = srcLayerInOrigGraph ? edge.second : edge.first;
+            const armnn::Layer* copyLayer = srcLayerInOrigGraph ? dstLayer : srcLayer;
             const armnn::Layer* nonCopyLayer = srcLayerInOrigGraph ? srcLayer : dstLayer;
 
             // Finds all edges connecting the copy layer to other layers.
@@ -412,10 +413,18 @@ static void TestGraphAfterAddingCopyLayers(const armnn::Graph& graph, const armn
 
                 // There must exist an edge connecting both layers directly in the original graph.
                 {
-                    const armnn::Layer* origEdgeN1 = srcLayerInOrigGraph ? nonCopyLayer : adjLayer;
-                    const armnn::Layer* origEdgeN2 = srcLayerInOrigGraph ? adjLayer : nonCopyLayer;
-                    auto origEdgeIter = std::find(origEdges.begin(), origEdges.end(),
-                        Edge(origEdgeN1, origEdgeN2));
+                    const armnn::Layer* origEdgeSrc = srcLayerInOrigGraph ? nonCopyLayer : adjLayer;
+                    const armnn::Layer* origEdgeDst = srcLayerInOrigGraph ? adjLayer : nonCopyLayer;
+
+                    auto origEdgeIter = origEdges.begin();
+                    for (; origEdgeIter != origEdges.end(); origEdgeIter++)
+                    {
+                        if (origEdgeIter->first->GetNameStr() == origEdgeSrc->GetNameStr() &&
+                            origEdgeIter->second->GetNameStr() == origEdgeDst->GetNameStr())
+                        {
+                            break;
+                        }
+                    }
 
                     if (origEdgeIter != origEdges.end())
                     {
@@ -439,6 +448,10 @@ struct CopyLayersFixture
 {
     CopyLayersFixture()
     {
+    }
+
+    void InitialiseTestGraph()
+    {
         using namespace armnn;
         using namespace std;
 
@@ -452,7 +465,7 @@ struct CopyLayersFixture
         inputLayer->GetOutputSlot(0).Connect(convLayer1->GetInputSlot(0));
 
         Layer* const convLayer2 = AddLayer<Convolution2dLayer>(convolutionDefaults, "conv2");
-        convLayer2->SetBackendId(Compute::CpuRef);
+        convLayer2->SetBackendId(Compute::CpuAcc);
 
         convLayer1->GetOutputSlot(0).Connect(convLayer2->GetInputSlot(0));
 
@@ -476,18 +489,19 @@ struct CopyLayersFixture
         actLayer->GetOutputSlot(0).Connect(softmaxLayer->GetInputSlot(0));
 
         Layer* const outputLayer = AddLayer<OutputLayer>(0, "output");
-        outputLayer->SetBackendId(armnn::Compute::CpuRef);
+        outputLayer->SetBackendId(armnn::Compute::CpuAcc);
 
         softmaxLayer->GetOutputSlot(0).Connect(outputLayer->GetInputSlot(0));
 
-        // Set the memory strategies
+        // Set the memory strategies - for this test should be DirectCompatibility for same backends,
+        // and CopyToTarget for different backends
         inputLayer->GetOutputSlot(0).SetMemoryStrategy(0, MemoryStrategy::DirectCompatibility);
-        convLayer1->GetOutputSlot(0).SetMemoryStrategy(0, MemoryStrategy::DirectCompatibility);
+        convLayer1->GetOutputSlot(0).SetMemoryStrategy(0, MemoryStrategy::CopyToTarget);
         convLayer1->GetOutputSlot(0).SetMemoryStrategy(1, MemoryStrategy::DirectCompatibility);
-        convLayer2->GetOutputSlot(0).SetMemoryStrategy(0, MemoryStrategy::DirectCompatibility);
+        convLayer2->GetOutputSlot(0).SetMemoryStrategy(0, MemoryStrategy::CopyToTarget);
         concatLayer->GetOutputSlot(0).SetMemoryStrategy(0, MemoryStrategy::DirectCompatibility);
         actLayer->GetOutputSlot(0).SetMemoryStrategy(0, MemoryStrategy::DirectCompatibility);
-        softmaxLayer->GetOutputSlot(0).SetMemoryStrategy(0, MemoryStrategy::DirectCompatibility);
+        softmaxLayer->GetOutputSlot(0).SetMemoryStrategy(0, MemoryStrategy::CopyToTarget);
     }
 
     armnn::TensorInfo m_TensorDesc;
@@ -513,6 +527,7 @@ private:
 
 BOOST_FIXTURE_TEST_CASE(AddCopyLayers, CopyLayersFixture)
 {
+    InitialiseTestGraph();
     const armnn::Graph origGraph(m_Graph);
     m_Graph.AddCopyLayers(m_Backends, m_FactoryRegistry);
 
@@ -521,6 +536,7 @@ BOOST_FIXTURE_TEST_CASE(AddCopyLayers, CopyLayersFixture)
 
 BOOST_FIXTURE_TEST_CASE(AddCopyLayersSeveralTimes, CopyLayersFixture)
 {
+    InitialiseTestGraph();
     m_Graph.AddCopyLayers(m_Backends, m_FactoryRegistry);
 
     // Calling AddCopyLayers() several times should not change the connections.