[gem5-dev] Cron m5test@zizzer /z/m5/regression/do-regression quick

2015-04-14 Thread Cron Daemon
* build/ALPHA/tests/opt/quick/se/00.hello/alpha/linux/minor-timing passed.
* build/ALPHA/tests/opt/quick/se/00.hello/alpha/linux/o3-timing passed.
* build/ALPHA/tests/opt/quick/se/00.hello/alpha/linux/simple-timing passed.
* build/ALPHA/tests/opt/quick/se/00.hello/alpha/linux/simple-atomic passed.
* build/ALPHA/tests/opt/quick/se/00.hello/alpha/linux/simple-timing-ruby 
passed.
* build/ALPHA/tests/opt/quick/se/00.hello/alpha/tru64/minor-timing passed.
 * build/ALPHA/tests/opt/quick/se/00.hello/alpha/tru64/o3-timing passed.
* build/ALPHA/tests/opt/quick/se/00.hello/alpha/tru64/simple-timing passed.
* build/ALPHA/tests/opt/quick/se/00.hello/alpha/tru64/simple-atomic passed.
 * build/ALPHA/tests/opt/quick/se/00.hello/alpha/tru64/simple-timing-ruby 
passed.
* build/ALPHA/tests/opt/quick/se/01.hello-2T-smt/alpha/linux/o3-timing 
passed.
* build/ALPHA/tests/opt/quick/se/20.eio-short/alpha/eio/simple-timing 
passed.
* build/ALPHA/tests/opt/quick/se/20.eio-short/alpha/eio/simple-atomic 
passed.
* build/ALPHA/tests/opt/quick/se/30.eio-mp/alpha/eio/simple-atomic-mp 
passed.
* build/ALPHA/tests/opt/quick/se/30.eon/alpha/tru64/simple-atomic passed.
* build/ALPHA/tests/opt/quick/se/30.eio-mp/alpha/eio/simple-timing-mp 
passed.
* build/ALPHA/tests/opt/quick/se/50.memtest/alpha/linux/memtest-ruby passed.
* build/ALPHA/tests/opt/quick/se/50.vortex/alpha/tru64/simple-atomic passed.
* build/ALPHA/tests/opt/quick/se/50.vortex/alpha/tru64/simple-timing passed.
* build/ALPHA/tests/opt/quick/se/70.twolf/alpha/tru64/simple-atomic passed.
* build/ALPHA/tests/opt/quick/se/70.twolf/alpha/tru64/simple-timing passed.
 * build/ALPHA/tests/opt/quick/se/60.rubytest/alpha/linux/rubytest-ruby 
passed.
* 
build/ALPHA/tests/opt/quick/fs/10.linux-boot/alpha/linux/tsunami-simple-atomic 
passed.
* 
build/ALPHA/tests/opt/quick/fs/10.linux-boot/alpha/linux/tsunami-simple-atomic-dual
 passed.
 * 
build/ALPHA/tests/opt/quick/fs/10.linux-boot/alpha/linux/tsunami-simple-timing 
passed.
* 
build/ALPHA/tests/opt/quick/fs/10.linux-boot/alpha/linux/tsunami-simple-timing-dual
 passed.
* 
build/ALPHA/tests/opt/quick/fs/80.netperf-stream/alpha/linux/twosys-tsunami-simple-atomic
 passed.
* 
build/ALPHA_MOESI_hammer/tests/opt/quick/se/00.hello/alpha/linux/simple-timing-ruby-MOESI_hammer
 passed.
* 
build/ALPHA_MOESI_hammer/tests/opt/quick/se/00.hello/alpha/tru64/simple-timing-ruby-MOESI_hammer
 passed.
* 
build/ALPHA_MOESI_hammer/tests/opt/quick/se/50.memtest/alpha/linux/memtest-ruby-MOESI_hammer
 passed.
* 
build/ALPHA_MOESI_hammer/tests/opt/quick/se/60.rubytest/alpha/linux/rubytest-ruby-MOESI_hammer
 passed.
* 
build/ALPHA_MESI_Two_Level/tests/opt/quick/se/00.hello/alpha/tru64/simple-timing-ruby-MESI_Two_Level
 passed.
* 
build/ALPHA_MESI_Two_Level/tests/opt/quick/se/50.memtest/alpha/linux/memtest-ruby-MESI_Two_Level
 passed.
* 
build/ALPHA_MESI_Two_Level/tests/opt/quick/se/60.rubytest/alpha/linux/rubytest-ruby-MESI_Two_Level
 passed.
 * 
build/ALPHA_MESI_Two_Level/tests/opt/quick/se/00.hello/alpha/linux/simple-timing-ruby-MESI_Two_Level
 passed.
* 
build/ALPHA_MOESI_CMP_directory/tests/opt/quick/se/00.hello/alpha/tru64/simple-timing-ruby-MOESI_CMP_directory
 passed.
* 
build/ALPHA_MOESI_CMP_directory/tests/opt/quick/se/00.hello/alpha/linux/simple-timing-ruby-MOESI_CMP_directory
 passed.
* 
build/ALPHA_MOESI_CMP_directory/tests/opt/quick/se/50.memtest/alpha/linux/memtest-ruby-MOESI_CMP_directory
 passed.
* 
build/ALPHA_MOESI_CMP_directory/tests/opt/quick/se/60.rubytest/alpha/linux/rubytest-ruby-MOESI_CMP_directory
 passed.
* 
build/ALPHA_MOESI_CMP_token/tests/opt/quick/se/50.memtest/alpha/linux/memtest-ruby-MOESI_CMP_token
 passed.
* 
build/ALPHA_MOESI_CMP_token/tests/opt/quick/se/00.hello/alpha/linux/simple-timing-ruby-MOESI_CMP_token
 passed.
* 
build/ALPHA_MOESI_CMP_token/tests/opt/quick/se/60.rubytest/alpha/linux/rubytest-ruby-MOESI_CMP_token
 passed.
* 
build/ALPHA_MOESI_CMP_token/tests/opt/quick/se/00.hello/alpha/tru64/simple-timing-ruby-MOESI_CMP_token
 passed.
* build/MIPS/tests/opt/quick/se/00.hello/mips/linux/simple-atomic passed.
 * build/MIPS/tests/opt/quick/se/00.hello/mips/linux/simple-timing passed.
* build/MIPS/tests/opt/quick/se/00.hello/mips/linux/simple-timing-ruby 
passed.
* build/MIPS/tests/opt/quick/se/00.hello/mips/linux/o3-timing passed.
* build/NULL/tests/opt/quick/se/50.memtest/null/none/memtest passed.
* build/NULL/tests/opt/quick/se/50.memtest/null/none/memtest-filter passed.
* build/NULL/tests/opt/quick/se/70.tgen/null/none/tgen-simple-mem passed.
* build/NULL/tests/opt/quick/se/70.tgen/null/none/tgen-dram-ctrl passed.
* build/POWER/tests/opt/quick/se/00.hello/power/linux/o3-timing 
passed.* build/POWER/tests/opt/quick/se/00.hello/power/linux/simple-atomic 
passed.
* 

[gem5-dev] Destructors not called ?

2015-04-14 Thread VAUMOURIN Grégory
Hello everyone,

I have a naive question. I'm trying to simulate some memory hierarchies with 
Ruby and it seems to me that the destructors of the Cache Memory objects 
created for the simulation are not called. Which object is in charge of 
destroying the cache memories ? Because the Cache Controller have the pointer 
to the cache memory object but it never calls the destructors . 
(I'm using the MESI protocol)  

--
Gregory Vaumourin 


___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev


Re: [gem5-dev] Destructors not called ?

2015-04-14 Thread VAUMOURIN Grégory
Thanks for the helpfull answer \o/ I'll see what I can do !

--
Gregory Vaumourin
___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev


[gem5-dev] changeset in gem5: config, cpu: fix progress interval for switch...

2015-04-14 Thread Malek Musleh
changeset ee82c2c30421 in /z/repo/gem5
details: http://repo.gem5.org/gem5?cmd=changeset;node=ee82c2c30421
description:
config, cpu: fix progress interval for switched CPUs
This patch ensures that the CPU progress Event is triggered for the new 
set of
switched_cpus that get scheduled (e.g. during fast-forwarding). it also 
avoids
printing the interval state if the cpu is currently switched out.

Committed by: Nilay Vaish ni...@cs.wisc.edu

diffstat:

 configs/common/Simulation.py |   1 +
 src/cpu/base.cc  |  11 ---
 2 files changed, 9 insertions(+), 3 deletions(-)

diffs (39 lines):

diff -r f56c10663a01 -r ee82c2c30421 configs/common/Simulation.py
--- a/configs/common/Simulation.py  Mon Apr 13 17:33:57 2015 -0500
+++ b/configs/common/Simulation.py  Tue Apr 14 11:01:10 2015 -0500
@@ -462,6 +462,7 @@
 switch_cpus[i].system =  testsys
 switch_cpus[i].workload = testsys.cpu[i].workload
 switch_cpus[i].clk_domain = testsys.cpu[i].clk_domain
+switch_cpus[i].progress_interval = testsys.cpu[i].progress_interval
 # simulation period
 if options.maxinsts:
 switch_cpus[i].max_insts_any_thread = options.maxinsts
diff -r f56c10663a01 -r ee82c2c30421 src/cpu/base.cc
--- a/src/cpu/base.cc   Mon Apr 13 17:33:57 2015 -0500
+++ b/src/cpu/base.cc   Tue Apr 14 11:01:10 2015 -0500
@@ -94,6 +94,14 @@
 CPUProgressEvent::process()
 {
 Counter temp = cpu-totalOps();
+
+if (_repeatEvent)
+  cpu-schedule(this, curTick() + _interval);
+
+if(cpu-switchedOut()) {
+  return;
+}
+
 #ifndef NDEBUG
 double ipc = double(temp - lastNumInst) / (_interval / cpu-clockPeriod());
 
@@ -107,9 +115,6 @@
 temp - lastNumInst);
 #endif
 lastNumInst = temp;
-
-if (_repeatEvent)
-cpu-schedule(this, curTick() + _interval);
 }
 
 const char *
___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev


[gem5-dev] changeset in gem5: stats: x86: changes due to recent patches

2015-04-14 Thread Nilay Vaish
changeset 3c6a78d23507 in /z/repo/gem5
details: http://repo.gem5.org/gem5?cmd=changeset;node=3c6a78d23507
description:
stats: x86: changes due to recent patches

The change in 20.parser is from new x87 instructions.  The change to
pc-o3-timing is not clear to me.  It seems that this test might be 
invoking
some undefined behavior.

diffstat:

 tests/long/fs/10.linux-boot/ref/x86/linux/pc-o3-timing/stats.txt |  2478 
+-
 tests/long/se/20.parser/ref/x86/linux/o3-timing/stats.txt|  1538 +++---
 2 files changed, 2007 insertions(+), 2009 deletions(-)

diffs (truncated from 4466 to 300 lines):

diff -r ee82c2c30421 -r 3c6a78d23507 
tests/long/fs/10.linux-boot/ref/x86/linux/pc-o3-timing/stats.txt
--- a/tests/long/fs/10.linux-boot/ref/x86/linux/pc-o3-timing/stats.txt  Tue Apr 
14 11:01:10 2015 -0500
+++ b/tests/long/fs/10.linux-boot/ref/x86/linux/pc-o3-timing/stats.txt  Tue Apr 
14 11:01:11 2015 -0500
@@ -1,134 +1,134 @@
 
 -- Begin Simulation Statistics --
-sim_seconds  5.162227   # 
Number of seconds simulated
-sim_ticks5162226977000   # 
Number of ticks simulated
-final_tick   5162226977000   # 
Number of ticks from beginning of simulation (restored from checkpoints and 
never reset)
+sim_seconds  5.154240   # 
Number of seconds simulated
+sim_ticks5154239928000   # 
Number of ticks simulated
+final_tick   5154239928000   # 
Number of ticks from beginning of simulation (restored from checkpoints and 
never reset)
 sim_freq 1   # 
Frequency of simulated ticks
-host_inst_rate 127909   # 
Simulator instruction rate (inst/s)
-host_op_rate   252832   # 
Simulator op (including micro ops) rate (op/s)
-host_tick_rate 1618521974   # 
Simulator tick rate (ticks/s)
-host_mem_usage 804704   # 
Number of bytes of host memory used
-host_seconds  3189.47   # 
Real time elapsed on the host
-sim_insts   407963408   # 
Number of instructions simulated
-sim_ops 806401326   # 
Number of ops (including micro ops) simulated
+host_inst_rate 129299   # 
Simulator instruction rate (inst/s)
+host_op_rate   255578   # 
Simulator op (including micro ops) rate (op/s)
+host_tick_rate 1633591785   # 
Simulator tick rate (ticks/s)
+host_mem_usage 806864   # 
Number of bytes of host memory used
+host_seconds  3155.16   # 
Real time elapsed on the host
+sim_insts   407959851   # 
Number of instructions simulated
+sim_ops 806389826   # 
Number of ops (including micro ops) simulated
 system.voltage_domain.voltage   1   # 
Voltage in Volts
 system.clk_domain.clock  1000   # 
Clock period in ticks
-system.physmem.bytes_read::cpu.dtb.walker 4608   # 
Number of bytes read from this memory
-system.physmem.bytes_read::cpu.itb.walker  384   # 
Number of bytes read from this memory
-system.physmem.bytes_read::cpu.inst   1045376   # 
Number of bytes read from this memory
-system.physmem.bytes_read::cpu.data  10748480   # 
Number of bytes read from this memory
+system.physmem.bytes_read::cpu.dtb.walker 4224   # 
Number of bytes read from this memory
+system.physmem.bytes_read::cpu.itb.walker  320   # 
Number of bytes read from this memory
+system.physmem.bytes_read::cpu.inst   1048832   # 
Number of bytes read from this memory
+system.physmem.bytes_read::cpu.data  10760128   # 
Number of bytes read from this memory
 system.physmem.bytes_read::pc.south_bridge.ide28352
   # Number of bytes read from this memory
-system.physmem.bytes_read::total 11827200   # 
Number of bytes read from this memory
-system.physmem.bytes_inst_read::cpu.inst  1045376  

[gem5-dev] changeset in gem5: sim: Use NULL instead of None for testing fil...

2015-04-14 Thread Nilay Vaish
changeset 0a78638881d7 in /z/repo/gem5
details: http://repo.gem5.org/gem5?cmd=changeset;node=0a78638881d7
description:
sim: Use NULL instead of None for testing filenames.
The filenames are initialized with NULL.  So the test should be
checking for them to be == NULL instead == None.

diffstat:

 src/sim/process.cc |  15 ---
 src/sim/process.hh |   2 +-
 2 files changed, 9 insertions(+), 8 deletions(-)

diffs (72 lines):

diff -r 024af426b59a -r 0a78638881d7 src/sim/process.cc
--- a/src/sim/process.ccMon Apr 13 17:33:57 2015 -0500
+++ b/src/sim/process.ccMon Apr 13 17:33:57 2015 -0500
@@ -272,7 +272,8 @@
 
 // generate new target fd for sim_fd
 int
-Process::alloc_fd(int sim_fd, string filename, int flags, int mode, bool pipe)
+Process::alloc_fd(int sim_fd, const string filename, int flags, int mode,
+  bool pipe)
 {
 // in case open() returns an error, don't allocate a new fd
 if (sim_fd == -1)
@@ -384,7 +385,7 @@
 
 if (in == stdin || in == cin)
 stdin_fd = STDIN_FILENO;
-else if (in == None)
+else if (in == NULL)
 stdin_fd = -1;
 else {
 // open standard in and seek to the right location
@@ -397,7 +398,7 @@
 stdout_fd = STDOUT_FILENO;
 else if (out == stderr || out == cerr)
 stdout_fd = STDERR_FILENO;
-else if (out == None)
+else if (out == NULL)
 stdout_fd = -1;
 else {
 stdout_fd = Process::openOutputFile(out);
@@ -409,7 +410,7 @@
 stderr_fd = STDOUT_FILENO;
 else if (err == stderr || err == cerr)
 stderr_fd = STDERR_FILENO;
-else if (err == None)
+else if (err == NULL)
 stderr_fd = -1;
 else if (err == out)
 stderr_fd = stdout_fd;
@@ -456,7 +457,7 @@
 fdo-fd = fd;
 
 //Seek to correct location before checkpoint
-if (lseek(fd,fdo-fileOffset, SEEK_SET)  0)
+if (lseek(fd, fdo-fileOffset, SEEK_SET)  0)
 panic(Unable to seek to correct location in file: %s,
   fdo-filename);
 }
@@ -472,8 +473,8 @@
 if (fdo-fd != -1) {
 fdo-fileOffset = lseek(fdo-fd, 0, SEEK_CUR);
 } else {
-fdo-filename = NULL;
-fdo-fileOffset = 0;
+fdo-filename = NULL;
+fdo-fileOffset = 0;
 }
 }
 }
diff -r 024af426b59a -r 0a78638881d7 src/sim/process.hh
--- a/src/sim/process.hhMon Apr 13 17:33:57 2015 -0500
+++ b/src/sim/process.hhMon Apr 13 17:33:57 2015 -0500
@@ -189,7 +189,7 @@
 void dup_fd(int sim_fd, int tgt_fd);
 
 // generate new target fd for sim_fd
-int alloc_fd(int sim_fd, std::string filename, int flags, int mode,
+int alloc_fd(int sim_fd, const std::string filename, int flags, int mode,
  bool pipe);
 
 // free target fd (e.g., after close)
___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev


[gem5-dev] changeset in gem5: sim: fix function for emulating dup()

2015-04-14 Thread Nilay Vaish
changeset 024af426b59a in /z/repo/gem5
details: http://repo.gem5.org/gem5?cmd=changeset;node=024af426b59a
description:
sim: fix function for emulating dup()
The function was using the host fd to obtain the fd object from the 
simulated
process.

diffstat:

 src/sim/syscall_emul.cc |  9 +
 1 files changed, 5 insertions(+), 4 deletions(-)

diffs (22 lines):

diff -r 46070443051e -r 024af426b59a src/sim/syscall_emul.cc
--- a/src/sim/syscall_emul.cc   Wed Apr 08 15:56:06 2015 -0500
+++ b/src/sim/syscall_emul.cc   Mon Apr 13 17:33:57 2015 -0500
@@ -594,13 +594,14 @@
 dupFunc(SyscallDesc *desc, int num, LiveProcess *process, ThreadContext *tc)
 {
 int index = 0;
-int fd = process-sim_fd(process-getSyscallArg(tc, index));
-if (fd  0)
+int tgt_fd = process-getSyscallArg(tc, index);
+int sim_fd = process-sim_fd(tgt_fd);
+if (sim_fd  0)
 return -EBADF;
 
-Process::FdMap *fdo = process-sim_fd_obj(fd);
+Process::FdMap *fdo = process-sim_fd_obj(tgt_fd);
 
-int result = dup(fd);
+int result = dup(sim_fd);
 return (result == -1) ? -errno :
 process-alloc_fd(result, fdo-filename, fdo-flags, fdo-mode, false);
 }
___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev


[gem5-dev] changeset in gem5: ruby: allow restoring from checkpoint when us...

2015-04-14 Thread Lena Olson
changeset 631e736554c9 in /z/repo/gem5
details: http://repo.gem5.org/gem5?cmd=changeset;node=631e736554c9
description:
ruby: allow restoring from checkpoint when using DRAMCtrl

Restoring from a checkpoint with ruby + the DRAMCtrl memory model was 
not
working, because ruby and DRAMCtrl disagreed on the current tick during 
warmup.
Since there is no reason to do timing requests during warmup, use 
functional
requests instead.

Committed by: Nilay Vaish ni...@cs.wisc.edu

diffstat:

 src/mem/ruby/slicc_interface/AbstractController.cc |  17 -
 src/mem/ruby/slicc_interface/AbstractController.hh |   3 +++
 2 files changed, 19 insertions(+), 1 deletions(-)

diffs (54 lines):

diff -r 0a78638881d7 -r 631e736554c9 
src/mem/ruby/slicc_interface/AbstractController.cc
--- a/src/mem/ruby/slicc_interface/AbstractController.ccMon Apr 13 
17:33:57 2015 -0500
+++ b/src/mem/ruby/slicc_interface/AbstractController.ccMon Apr 13 
17:33:57 2015 -0500
@@ -40,7 +40,8 @@
   m_transitions_per_cycle(p-transitions_per_cycle),
   m_buffer_size(p-buffer_size), m_recycle_latency(p-recycle_latency),
   memoryPort(csprintf(%s.memory, name()), this, ),
-  m_responseFromMemory_ptr(new MessageBuffer())
+  m_responseFromMemory_ptr(new MessageBuffer()),
+  m_rubySystem(p-ruby_system)
 {
 // Set the sender pointer of the response message buffer from the
 // memory controller.
@@ -217,6 +218,13 @@
 SenderState *s = new SenderState(id);
 pkt-pushSenderState(s);
 
+// Use functional rather than timing accesses during warmup
+if (m_rubySystem-m_warmup_enabled) {
+memoryPort.sendFunctional(pkt);
+recvTimingResp(pkt);
+return;
+}
+
 memoryPort.schedTimingReq(pkt, clockEdge(latency));
 }
 
@@ -237,6 +245,13 @@
 SenderState *s = new SenderState(id);
 pkt-pushSenderState(s);
 
+// Use functional rather than timing accesses during warmup
+if (m_rubySystem-m_warmup_enabled) {
+memoryPort.sendFunctional(pkt);
+recvTimingResp(pkt);
+return;
+}
+
 // Create a block and copy data from the block.
 memoryPort.schedTimingReq(pkt, clockEdge(latency));
 }
diff -r 0a78638881d7 -r 631e736554c9 
src/mem/ruby/slicc_interface/AbstractController.hh
--- a/src/mem/ruby/slicc_interface/AbstractController.hhMon Apr 13 
17:33:57 2015 -0500
+++ b/src/mem/ruby/slicc_interface/AbstractController.hhMon Apr 13 
17:33:57 2015 -0500
@@ -205,6 +205,9 @@
 // memory controller.
 MessageBuffer *m_responseFromMemory_ptr;
 
+// Needed so we know if we are warming up
+RubySystem *m_rubySystem;
+
 // State that is stored in packets sent to the memory controller.
 struct SenderState : public Packet::SenderState
 {
___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev


Re: [gem5-dev] changeset in gem5: ruby: allow restoring from checkpoint when us...

2015-04-14 Thread Joel Hestness
Hi Nilay,

On Tue, Apr 14, 2015 at 11:33 AM, Nilay Vaish ni...@cs.wisc.edu wrote:

 On Tue, 14 Apr 2015, Joel Hestness wrote:

  This patch didn't address my requested change (
 http://reviews.gem5.org/r/2702/), and shouldn't have been checked in. The
 AbstractController should not have or even need another pointer to the
 RubySystem, because it can access it with g_system_ptr. This is a dead
 simple fix. Adding the pointer is going in the wrong direction for
 abstraction.


 Joel, in my opinion we should be eliminating global / static variables. I
 have personally found them to be a problem while trying to parallelize Ruby.


So, I have a couple issues with this:
 1) You didn't raise this opinion in the review itself, but instead made
the decision without seeking consensus.
 2) Duplicating a global pointer as members within other objects does not
change the way that the pointers need to be handled when parallelizing
Ruby. If there is a shared data member within the RubySystem that needs to
be locked to ensure consistency, you'll need lock that variable no matter
how other objects access the RubySystem. Further, the RubySystem global
pointer is set exactly once in the RubySystem constructor and never allowed
to be changed again. Therefore, there should be no worries about races on
the global pointer itself, AND distributing RubySystem pointers into other
objects is more likely to generate potential races if there is ever a need
to update those pointers. Thus, I strongly disagree with your opinion on
distributing pointers into the AbstractControllers.

  Am I missing something here?

  Joel


-- 
  Joel Hestness
  PhD Candidate, Computer Architecture
  Dept. of Computer Science, University of Wisconsin - Madison
  http://pages.cs.wisc.edu/~hestness/
___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev


[gem5-dev] changeset in gem5: x86: implements x87 mult/div instructions

2015-04-14 Thread Nilay Vaish
changeset 2f1a0f6d5d77 in /z/repo/gem5
details: http://repo.gem5.org/gem5?cmd=changeset;node=2f1a0f6d5d77
description:
x86: implements x87 mult/div instructions

diffstat:

 src/arch/x86/isa/decoder/x87.isa|  23 --
 src/arch/x86/isa/insts/x87/arithmetic/division.py   |  59 -
 src/arch/x86/isa/insts/x87/arithmetic/multiplication.py |  59 -
 3 files changed, 130 insertions(+), 11 deletions(-)

diffs (207 lines):

diff -r 631e736554c9 -r 2f1a0f6d5d77 src/arch/x86/isa/decoder/x87.isa
--- a/src/arch/x86/isa/decoder/x87.isa  Mon Apr 13 17:33:57 2015 -0500
+++ b/src/arch/x86/isa/decoder/x87.isa  Mon Apr 13 17:33:57 2015 -0500
@@ -45,7 +45,10 @@
 // 32-bit memory operand
 default: Inst::FADD1(Md);
 }
-0x1: fmul();
+0x1: decode MODRM_MOD {
+0x3: Inst::FMUL1(Eq);
+default: Inst::FMUL1(Md);
+}
 0x2: fcom();
 0x3: fcomp();
 0x4: decode MODRM_MOD {
@@ -53,7 +56,10 @@
 default: Inst::FSUB1(Md);
 }
 0x5: fsubr();
-0x6: fdiv();
+0x6: decode MODRM_MOD {
+0x3: Inst::FDIV1(Eq);
+default: Inst::FDIV1(Md);
+}
 0x7: fdivr();
 }
 0x1: decode MODRM_REG {
@@ -210,7 +216,10 @@
 0x3: Inst::FADD2(Eq);
 default: Inst::FADD2(Mq);
 }
-0x1: fmul();
+0x1: decode MODRM_MOD {
+0x3: Inst::FMUL2(Eq);
+default: Inst::FMUL2(Mq);
+}
 0x2: decode MODRM_MOD {
 0x3: Inst::UD2();
 default: fcom();
@@ -229,10 +238,10 @@
 }
 0x6: decode MODRM_MOD {
 0x3: fdivr();
-default: fdiv();
+default: Inst::FDIV2(Mq);
 }
 0x7: decode MODRM_MOD {
-0x3: fdiv();
+0x3: Inst::FDIV2(Eq);
 default: fdivr();
 }
 }
@@ -282,7 +291,7 @@
 default: fiadd();
 }
 0x1: decode MODRM_MOD {
-0x3: fmulp();
+0x3: Inst::FMULP(Eq);
 default: fimul();
 }
 0x2: decode MODRM_MOD {
@@ -312,7 +321,7 @@
 default: fidiv();
 }
 0x7: decode MODRM_MOD {
-0x3: fdivp();
+0x3: Inst::FDIVP(Eq);
 default: fidivr();
 }
 }
diff -r 631e736554c9 -r 2f1a0f6d5d77 
src/arch/x86/isa/insts/x87/arithmetic/division.py
--- a/src/arch/x86/isa/insts/x87/arithmetic/division.py Mon Apr 13 17:33:57 
2015 -0500
+++ b/src/arch/x86/isa/insts/x87/arithmetic/division.py Mon Apr 13 17:33:57 
2015 -0500
@@ -36,8 +36,63 @@
 # Authors: Gabe Black
 
 microcode = '''
-# FDIV
-# FDIVP
+def macroop FDIV1_R
+{
+divfp st(0), st(0), sti
+};
+
+
+def macroop FDIV1_M
+{
+ldfp87 ufp1, seg, sib, disp
+divfp st(0), st(0), ufp1
+};
+
+def macroop FDIV1_P
+{
+rdip t7
+ldfp87 ufp1, seg, riprel, disp
+divfp st(0), st(0), ufp1
+};
+
+def macroop FDIV2_R
+{
+divfp sti, sti, st(0)
+};
+
+def macroop FDIV2_M
+{
+ldfp87 ufp1, seg, sib, disp
+divfp st(0), st(0), ufp1
+};
+
+def macroop FDIV2_P
+{
+rdip t7
+ldfp87 ufp1, seg, riprel, disp
+divfp st(0), st(0), ufp1
+};
+
+def macroop FDIVP
+{
+divfp st(1), st(1), st(0), spm=1
+};
+
+def macroop FDIVP_R
+{
+divfp sti, sti, st(0), spm=1
+};
+
+def macroop FDIVP_M
+{
+fault std::make_sharedUnimpInstFault()
+};
+
+def macroop FDIVP_P
+{
+   fault std::make_sharedUnimpInstFault()
+};
+
 # FIDIV
 # FDIVR
 # FDIVRP
diff -r 631e736554c9 -r 2f1a0f6d5d77 
src/arch/x86/isa/insts/x87/arithmetic/multiplication.py
--- a/src/arch/x86/isa/insts/x87/arithmetic/multiplication.py   Mon Apr 13 
17:33:57 2015 -0500
+++ b/src/arch/x86/isa/insts/x87/arithmetic/multiplication.py   Mon Apr 13 
17:33:57 2015 -0500
@@ -36,7 +36,62 @@
 # Authors: Gabe Black
 
 microcode = '''
-# FMUL
-# FMULP
+def macroop FMUL1_R
+{
+mulfp st(0), sti, st(0)
+};
+
+
+def macroop FMUL1_M
+{
+ldfp87 ufp1, seg, sib, disp
+mulfp st(0), st(0), ufp1
+};
+
+def macroop FMUL1_P
+{
+rdip t7
+ldfp87 ufp1, seg, riprel, disp
+mulfp st(0), st(0), ufp1
+};
+
+def macroop FMUL2_R
+{
+mulfp sti, sti, st(0)
+};
+
+def macroop FMUL2_M
+{
+ldfp87 ufp1, seg, sib, disp
+mulfp st(0), st(0), ufp1
+};
+
+def macroop FMUL2_P
+{
+rdip t7
+ldfp87 ufp1, seg, riprel, disp
+mulfp st(0), st(0), ufp1
+};
+
+def macroop FMULP
+{
+mulfp st(1), st(0), st(1), spm=1
+};
+
+def macroop FMULP_R
+{
+mulfp sti, sti, st(0), spm=1
+};
+
+def macroop FMULP_M
+{
+fault std::make_sharedUnimpInstFault()
+};
+
+def macroop FMULP_P
+{
+   fault std::make_sharedUnimpInstFault()
+};
+
 # FIMUL
 

Re: [gem5-dev] changeset in gem5: ruby: allow restoring from checkpoint when us...

2015-04-14 Thread Nilay Vaish

On Tue, 14 Apr 2015, Joel Hestness wrote:


This patch didn't address my requested change (
http://reviews.gem5.org/r/2702/), and shouldn't have been checked in. The
AbstractController should not have or even need another pointer to the
RubySystem, because it can access it with g_system_ptr. This is a dead
simple fix. Adding the pointer is going in the wrong direction for
abstraction.



Joel, in my opinion we should be eliminating global / static variables. 
I have personally found them to be a problem while trying to parallelize 
Ruby.


--
Nilay
___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev


Re: [gem5-dev] changeset in gem5: ruby: allow restoring from checkpoint when us...

2015-04-14 Thread Joel Hestness
This patch didn't address my requested change (
http://reviews.gem5.org/r/2702/), and shouldn't have been checked in. The
AbstractController should not have or even need another pointer to the
RubySystem, because it can access it with g_system_ptr. This is a dead
simple fix. Adding the pointer is going in the wrong direction for
abstraction.

  Joel



On Tue, Apr 14, 2015 at 11:03 AM, Lena Olson l...@cs.wisc.edu wrote:

 changeset 631e736554c9 in /z/repo/gem5
 details: http://repo.gem5.org/gem5?cmd=changeset;node=631e736554c9
 description:
 ruby: allow restoring from checkpoint when using DRAMCtrl

 Restoring from a checkpoint with ruby + the DRAMCtrl memory model
 was not
 working, because ruby and DRAMCtrl disagreed on the current tick
 during warmup.
 Since there is no reason to do timing requests during warmup, use
 functional
 requests instead.

 Committed by: Nilay Vaish ni...@cs.wisc.edu

 diffstat:

  src/mem/ruby/slicc_interface/AbstractController.cc |  17 -
  src/mem/ruby/slicc_interface/AbstractController.hh |   3 +++
  2 files changed, 19 insertions(+), 1 deletions(-)

 diffs (54 lines):

 diff -r 0a78638881d7 -r 631e736554c9
 src/mem/ruby/slicc_interface/AbstractController.cc
 --- a/src/mem/ruby/slicc_interface/AbstractController.ccMon Apr 13
 17:33:57 2015 -0500
 +++ b/src/mem/ruby/slicc_interface/AbstractController.ccMon Apr 13
 17:33:57 2015 -0500
 @@ -40,7 +40,8 @@
m_transitions_per_cycle(p-transitions_per_cycle),
m_buffer_size(p-buffer_size),
 m_recycle_latency(p-recycle_latency),
memoryPort(csprintf(%s.memory, name()), this, ),
 -  m_responseFromMemory_ptr(new MessageBuffer())
 +  m_responseFromMemory_ptr(new MessageBuffer()),
 +  m_rubySystem(p-ruby_system)
  {
  // Set the sender pointer of the response message buffer from the
  // memory controller.
 @@ -217,6 +218,13 @@
  SenderState *s = new SenderState(id);
  pkt-pushSenderState(s);

 +// Use functional rather than timing accesses during warmup
 +if (m_rubySystem-m_warmup_enabled) {
 +memoryPort.sendFunctional(pkt);
 +recvTimingResp(pkt);
 +return;
 +}
 +
  memoryPort.schedTimingReq(pkt, clockEdge(latency));
  }

 @@ -237,6 +245,13 @@
  SenderState *s = new SenderState(id);
  pkt-pushSenderState(s);

 +// Use functional rather than timing accesses during warmup
 +if (m_rubySystem-m_warmup_enabled) {
 +memoryPort.sendFunctional(pkt);
 +recvTimingResp(pkt);
 +return;
 +}
 +
  // Create a block and copy data from the block.
  memoryPort.schedTimingReq(pkt, clockEdge(latency));
  }
 diff -r 0a78638881d7 -r 631e736554c9
 src/mem/ruby/slicc_interface/AbstractController.hh
 --- a/src/mem/ruby/slicc_interface/AbstractController.hhMon Apr 13
 17:33:57 2015 -0500
 +++ b/src/mem/ruby/slicc_interface/AbstractController.hhMon Apr 13
 17:33:57 2015 -0500
 @@ -205,6 +205,9 @@
  // memory controller.
  MessageBuffer *m_responseFromMemory_ptr;

 +// Needed so we know if we are warming up
 +RubySystem *m_rubySystem;
 +
  // State that is stored in packets sent to the memory controller.
  struct SenderState : public Packet::SenderState
  {
 ___
 gem5-dev mailing list
 gem5-dev@gem5.org
 http://m5sim.org/mailman/listinfo/gem5-dev




-- 
  Joel Hestness
  PhD Candidate, Computer Architecture
  Dept. of Computer Science, University of Wisconsin - Madison
  http://pages.cs.wisc.edu/~hestness/
___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev


Re: [gem5-dev] changeset in gem5: ruby: allow restoring from checkpoint when us...

2015-04-14 Thread Joel Hestness
Hi Brandon,


 Not looking to pick sides here, but I have also encountered problems with
 global/static variables in Ruby as well.  This past December, I spent a
 couple of weeks trying to clean up the Ruby code so that we could invoke
 two instances of Ruby at the same time in the simulator.  It was not
 trivial as the problems extended into slicc auto-generated code as well and
 I ended up temporarily abandoning it.  The high level code organization and
 design decisions should not prevent us from doing simple straight-forward
 things like invoking two Ruby instances at the same time.

 When I was trying to solve the problem, I independently followed the same
 method as Nilay.  I added ruby system pointers to most of the objects so
 that they could back-reference the system object (since it's the central
 point of that Ruby instance) and obtain access to the other subsidiary
 objects.  I felt really dirty while writing the code and frankly the code
 was painful to look at, but there didn't appear to be a better method (at
 least not one that I could see).  A couple of folks that I spoke with here
 at AMD felt that the global/static variables needed to be fixed, but agreed
 that there was no easy solution.


Thank you for this input. I completely agree that the code organization is
pretty problematic for using multiple instances of the RubySystem, which
would be nice for simulating multiple systems in a single simulation. I
also thought through this during my review of the patch in question. That
said, it seems that your argument here favors neither or both Nilay's and
my positions, but not either. First, I haven't heard publicly that the gem5
community has adopted the aim developing gem5 toward allowing multiple
RubySystem instances. I'd prefer the community unite on a
multiple-RubySystem aim before we use the possible functionality to support
either position.

Now, I'm willing to accept that there are gem5 users who may desire to
simulate multiple systems and that it may be in the community's interest to
adopt this direction. However, as the code is currently written, this
cannot work (as you note) due to inappropriately scoped RubySystem
variables. It's hard to say whether/how the RubySystem should exist in a
multi-system simulation, and I can envision organizations that support both
Nilay and me. Some details:

Part of my original suggestion on this patch was aimed at better scoping,
which would support multiple RubySystem instances: we should statically
(i.e. globally) define the warmup and cooldown variables of the
RubySystem. Warmup and cooldown are simulator-wide operations, which could
cause activity across separate RubySystem instances, depending on the
simulated systems' organizations. Second, some variables that are currently
static in the RubySystem cannot be. For example, cache block size could be
different between two separate RubySystems. Such variables should likely be
instance variables.

So, it is likely that there are parts of the RubySystem that should be
specific to each instance, and there are likely parts that should still be
global. Does that mean we'd need to pass a local RubySystem pointer to each
controller? If we improve the variable scoping, it should be less likely we
need local pointers, but it's still unclear. For now, I expect that keeping
the code consistent (i.e. just using the existing g_system_ptr) will make
it the most extensible for adding the multiple RubySystem functionality
later.


  Joel



 
 From: gem5-dev [gem5-dev-boun...@gem5.org] on behalf of Nilay Vaish [
 ni...@cs.wisc.edu]
 Sent: Tuesday, April 14, 2015 11:33 AM
 To: gem5 Developer List
 Subject: Re: [gem5-dev] changeset in gem5: ruby: allow restoring from
 checkpoint when us...

 On Tue, 14 Apr 2015, Joel Hestness wrote:

  This patch didn't address my requested change (
  http://reviews.gem5.org/r/2702/), and shouldn't have been checked in.
 The
  AbstractController should not have or even need another pointer to the
  RubySystem, because it can access it with g_system_ptr. This is a dead
  simple fix. Adding the pointer is going in the wrong direction for
  abstraction.
 

 Joel, in my opinion we should be eliminating global / static variables.
 I have personally found them to be a problem while trying to parallelize
 Ruby.

 --
 Nilay
 ___
 gem5-dev mailing list
 gem5-dev@gem5.org
 http://m5sim.org/mailman/listinfo/gem5-dev
 ___
 gem5-dev mailing list
 gem5-dev@gem5.org
 http://m5sim.org/mailman/listinfo/gem5-dev



-- 
  Joel Hestness
  PhD Candidate, Computer Architecture
  Dept. of Computer Science, University of Wisconsin - Madison
  http://pages.cs.wisc.edu/~hestness/
___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev


Re: [gem5-dev] changeset in gem5: ruby: allow restoring from checkpoint when us...

2015-04-14 Thread Nilay Vaish

On Tue, 14 Apr 2015, Joel Hestness wrote:


Hi Nilay,

In this case, I opened an issue on this patch that I wanted to see fixed,
and I even went so far as to propose what I still view as a more
appropriate solution. My request wasn't even addressed. This is very
different than no one commenting on a review request and you checking it in.

I read nearly all Ruby review requests. I rarely comment, either because
I'm fine with the concept of the patch, or I don't have the expertise to
quickly understand the changes to the code. I can offer to increase my
vocalization in reviews, but we have to make sure we use appropriate
process when deciding to check in a patch. If an issue has been opened, it
needs to be addressed.

I hope we all agree that executive decisions can create contention and that
we should avoid them.


I am guessing the author decided not to take any action.  I could have 
done what you suggested, but I did not feel the requirement.



There are two strong reasons not to use local pointers. First, you don't
need a local pointer if there is a global one (unless using the global one
results in a major performance penalty, which is unlikely here). It results
in unnecessary object instance bloat. Second, it is inconsistent with other
uses of the RubySystem, which almost exclusively use g_system_ptr. There
are 11 existing Ruby system classes that use g_system_ptr to access the
RubySystem, and basically all generated controllers (including the
AbstractControllers!) do also.

The only way I can be assuaged is to hear a clearer explanation of why the
AbstractControllers *should* have their own pointers in this patch. At this
point, I see no reason.



If you feel the need, just go ahead and revert the patch and / or commit 
one that uses the global pointer.  I am not opposed to using the global 
pointer, but most likely would not do so myself.


--
Nilay
___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev


Re: [gem5-dev] Destructors not called ?

2015-04-14 Thread Steve Reinhardt
What are you trying to accomplish?  If you just need to do something when
the simulation terminates, you can use registerExitCallback(), declared in
sim/core.hh.

(And apparently also in sim/sim_exit.hh; I'm not sure why it's declared
twice.)

Steve

On Tue, Apr 14, 2015 at 5:10 AM, VAUMOURIN Grégory gregory.vaumou...@cea.fr
 wrote:

 Thanks for the helpfull answer \o/ I'll see what I can do !

 --
 Gregory Vaumourin
 ___
 gem5-dev mailing list
 gem5-dev@gem5.org
 http://m5sim.org/mailman/listinfo/gem5-dev

___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev


Re: [gem5-dev] changeset in gem5: ruby: allow restoring from checkpoint when us...

2015-04-14 Thread Nilay Vaish

On Tue, 14 Apr 2015, Joel Hestness wrote:


Hi Nilay,

So, I have a couple issues with this:
1) You didn't raise this opinion in the review itself, but instead made
the decision without seeking consensus.


The patch itself is not at variance with what I might have done to address 
the issue.  Since no one else currently commits patches meant for ruby, I 
tend to decide on my own what should be done.  Along with this patch, I 
committed three other patches that were not discussed at all.  Again, in 
all such cases, I do what I feel should be done.



2) Duplicating a global pointer as members within other objects does not
change the way that the pointers need to be handled when parallelizing
Ruby. If there is a shared data member within the RubySystem that needs to
be locked to ensure consistency, you'll need lock that variable no matter
how other objects access the RubySystem. Further, the RubySystem global
pointer is set exactly once in the RubySystem constructor and never allowed
to be changed again. Therefore, there should be no worries about races on
the global pointer itself, AND distributing RubySystem pointers into other
objects is more likely to generate potential races if there is ever a need
to update those pointers. Thus, I strongly disagree with your opinion on
distributing pointers into the AbstractControllers.

 Am I missing something here?



You can use g_system_ptr, or you can make a local copy of the pointer. 
To me both ways are equally fine, neither seems better. It is just that I 
prefer not using global variables, so I probably would have gone the same 
way as the patch does.


--
Nilay
___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev


[gem5-dev] Mem Request Assertion fail [VALID_VADDR]

2015-04-14 Thread Vinayak Bhargav Srinath
Hi ,

I'm trying to access the Virtual Address of the packets to *handleFill()*
and *access()*  methods using *pkt-req-getVaddr()*
Seems that some of the packets don't have Virtual Addresses.. If this is
the case, then how can I access the Virtual address of a particular data
being written into the cache.
I just want the Virtual Addresses of data flowing into the dcache (for
allocating cache blocks). I'm running in syscall emulation mode using the
benchmark x264.

I think I managed to statically compile x264 without pthreads as it does
not throw me the static binary required error.
--
Here's my run command:
build/X86/gem5.opt --debug-file=Cache.dbg --debug-flags=Cache
--outdir=m5out/x264 --redirect-stdout
--stdout-file=afb_0_n8T_0_cvdd_700.out --redirect-stderr
--stderr-file=afb_0_n8T_0_cvdd_700.err
--stats-file=afb_0_n8T_0_cvdd_700.stats ./configs/example/se.py
--cpu-type=detailed --caches --l2cache --num-cpus=1 --cpu-clock=1GHz
--l1d_size=32kB --l1i_size=32kB --l1d_assoc=4 --l1i_assoc=4 --l2_size=256kB
--l2_assoc=8 --mem-type=LPDDR3_1600_x32 --mem-size=512MB
--cacheline_size=64 -c /home/srina005/x264-snapshot-20141218-2245/x264 -o 
--qp 0 -o m5out/X86_Hybrid_8T_6T_LE_BFE/x264/out.264
m5out/x264/input/akiyo_cif.yuv --input-res 352x288


and this is my error message:

0: system.remote_gdb.listener: listening for remote gdb #0 on port 7020
yuv [info]: 352x288p 0:0 @ 25/1 fps (cfr)
warn: ignoring syscall rt_sigaction(2, ...)
x264 [info]: using cpu capabilities: none!
*gem5.opt: build/X86/mem/request.hh:509: Addr Request::getVaddr() const:
Assertion `privateFlags.isSet(VALID_VADDR)' failed.*
Program aborted at tick 6230504500


I tested it on hello world. That seems to work fine..

-- 
Thanks and Regards,
*Vinayak Bhargav Srinath*
*Ph: +1-612-987-9211*
___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev


Re: [gem5-dev] changeset in gem5: ruby: allow restoring from checkpoint when us...

2015-04-14 Thread Potter, Brandon
Hello Joel,

Not looking to pick sides here, but I have also encountered problems with 
global/static variables in Ruby as well.  This past December, I spent a couple 
of weeks trying to clean up the Ruby code so that we could invoke two instances 
of Ruby at the same time in the simulator.  It was not trivial as the problems 
extended into slicc auto-generated code as well and I ended up temporarily 
abandoning it.  The high level code organization and design decisions should 
not prevent us from doing simple straight-forward things like invoking two Ruby 
instances at the same time.

When I was trying to solve the problem, I independently followed the same 
method as Nilay.  I added ruby system pointers to most of the objects so that 
they could back-reference the system object (since it's the central point of 
that Ruby instance) and obtain access to the other subsidiary objects.  I felt 
really dirty while writing the code and frankly the code was painful to look 
at, but there didn't appear to be a better method (at least not one that I 
could see).  A couple of folks that I spoke with here at AMD felt that the 
global/static variables needed to be fixed, but agreed that there was no easy 
solution.

Regards,
Brandon

From: gem5-dev [gem5-dev-boun...@gem5.org] on behalf of Nilay Vaish 
[ni...@cs.wisc.edu]
Sent: Tuesday, April 14, 2015 11:33 AM
To: gem5 Developer List
Subject: Re: [gem5-dev] changeset in gem5: ruby: allow restoring from 
checkpoint when us...

On Tue, 14 Apr 2015, Joel Hestness wrote:

 This patch didn't address my requested change (
 http://reviews.gem5.org/r/2702/), and shouldn't have been checked in. The
 AbstractController should not have or even need another pointer to the
 RubySystem, because it can access it with g_system_ptr. This is a dead
 simple fix. Adding the pointer is going in the wrong direction for
 abstraction.


Joel, in my opinion we should be eliminating global / static variables.
I have personally found them to be a problem while trying to parallelize
Ruby.

--
Nilay
___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev
___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev


Re: [gem5-dev] changeset in gem5: ruby: allow restoring from checkpoint when us...

2015-04-14 Thread Joel Hestness
Hi Nilay,

On Tue, Apr 14, 2015 at 4:33 PM, Nilay Vaish ni...@cs.wisc.edu wrote:

 On Tue, 14 Apr 2015, Joel Hestness wrote:

  Hi Nilay,

 So, I have a couple issues with this:
 1) You didn't raise this opinion in the review itself, but instead made
 the decision without seeking consensus.


 The patch itself is not at variance with what I might have done to address
 the issue.  Since no one else currently commits patches meant for ruby, I
 tend to decide on my own what should be done.  Along with this patch, I
 committed three other patches that were not discussed at all.  Again, in
 all such cases, I do what I feel should be done.


I understand the desire to shuttle patches through the review process with
a slim process. However, the executive decision on this patch was out of
line. The gem5 commit access page (http://gem5.org/Commit_Access)
specifically states:


- Your reviews may request some changes; if so, make those and (unless
the changes are trivial) re-post the modified patch for review, updating
the existing review as described above.


and, at the end of the page:

 A patch can be committed after it has been on the reviewboard for 10 days
 if no one has commented on it or asked for you to wait.


In this case, I opened an issue on this patch that I wanted to see fixed,
and I even went so far as to propose what I still view as a more
appropriate solution. My request wasn't even addressed. This is very
different than no one commenting on a review request and you checking it in.

I read nearly all Ruby review requests. I rarely comment, either because
I'm fine with the concept of the patch, or I don't have the expertise to
quickly understand the changes to the code. I can offer to increase my
vocalization in reviews, but we have to make sure we use appropriate
process when deciding to check in a patch. If an issue has been opened, it
needs to be addressed.

I hope we all agree that executive decisions can create contention and that
we should avoid them.



 2) Duplicating a global pointer as members within other objects does not
 change the way that the pointers need to be handled when parallelizing
 Ruby. If there is a shared data member within the RubySystem that needs to
 be locked to ensure consistency, you'll need lock that variable no matter
 how other objects access the RubySystem. Further, the RubySystem global
 pointer is set exactly once in the RubySystem constructor and never
 allowed
 to be changed again. Therefore, there should be no worries about races on
 the global pointer itself, AND distributing RubySystem pointers into other
 objects is more likely to generate potential races if there is ever a need
 to update those pointers. Thus, I strongly disagree with your opinion on
 distributing pointers into the AbstractControllers.

  Am I missing something here?


 You can use g_system_ptr, or you can make a local copy of the pointer. To
 me both ways are equally fine, neither seems better. It is just that I
 prefer not using global variables, so I probably would have gone the same
 way as the patch does.


There are two strong reasons not to use local pointers. First, you don't
need a local pointer if there is a global one (unless using the global one
results in a major performance penalty, which is unlikely here). It results
in unnecessary object instance bloat. Second, it is inconsistent with other
uses of the RubySystem, which almost exclusively use g_system_ptr. There
are 11 existing Ruby system classes that use g_system_ptr to access the
RubySystem, and basically all generated controllers (including the
AbstractControllers!) do also.

The only way I can be assuaged is to hear a clearer explanation of why the
AbstractControllers *should* have their own pointers in this patch. At this
point, I see no reason.

  Joel


-- 
  Joel Hestness
  PhD Candidate, Computer Architecture
  Dept. of Computer Science, University of Wisconsin - Madison
  http://pages.cs.wisc.edu/~hestness/
___
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev