Hello Andreas Sandberg,

I'd like you to do a code review. Please visit

    https://gem5-review.googlesource.com/9781

to review the following change.


Change subject: sim,cpu,mem,arch: Introduced MasterInfo data structure
......................................................................

sim,cpu,mem,arch: Introduced MasterInfo data structure

With this patch a gem5 System will store more info about its Masters.
While it was previously keeping track of the Master name and Master ID
only, it is now adding a per-Master pointer to the SimObject related to
the Master.
This will make it possible for a client to query a System for a Master
using either the master's name or the master's pointer.

Change-Id: I8b97d328a65cd06f329e2cdd3679451c17d2b8f6
Signed-off-by: Giacomo Travaglini <giacomo.travagl...@arm.com>
Reviewed-by: Andreas Sandberg <andreas.sandb...@arm.com>
---
M src/arch/arm/stage2_mmu.cc
M src/arch/x86/pagetable_walker.hh
M src/cpu/base.cc
M src/cpu/checker/cpu.cc
M src/cpu/testers/directedtest/DirectedGenerator.cc
M src/cpu/testers/garnet_synthetic_traffic/GarnetSyntheticTraffic.cc
M src/cpu/testers/memtest/memtest.cc
M src/cpu/testers/rubytest/RubyTester.cc
M src/cpu/testers/traffic_gen/traffic_gen.cc
M src/cpu/trace/trace_cpu.cc
M src/dev/dma_device.cc
M src/gpu-compute/compute_unit.cc
M src/gpu-compute/dispatcher.cc
M src/mem/cache/prefetch/base.cc
M src/mem/external_master.cc
A src/mem/mem_master.hh
M src/mem/ruby/slicc_interface/AbstractController.cc
M src/sim/system.cc
M src/sim/system.hh
19 files changed, 176 insertions(+), 40 deletions(-)



diff --git a/src/arch/arm/stage2_mmu.cc b/src/arch/arm/stage2_mmu.cc
index 5c28d07..ba820e3 100644
--- a/src/arch/arm/stage2_mmu.cc
+++ b/src/arch/arm/stage2_mmu.cc
@@ -51,7 +51,7 @@
 Stage2MMU::Stage2MMU(const Params *p)
     : SimObject(p), _stage1Tlb(p->tlb), _stage2Tlb(p->stage2_tlb),
       port(_stage1Tlb->getTableWalker(), p->sys),
-      masterId(p->sys->getMasterId(_stage1Tlb->getTableWalker()->name()))
+      masterId(p->sys->getMasterId(_stage1Tlb->getTableWalker()))
 {
     // we use the stage-one table walker as the parent of the port,
     // and to get our master id, this is done to keep things
diff --git a/src/arch/x86/pagetable_walker.hh b/src/arch/x86/pagetable_walker.hh
index d71db7e..d5aa631 100644
--- a/src/arch/x86/pagetable_walker.hh
+++ b/src/arch/x86/pagetable_walker.hh
@@ -203,7 +203,7 @@
         Walker(const Params *params) :
             MemObject(params), port(name() + ".port", this),
funcState(this, NULL, NULL, true), tlb(NULL), sys(params->system),
-            masterId(sys->getMasterId(name())),
+            masterId(sys->getMasterId(this)),
             numSquashable(params->num_squash_per_cycle),
             startWalkWrapperEvent([this]{ startWalkWrapper(); }, name())
         {
diff --git a/src/cpu/base.cc b/src/cpu/base.cc
index 4fd804b..c576f1d 100644
--- a/src/cpu/base.cc
+++ b/src/cpu/base.cc
@@ -127,8 +127,8 @@

 BaseCPU::BaseCPU(Params *p, bool is_checker)
     : MemObject(p), instCnt(0), _cpuId(p->cpu_id), _socketId(p->socket_id),
-      _instMasterId(p->system->getMasterId(name() + ".inst")),
-      _dataMasterId(p->system->getMasterId(name() + ".data")),
+      _instMasterId(p->system->getMasterId(this, "inst")),
+      _dataMasterId(p->system->getMasterId(this, "data")),
       _taskId(ContextSwitchTaskId::Unknown), _pid(invldPid),
_switchedOut(p->switched_out), _cacheLineSize(p->system->cacheLineSize()),
       interrupts(p->interrupts), profileEvent(NULL),
diff --git a/src/cpu/checker/cpu.cc b/src/cpu/checker/cpu.cc
index 48fcb20..07b6553 100644
--- a/src/cpu/checker/cpu.cc
+++ b/src/cpu/checker/cpu.cc
@@ -62,7 +62,7 @@
 void
 CheckerCPU::init()
 {
-    masterId = systemPtr->getMasterId(name());
+    masterId = systemPtr->getMasterId(this);
 }

 CheckerCPU::CheckerCPU(Params *p)
diff --git a/src/cpu/testers/directedtest/DirectedGenerator.cc b/src/cpu/testers/directedtest/DirectedGenerator.cc
index e37868b..2d76b86 100644
--- a/src/cpu/testers/directedtest/DirectedGenerator.cc
+++ b/src/cpu/testers/directedtest/DirectedGenerator.cc
@@ -33,7 +33,7 @@

 DirectedGenerator::DirectedGenerator(const Params *p)
     : SimObject(p),
-      masterId(p->system->getMasterId(name()))
+      masterId(p->system->getMasterId(this))
 {
     m_num_cpus = p->num_cpus;
     m_directed_tester = NULL;
diff --git a/src/cpu/testers/garnet_synthetic_traffic/GarnetSyntheticTraffic.cc b/src/cpu/testers/garnet_synthetic_traffic/GarnetSyntheticTraffic.cc
index f7513d3..56edd84 100644
--- a/src/cpu/testers/garnet_synthetic_traffic/GarnetSyntheticTraffic.cc
+++ b/src/cpu/testers/garnet_synthetic_traffic/GarnetSyntheticTraffic.cc
@@ -93,7 +93,7 @@
       injVnet(p->inj_vnet),
       precision(p->precision),
       responseLimit(p->response_limit),
-      masterId(p->system->getMasterId(name()))
+      masterId(p->system->getMasterId(this))
 {
     // set up counters
     noResponseCycles = 0;
diff --git a/src/cpu/testers/memtest/memtest.cc b/src/cpu/testers/memtest/memtest.cc
index 6f3f9b3..ccd978c 100644
--- a/src/cpu/testers/memtest/memtest.cc
+++ b/src/cpu/testers/memtest/memtest.cc
@@ -96,7 +96,7 @@
       percentReads(p->percent_reads),
       percentFunctional(p->percent_functional),
       percentUncacheable(p->percent_uncacheable),
-      masterId(p->system->getMasterId(name())),
+      masterId(p->system->getMasterId(this)),
       blockSize(p->system->cacheLineSize()),
       blockAddrMask(blockSize - 1),
       progressInterval(p->progress_interval),
diff --git a/src/cpu/testers/rubytest/RubyTester.cc b/src/cpu/testers/rubytest/RubyTester.cc
index d9ca030..67c8248 100644
--- a/src/cpu/testers/rubytest/RubyTester.cc
+++ b/src/cpu/testers/rubytest/RubyTester.cc
@@ -53,7 +53,7 @@
   : MemObject(p),
     checkStartEvent([this]{ wakeup(); }, "RubyTester tick",
                     false, Event::CPU_Tick_Pri),
-    _masterId(p->system->getMasterId(name())),
+    _masterId(p->system->getMasterId(this)),
     m_checkTable_ptr(nullptr),
     m_num_cpus(p->num_cpus),
     m_checks_to_complete(p->checks_to_complete),
diff --git a/src/cpu/testers/traffic_gen/traffic_gen.cc b/src/cpu/testers/traffic_gen/traffic_gen.cc
index 7668c51..2d4dd37 100644
--- a/src/cpu/testers/traffic_gen/traffic_gen.cc
+++ b/src/cpu/testers/traffic_gen/traffic_gen.cc
@@ -57,7 +57,7 @@
 TrafficGen::TrafficGen(const TrafficGenParams* p)
     : MemObject(p),
       system(p->system),
-      masterID(system->getMasterId(name())),
+      masterID(system->getMasterId(this)),
       configFile(p->config_file),
       elasticReq(p->elastic_req),
       progressCheck(p->progress_check),
diff --git a/src/cpu/trace/trace_cpu.cc b/src/cpu/trace/trace_cpu.cc
index 824c125..77755e8 100644
--- a/src/cpu/trace/trace_cpu.cc
+++ b/src/cpu/trace/trace_cpu.cc
@@ -50,8 +50,8 @@
     :   BaseCPU(params),
         icachePort(this),
         dcachePort(this),
-        instMasterID(params->system->getMasterId(name() + ".inst")),
-        dataMasterID(params->system->getMasterId(name() + ".data")),
+        instMasterID(params->system->getMasterId(this, "inst")),
+        dataMasterID(params->system->getMasterId(this, "data")),
         instTraceFile(params->instTraceFile),
         dataTraceFile(params->dataTraceFile),
icacheGen(*this, ".iside", icachePort, instMasterID, instTraceFile),
diff --git a/src/dev/dma_device.cc b/src/dev/dma_device.cc
index f6f751c..a80cffc 100644
--- a/src/dev/dma_device.cc
+++ b/src/dev/dma_device.cc
@@ -55,7 +55,7 @@

 DmaPort::DmaPort(MemObject *dev, System *s)
     : MasterPort(dev->name() + ".dma", dev),
-      device(dev), sys(s), masterId(s->getMasterId(dev->name())),
+      device(dev), sys(s), masterId(s->getMasterId(dev)),
       sendEvent([this]{ sendDma(); }, dev->name()),
       pendingCount(0), inRetry(false)
 { }
diff --git a/src/gpu-compute/compute_unit.cc b/src/gpu-compute/compute_unit.cc
index 87f29eb..8f2c659 100644
--- a/src/gpu-compute/compute_unit.cc
+++ b/src/gpu-compute/compute_unit.cc
@@ -74,7 +74,7 @@
     coalescerToVrfBusWidth(p->coalescer_to_vrf_bus_width),
     req_tick_latency(p->mem_req_latency * p->clk_domain->clockPeriod()),
     resp_tick_latency(p->mem_resp_latency * p->clk_domain->clockPeriod()),
-    _masterId(p->system->getMasterId(name() + ".ComputeUnit")),
+    _masterId(p->system->getMasterId(this, "ComputeUnit")),
     lds(*p->localDataStore), _cacheLineSize(p->system->cacheLineSize()),
     globalSeqNum(0), wavefrontSize(p->wfSize),
     kernelLaunchInst(new KernelLaunchStaticInst())
diff --git a/src/gpu-compute/dispatcher.cc b/src/gpu-compute/dispatcher.cc
index 7fd1101..a645281 100644
--- a/src/gpu-compute/dispatcher.cc
+++ b/src/gpu-compute/dispatcher.cc
@@ -47,7 +47,7 @@
 GpuDispatcher *GpuDispatcher::instance = nullptr;

 GpuDispatcher::GpuDispatcher(const Params *p)
-    : DmaDevice(p), _masterId(p->system->getMasterId(name() + ".disp")),
+    : DmaDevice(p), _masterId(p->system->getMasterId(this, "disp")),
       pioAddr(p->pio_addr), pioSize(4096), pioDelay(p->pio_latency),
       dispatchCount(0), dispatchActive(false), cpu(p->cpu),
       shader(p->shader_pointer), driver(p->cl_driver),
diff --git a/src/mem/cache/prefetch/base.cc b/src/mem/cache/prefetch/base.cc
index 6b4cf05..90c6742 100644
--- a/src/mem/cache/prefetch/base.cc
+++ b/src/mem/cache/prefetch/base.cc
@@ -58,7 +58,7 @@
     : ClockedObject(p), cache(nullptr), blkSize(0), lBlkSize(0),
       system(p->sys), onMiss(p->on_miss), onRead(p->on_read),
       onWrite(p->on_write), onData(p->on_data), onInst(p->on_inst),
-      masterId(system->getMasterId(name())),
+      masterId(system->getMasterId(this)),
       pageBytes(system->getPageBytes())
 {
 }
diff --git a/src/mem/external_master.cc b/src/mem/external_master.cc
index e0e8c1e..373aa84 100644
--- a/src/mem/external_master.cc
+++ b/src/mem/external_master.cc
@@ -57,7 +57,7 @@
     portName(params->name + ".port"),
     portType(params->port_type),
     portData(params->port_data),
-    masterId(params->system->getMasterId(params->name))
+    masterId(params->system->getMasterId(this))
 {}

 BaseMasterPort &
diff --git a/src/mem/mem_master.hh b/src/mem/mem_master.hh
new file mode 100644
index 0000000..f196825
--- /dev/null
+++ b/src/mem/mem_master.hh
@@ -0,0 +1,72 @@
+/*
+ * Copyright (c) 2018 ARM Limited
+ * All rights reserved
+ *
+ * The license below extends only to copyright in the software and shall
+ * not be construed as granting a license to any other intellectual
+ * property including but not limited to intellectual property relating
+ * to a hardware implementation of the functionality of the software
+ * licensed hereunder.  You may use the software subject to the license
+ * terms below provided that you ensure that this notice is replicated
+ * unmodified and in its entirety in all distributions of the software,
+ * modified or unmodified, in source code or in binary form.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met: redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer;
+ * redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution;
+ * neither the name of the copyright holders nor the names of its
+ * contributors may be used to endorse or promote products derived from
+ * this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * Authors: Giacomo Travaglini
+ */
+
+/**
+ * @file
+ * MasterInfo declaration.
+ */
+
+#ifndef __MEM_MEM_MASTER_HH__
+#define __MEM_MEM_MASTER_HH__
+
+#include "mem/request.hh"
+#include "sim/sim_object.hh"
+
+/**
+ * The MasterInfo class contains data about a specific master.
+ */
+struct MasterInfo
+{
+    MasterInfo(const SimObject* _obj,
+               std::string master_name,
+               MasterID master_id)
+      : obj(_obj), masterName(master_name), masterId(master_id)
+    {}
+
+    /** SimObject related to the Master */
+    const SimObject* obj;
+
+    /** Master Name */
+    std::string masterName;
+
+    /** Master ID */
+    MasterID masterId;
+};
+
+#endif // __MEM_MEM_MASTER_HH__
diff --git a/src/mem/ruby/slicc_interface/AbstractController.cc b/src/mem/ruby/slicc_interface/AbstractController.cc
index b920ff7..de5e810 100644
--- a/src/mem/ruby/slicc_interface/AbstractController.cc
+++ b/src/mem/ruby/slicc_interface/AbstractController.cc
@@ -51,7 +51,7 @@
 AbstractController::AbstractController(const Params *p)
     : MemObject(p), Consumer(this), m_version(p->version),
       m_clusterID(p->cluster_id),
-      m_masterId(p->system->getMasterId(name())), m_is_blocking(false),
+      m_masterId(p->system->getMasterId(this)), m_is_blocking(false),
       m_number_of_TBEs(p->number_of_TBEs),
       m_transitions_per_cycle(p->transitions_per_cycle),
       m_buffer_size(p->buffer_size), m_recycle_latency(p->recycle_latency),
diff --git a/src/sim/system.cc b/src/sim/system.cc
index 38eed1c..911ee5d 100644
--- a/src/sim/system.cc
+++ b/src/sim/system.cc
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011-2014,2017 ARM Limited
+ * Copyright (c) 2011-2014,2017-2018 ARM Limited
  * All rights reserved
  *
  * The license below extends only to copyright in the software and shall
@@ -135,11 +135,11 @@

     // Get the generic system master IDs
     MasterID tmp_id M5_VAR_USED;
-    tmp_id = getMasterId("writebacks");
+    tmp_id = getMasterId(this, "writebacks");
     assert(tmp_id == Request::wbMasterId);
-    tmp_id = getMasterId("functional");
+    tmp_id = getMasterId(this, "functional");
     assert(tmp_id == Request::funcMasterId);
-    tmp_id = getMasterId("interrupt");
+    tmp_id = getMasterId(this, "interrupt");
     assert(tmp_id == Request::intMasterId);

     if (FullSystem) {
@@ -492,15 +492,27 @@
 }

 MasterID
-System::getMasterId(std::string master_name)
+System::getGlobalMasterId(std::string master_name)
 {
-    // strip off system name if the string starts with it
+    return _getMasterId(nullptr, master_name);
+}
+
+MasterID
+System::getMasterId(const SimObject* master, std::string submaster)
+{
+    auto master_name = leafMasterName(master, submaster);
+    return _getMasterId(master, master_name);
+}
+
+MasterID
+System::_getMasterId(const SimObject* master, std::string master_name)
+{
     if (startswith(master_name, name()))
         master_name = master_name.erase(0, name().size() + 1);

     // CPUs in switch_cpus ask for ids again after switching
-    for (int i = 0; i < masterIds.size(); i++) {
-        if (masterIds[i] == master_name) {
+    for (int i = 0; i < masters.size(); i++) {
+        if (masters[i].masterName == master_name) {
             return i;
         }
     }
@@ -514,18 +526,32 @@
                 "You must do so in init().\n");
     }

-    masterIds.push_back(master_name);
+    // Generate a new MasterID incrementally
+    MasterID master_id = masters.size();

-    return masterIds.size() - 1;
+    // Append the new Master metadata to the group of system Masters.
+    masters.emplace_back(master, master_name, master_id);
+
+    return masters.back().masterId;
+}
+
+std::string
+System::leafMasterName(const SimObject* master, const std::string& submaster)
+{
+    // Get the full master name by appending the submaster name to
+    // the root SimObject master name
+    auto master_name = master->name() + "." + submaster;
+    return master_name;
 }

 std::string
 System::getMasterName(MasterID master_id)
 {
-    if (master_id >= masterIds.size())
+    if (master_id >= masters.size())
         fatal("Invalid master_id passed to getMasterName()\n");

-    return masterIds[master_id];
+    const auto& master_info = masters[master_id];
+    return master_info.masterName;
 }

 System *
diff --git a/src/sim/system.hh b/src/sim/system.hh
index a72f2a7..c9e2630 100644
--- a/src/sim/system.hh
+++ b/src/sim/system.hh
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, 2014 ARM Limited
+ * Copyright (c) 2012, 2014, 2018 ARM Limited
  * All rights reserved
  *
  * The license below extends only to copyright in the software and shall
@@ -57,6 +57,7 @@
 #include "base/statistics.hh"
 #include "config/the_isa.hh"
 #include "enums/MemoryMode.hh"
+#include "mem/mem_master.hh"
 #include "mem/mem_object.hh"
 #include "mem/physical.hh"
 #include "mem/port.hh"
@@ -320,31 +321,68 @@
      * It's used to uniquely id any master in the system by name for things
      * like cache statistics.
      */
-    std::vector<std::string> masterIds;
+    std::vector<MasterInfo> masters;

     ThermalModel * thermalModel;

   public:

- /** Request an id used to create a request object in the system. All objects
+    /**
+ * Request an id used to create a request object in the system. All objects * that intend to issues requests into the memory system must request an id
      * in the init() phase of startup. All master ids must be fixed by the
* regStats() phase that immediately precedes it. This allows objects in
      * the memory system to understand how many masters may exist and
* appropriately name the bins of their per-master stats before the stats
-     * are finalized
+     * are finalized.
+     *
+     * Registers a LOCAL MasterID:
+     * This method takes two parameters, one of which is optional.
+     * The first one is the master object, and it is compulsory; in case
+     * a object has multiple (sub)masters, a second parameter must be
+     * provided and it contains the name of the submaster. As an example:
+ * For a cpu having two masters: a data master and an instruction master,
+     * the method must be called twice:
+     * instMasterId = getMasterId(cpu, "inst");
+     * dataMasterId = getMasterId(cpu, "data");
+     *
+     * @param master SimObject related to the master
+     * @param submaster String containing the submaster's name
+     * @return the master's ID.
      */
-    MasterID getMasterId(std::string req_name);
+    MasterID getMasterId(const SimObject* master,
+                         std::string submaster = std::string());

-    /** Get the name of an object for a given request id.
+    /**
+     * Registers a GLOBAL MasterID:
+     * No SimObject is passed, so the master gets registered by providing
+     * the full master name.
+     *
+     * @param masterName full name of the master
+     * @return the master's ID.
+     */
+    MasterID getGlobalMasterId(std::string master_name);
+
+    /**
+     * Get the name of an object for a given request id.
      */
     std::string getMasterName(MasterID master_id);

     /** Get the number of masters registered in the system */
-    MasterID maxMasters()
-    {
-        return masterIds.size();
-    }
+    MasterID maxMasters() { return masters.size(); }
+
+  protected:
+    /** helper function for getMasterId */
+ MasterID _getMasterId(const SimObject* master, std::string master_name);
+
+    /**
+     * Helper function for constructing the full (sub)master name
+     * by providing the root master and the relative submaster name.
+     */
+    std::string leafMasterName(const SimObject* master,
+                               const std::string& submaster);
+
+  public:

     void regStats() override;
     /**

--
To view, visit https://gem5-review.googlesource.com/9781
To unsubscribe, or for help writing mail filters, visit https://gem5-review.googlesource.com/settings

Gerrit-Project: public/gem5
Gerrit-Branch: master
Gerrit-Change-Id: I8b97d328a65cd06f329e2cdd3679451c17d2b8f6
Gerrit-Change-Number: 9781
Gerrit-PatchSet: 1
Gerrit-Owner: Giacomo Travaglini <giacomo.travagl...@arm.com>
Gerrit-Reviewer: Andreas Sandberg <andreas.sandb...@arm.com>
Gerrit-MessageType: newchange
_______________________________________________
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Reply via email to