[m5-dev] Cron m5test@zizzer /z/m5/regression/do-regression quick
* build/ALPHA_SE/tests/opt/quick/20.eio-short/alpha/eio/simple-timing passed. * build/ALPHA_SE/tests/opt/quick/60.rubytest/alpha/linux/rubytest-ruby passed. * build/ALPHA_SE/tests/opt/quick/20.eio-short/alpha/eio/simple-atomic passed. * build/ALPHA_SE/tests/opt/quick/00.hello/alpha/tru64/simple-atomic passed. * build/ALPHA_SE/tests/opt/quick/00.hello/alpha/tru64/o3-timing passed. * build/ALPHA_SE/tests/opt/quick/30.eio-mp/alpha/eio/simple-timing-mp passed. * build/ALPHA_SE/tests/opt/quick/30.eio-mp/alpha/eio/simple-atomic-mp passed. * build/ALPHA_SE/tests/opt/quick/00.hello/alpha/linux/simple-timing-ruby passed. * build/ALPHA_SE/tests/opt/quick/00.hello/alpha/tru64/simple-timing-ruby passed. * build/ALPHA_SE/tests/opt/quick/00.hello/alpha/linux/simple-timing passed. * build/ALPHA_SE/tests/opt/quick/01.hello-2T-smt/alpha/linux/o3-timing passed. * build/ALPHA_SE/tests/opt/quick/00.hello/alpha/linux/inorder-timing passed. * build/ALPHA_SE/tests/opt/quick/00.hello/alpha/tru64/simple-timing passed. * build/ALPHA_SE/tests/opt/quick/00.hello/alpha/linux/o3-timing passed. * build/ALPHA_SE/tests/opt/quick/00.hello/alpha/linux/simple-atomic passed. * build/ALPHA_SE_MOESI_hammer/tests/opt/quick/60.rubytest/alpha/linux/rubytest-ruby-MOESI_hammer passed. * build/ALPHA_SE_MOESI_hammer/tests/opt/quick/00.hello/alpha/tru64/simple-timing-ruby-MOESI_hammer passed. * build/ALPHA_SE_MOESI_hammer/tests/opt/quick/00.hello/alpha/linux/simple-timing-ruby-MOESI_hammer passed. * build/ALPHA_SE_MESI_CMP_directory/tests/opt/quick/60.rubytest/alpha/linux/rubytest-ruby-MESI_CMP_directory passed. * build/ALPHA_SE/tests/opt/quick/50.memtest/alpha/linux/memtest-ruby passed. * build/ALPHA_SE_MESI_CMP_directory/tests/opt/quick/00.hello/alpha/tru64/simple-timing-ruby-MESI_CMP_directory passed. * build/ALPHA_SE_MESI_CMP_directory/tests/opt/quick/00.hello/alpha/linux/simple-timing-ruby-MESI_CMP_directory passed. * build/ALPHA_SE_MOESI_CMP_directory/tests/opt/quick/60.rubytest/alpha/linux/rubytest-ruby-MOESI_CMP_directory passed. * build/ALPHA_SE_MOESI_CMP_directory/tests/opt/quick/00.hello/alpha/tru64/simple-timing-ruby-MOESI_CMP_directory passed. * build/ALPHA_SE_MOESI_CMP_directory/tests/opt/quick/00.hello/alpha/linux/simple-timing-ruby-MOESI_CMP_directory passed. * build/ALPHA_SE/tests/opt/quick/50.memtest/alpha/linux/memtest passed. * build/ALPHA_SE_MOESI_CMP_token/tests/opt/quick/60.rubytest/alpha/linux/rubytest-ruby-MOESI_CMP_token passed. * build/ALPHA_SE_MOESI_CMP_token/tests/opt/quick/00.hello/alpha/tru64/simple-timing-ruby-MOESI_CMP_token passed. * build/ALPHA_SE_MOESI_CMP_token/tests/opt/quick/00.hello/alpha/linux/simple-timing-ruby-MOESI_CMP_token passed. * build/ALPHA_SE_MOESI_hammer/tests/opt/quick/50.memtest/alpha/linux/memtest-ruby-MOESI_hammer passed. * build/ALPHA_FS/tests/opt/quick/10.linux-boot/alpha/linux/tsunami-simple-timing passed. * build/ALPHA_FS/tests/opt/quick/10.linux-boot/alpha/linux/tsunami-simple-timing-dual passed. * build/ALPHA_FS/tests/opt/quick/10.linux-boot/alpha/linux/tsunami-simple-atomic-dual passed. * build/ALPHA_FS/tests/opt/quick/10.linux-boot/alpha/linux/tsunami-simple-atomic passed. * build/MIPS_SE/tests/opt/quick/00.hello/mips/linux/simple-atomic passed. * build/MIPS_SE/tests/opt/quick/00.hello/mips/linux/simple-timing-ruby passed. * build/MIPS_SE/tests/opt/quick/00.hello/mips/linux/simple-timing passed. * build/MIPS_SE/tests/opt/quick/00.hello/mips/linux/o3-timing passed. * build/ALPHA_FS/tests/opt/quick/80.netperf-stream/alpha/linux/twosys-tsunami-simple-atomic passed. * build/MIPS_SE/tests/opt/quick/00.hello/mips/linux/inorder-timing passed. * build/POWER_SE/tests/opt/quick/00.hello/power/linux/o3-timing passed. * build/POWER_SE/tests/opt/quick/00.hello/power/linux/simple-atomic passed. * build/ALPHA_SE_MOESI_CMP_token/tests/opt/quick/50.memtest/alpha/linux/memtest-ruby-MOESI_CMP_token passed. * build/SPARC_SE/tests/opt/quick/02.insttest/sparc/linux/simple-atomic passed. * build/SPARC_SE/tests/opt/quick/02.insttest/sparc/linux/o3-timing passed. * build/SPARC_SE/tests/opt/quick/00.hello/sparc/linux/simple-atomic passed. * build/SPARC_SE/tests/opt/quick/40.m5threads-test-atomic/sparc/linux/simple-timing-mp passed. * build/SPARC_SE/tests/opt/quick/00.hello/sparc/linux/simple-timing-ruby passed. * build/SPARC_SE/tests/opt/quick/40.m5threads-test-atomic/sparc/linux/o3-timing-mp passed. * build/SPARC_SE/tests/opt/quick/02.insttest/sparc/linux/simple-timing passed. * build/SPARC_SE/tests/opt/quick/00.hello/sparc/linux/simple-timing passed. * build/SPARC_SE/tests/opt/quick/40.m5threads-test-atomic/sparc/linux/simple-atomic-mp passed. * build/X86_SE/tests/opt/quick/00.hello/x86/linux/simple-timing-ruby passed. *
[m5-dev] Review Request: SConstruct: automatically update .hg/hgrc with style hooks
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/668/ --- Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- SConstruct: automatically update .hg/hgrc with style hooks Seems easier than pestering people about it. Note also that path is now absolute, so you don't get errors when invoking hg from subdirectories. Diffs - SConstruct 66a3187a6714 Diff: http://reviews.m5sim.org/r/668/diff Testing --- Thanks, Steve ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
[m5-dev] changeset in m5: scons: allow use of current builds as default b...
changeset 06f3a4cbd585 in /z/repo/m5
details: http://repo.m5sim.org/m5?cmd=changeset;node=06f3a4cbd585
description:
scons: allow use of current builds as default build settings
Currently the --default= option only looks at the predefined
build configs (in m5/build_opts), so you're limited to basing
a new build config off of those (ALPHA_SE, etc.). If you've
already defined a non-standard build config and want to clone
it or tweak it, you have to start from scratch. This patch
causes --default= to look first among the existing builds
(in build/variables) before looking in build_opts so you
can specify an existing non-standard build config as a
starting point for a new config.
diffstat:
SConstruct | 20 +---
1 files changed, 13 insertions(+), 7 deletions(-)
diffs (35 lines):
diff -r 66a3187a6714 -r 06f3a4cbd585 SConstruct
--- a/SConstructMon May 02 00:16:14 2011 -0400
+++ b/SConstructMon May 02 12:40:31 2011 -0700
@@ -967,18 +967,24 @@
# Get default build variables from source tree. Variables are
# normally determined by name of $VARIANT_DIR, but can be
-# overriden by 'default=' arg on command line.
+# overridden by '--default=' arg on command line.
default = GetOption('default')
-if not default:
-default = variant_dir
-default_vars_file = joinpath('build_opts', default)
-if isfile(default_vars_file):
+opts_dir = joinpath(main.root.abspath, 'build_opts')
+if default:
+default_vars_files = [joinpath(build_root, 'variables', default),
+ joinpath(opts_dir, default)]
+else:
+default_vars_files = [joinpath(opts_dir, variant_dir)]
+existing_files = filter(isfile, default_vars_files)
+if existing_files:
+default_vars_file = existing_files[0]
sticky_vars.files.append(default_vars_file)
print Variables file %s not found,\n using defaults in %s \
% (current_vars_file, default_vars_file)
else:
-print Error: cannot find variables file %s or %s \
- % (current_vars_file, default_vars_file)
+print Error: cannot find variables file %s or \
+ default file(s) %s \
+ % (current_vars_file, ' or '.join(default_vars_files))
Exit(1)
# Apply current variable settings to env
___
m5-dev mailing list
m5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/m5-dev
[m5-dev] changeset in m5: scons: interpret paths relative to launch direc...
changeset 3f49ed206f46 in /z/repo/m5
details: http://repo.m5sim.org/m5?cmd=changeset;node=3f49ed206f46
description:
scons: interpret paths relative to launch directory
Make sure all command-line targets and EXTRAS directories
are interpreted relative to the launch directory. This
turns out to be very useful when building code from an
EXTRAS directory using SCons's -C option.
We were trying to do this with targets but it didn't actually
work since we didn't update BUILD_TARGETS (so SCons got
confused internally). We weren't even trying with EXTRAS.
To simplify the code, the default target is also interpreted
relative to the launch dir even though it was explicitly
handled as relative to the m5 dir before... I doubt anyone
really uses this anyway so it didn't seem worth the complexity.
(Maybe we should get rid of it?)
diffstat:
SConstruct | 52 +++-
1 files changed, 19 insertions(+), 33 deletions(-)
diffs (102 lines):
diff -r 06f3a4cbd585 -r 3f49ed206f46 SConstruct
--- a/SConstructMon May 02 12:40:31 2011 -0700
+++ b/SConstructMon May 02 12:40:32 2011 -0700
@@ -264,30 +264,29 @@
return i
raise ValueError, element not found
+# Take a list of paths (or SCons Nodes) and return a list with all
+# paths made absolute and ~-expanded. Paths will be interpreted
+# relative to the launch directory unless a different root is provided
+def makePathListAbsolute(path_list, root=GetLaunchDir()):
+return [abspath(joinpath(root, expanduser(str(p
+for p in path_list]
+
# Each target must have 'build' in the interior of the path; the
# directory below this will determine the build parameters. For
# example, for target 'foo/bar/build/ALPHA_SE/arch/alpha/blah.do' we
# recognize that ALPHA_SE specifies the configuration because it
-# follow 'build' in the bulid path.
+# follow 'build' in the build path.
-# Generate absolute paths to targets so we can see where the build dir is
-if COMMAND_LINE_TARGETS:
-# Ask SCons which directory it was invoked from
-launch_dir = GetLaunchDir()
-# Make targets relative to invocation directory
-abs_targets = [ normpath(joinpath(launch_dir, str(x))) for x in \
-COMMAND_LINE_TARGETS]
-else:
-# Default targets are relative to root of tree
-abs_targets = [ normpath(joinpath(main.root.abspath, str(x))) for x in \
-DEFAULT_TARGETS]
-
+# The funky assignment to [:] is needed to replace the list contents
+# in place rather than reassign the symbol to a new list, which
+# doesn't work (obviously!).
+BUILD_TARGETS[:] = makePathListAbsolute(BUILD_TARGETS)
# Generate a list of the unique build roots and configs that the
# collected targets reference.
variant_paths = []
build_root = None
-for t in abs_targets:
+for t in BUILD_TARGETS:
path_dirs = t.split('/')
try:
build_top = rfind(path_dirs, 'build', -2)
@@ -326,21 +325,6 @@
# tree (not specific to a particular build like ALPHA_SE)
#
-# Variable validators converters for global sticky variables
-def PathListMakeAbsolute(val):
-if not val:
-return val
-f = lambda p: abspath(expanduser(p))
-return ':'.join(map(f, val.split(':')))
-
-def PathListAllExist(key, val, env):
-if not val:
-return
-paths = val.split(':')
-for path in paths:
-if not isdir(path):
-raise SCons.Errors.UserError(Path does not exist: '%s' % path)
-
global_vars_file = joinpath(build_root, 'variables.global')
global_vars = Variables(global_vars_file, args=ARGUMENTS)
@@ -351,8 +335,7 @@
('BATCH', 'Use batch pool for build and tests', False),
('BATCH_CMD', 'Batch pool submission command name', 'qdo'),
('M5_BUILD_CACHE', 'Cache built objects in this directory', False),
-('EXTRAS', 'Add extra directories to the compilation', '',
- PathListAllExist, PathListMakeAbsolute),
+('EXTRAS', 'Add extra directories to the compilation', '')
)
# Update main environment with values from ARGUMENTS global_vars_file
@@ -363,10 +346,10 @@
global_vars.Save(global_vars_file, main)
# Parse EXTRAS variable to build list of all directories where we're
-# look for sources etc. This list is exported as base_dir_list.
+# look for sources etc. This list is exported as extras_dir_list.
base_dir = main.srcdir.abspath
if main['EXTRAS']:
-extras_dir_list = main['EXTRAS'].split(':')
+extras_dir_list = makePathListAbsolute(main['EXTRAS'].split(':'))
else:
extras_dir_list = []
@@ -808,6 +791,9 @@
# Walk the tree and execute all SConsopts scripts that wil add to the
# above variables
for bdir in [ base_dir ] + extras_dir_list:
+if not isdir(bdir):
+print Error: directory '%s' does not exist % bdir
+Exit(1)
for root, dirs, files in os.walk(bdir):
if
Re: [m5-dev] Review Request: SConstruct: automatically update .hg/hgrc with style hooks
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/668/#review1173 --- SConstruct http://reviews.m5sim.org/r/668/#comment1609 I know you didn't change this, but should we check for both hooks? (I added the pre-qrefresh one a while ago) SConstruct http://reviews.m5sim.org/r/668/#comment1610 Should we prompt the user for permission? That way the user would at least know that something happened. - Nathan On 2011-05-02 12:34:56, Steve Reinhardt wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/668/ --- (Updated 2011-05-02 12:34:56) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- SConstruct: automatically update .hg/hgrc with style hooks Seems easier than pestering people about it. Note also that path is now absolute, so you don't get errors when invoking hg from subdirectories. Diffs - SConstruct 66a3187a6714 Diff: http://reviews.m5sim.org/r/668/diff Testing --- Thanks, Steve ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: SConstruct: automatically update .hg/hgrc with style hooks
On 2011-05-02 12:51:56, Nathan Binkert wrote: SConstruct, line 243 http://reviews.m5sim.org/r/668/diff/1/?file=12211#file12211line243 I know you didn't change this, but should we check for both hooks? (I added the pre-qrefresh one a while ago) Doesn't make me much difference... seems pretty unlikely anyone would have one and not the other though. On 2011-05-02 12:51:56, Nathan Binkert wrote: SConstruct, line 247 http://reviews.m5sim.org/r/668/diff/1/?file=12211#file12211line247 Should we prompt the user for permission? That way the user would at least know that something happened. I doubt most people care. How many would really say no? Most people just want things to be automatic. If they really do care they can always edit it out later (the comments will let them know where the hooks came from). - Steve --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/668/#review1173 --- On 2011-05-02 12:34:56, Steve Reinhardt wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/668/ --- (Updated 2011-05-02 12:34:56) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- SConstruct: automatically update .hg/hgrc with style hooks Seems easier than pestering people about it. Note also that path is now absolute, so you don't get errors when invoking hg from subdirectories. Diffs - SConstruct 66a3187a6714 Diff: http://reviews.m5sim.org/r/668/diff Testing --- Thanks, Steve ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: SConstruct: automatically update .hg/hgrc with style hooks
On 2011-05-02 12:51:56, Nathan Binkert wrote: SConstruct, line 247 http://reviews.m5sim.org/r/668/diff/1/?file=12211#file12211line247 Should we prompt the user for permission? That way the user would at least know that something happened. Steve Reinhardt wrote: I doubt most people care. How many would really say no? Most people just want things to be automatic. If they really do care they can always edit it out later (the comments will let them know where the hooks came from). I agree that nobody would say no, but most people also would never notice that it had happened because the message would just fly by if there was no prompt. I guess that you're arguing that that would be a good thing :) On 2011-05-02 12:51:56, Nathan Binkert wrote: SConstruct, line 243 http://reviews.m5sim.org/r/668/diff/1/?file=12211#file12211line243 I know you didn't change this, but should we check for both hooks? (I added the pre-qrefresh one a while ago) Steve Reinhardt wrote: Doesn't make me much difference... seems pretty unlikely anyone would have one and not the other though. Old repos may not have the new hook. (I only found out that we could use pre-qrefresh a few months ago). - Nathan --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/668/#review1173 --- On 2011-05-02 12:34:56, Steve Reinhardt wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/668/ --- (Updated 2011-05-02 12:34:56) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- SConstruct: automatically update .hg/hgrc with style hooks Seems easier than pestering people about it. Note also that path is now absolute, so you don't get errors when invoking hg from subdirectories. Diffs - SConstruct 66a3187a6714 Diff: http://reviews.m5sim.org/r/668/diff Testing --- Thanks, Steve ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: SConstruct: automatically update .hg/hgrc with style hooks
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/668/#review1176 --- Ship it! Whether you decide to change this or not, it's a good idea. - Nathan On 2011-05-02 12:34:56, Steve Reinhardt wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/668/ --- (Updated 2011-05-02 12:34:56) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- SConstruct: automatically update .hg/hgrc with style hooks Seems easier than pestering people about it. Note also that path is now absolute, so you don't get errors when invoking hg from subdirectories. Diffs - SConstruct 66a3187a6714 Diff: http://reviews.m5sim.org/r/668/diff Testing --- Thanks, Steve ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: SConstruct: automatically update .hg/hgrc with style hooks
On 2011-05-02 12:51:56, Nathan Binkert wrote: SConstruct, line 243 http://reviews.m5sim.org/r/668/diff/1/?file=12211#file12211line243 I know you didn't change this, but should we check for both hooks? (I added the pre-qrefresh one a while ago) Steve Reinhardt wrote: Doesn't make me much difference... seems pretty unlikely anyone would have one and not the other though. Nathan Binkert wrote: Old repos may not have the new hook. (I only found out that we could use pre-qrefresh a few months ago). The only hitch is that if we just append the hook text when only one is missing we'll get duplicates, like this: [extensions] style = /home/stever/hg/amd/m5/util/style.py [hooks] pretxncommit.style = python:style.check_style # The following lines were automatically added by m5/SConstruct # to provide the m5 style-checking hooks [extensions] style = /home/stever/hg/amd/m5/util/style.py [hooks] pretxncommit.style = python:style.check_style pre-qrefresh.style = python:style.check_style # End of SConstruct additions I don't think it's a problem (my very limited testing hasn't shown anything), but it is a little weird. If you think this is OK, then I'll go ahead and test for both. Otherwise I could test for both, print a warning if they're only half there, and only do the auto append if they're both missing. - Steve --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/668/#review1173 --- On 2011-05-02 12:34:56, Steve Reinhardt wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/668/ --- (Updated 2011-05-02 12:34:56) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- SConstruct: automatically update .hg/hgrc with style hooks Seems easier than pestering people about it. Note also that path is now absolute, so you don't get errors when invoking hg from subdirectories. Diffs - SConstruct 66a3187a6714 Diff: http://reviews.m5sim.org/r/668/diff Testing --- Thanks, Steve ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: SConstruct: automatically update .hg/hgrc with style hooks
On 2011-05-02 12:51:56, Nathan Binkert wrote: SConstruct, line 247 http://reviews.m5sim.org/r/668/diff/1/?file=12211#file12211line247 Should we prompt the user for permission? That way the user would at least know that something happened. Steve Reinhardt wrote: I doubt most people care. How many would really say no? Most people just want things to be automatic. If they really do care they can always edit it out later (the comments will let them know where the hooks came from). Nathan Binkert wrote: I agree that nobody would say no, but most people also would never notice that it had happened because the message would just fly by if there was no prompt. I guess that you're arguing that that would be a good thing :) Changing config files behind peoples back is a really bad idea in my opinion. I know I've stopped using entire distros (Suse) because they mucked with config files behind my back and perpetually broke my system. My configs are mine, and the minimal level of respect for that would be if we asked permission before we let ourselves in. I think leaving well enough alone and having the user go in and fix it themselves is actually the best approach. - Gabe --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/668/#review1173 --- On 2011-05-02 12:34:56, Steve Reinhardt wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/668/ --- (Updated 2011-05-02 12:34:56) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- SConstruct: automatically update .hg/hgrc with style hooks Seems easier than pestering people about it. Note also that path is now absolute, so you don't get errors when invoking hg from subdirectories. Diffs - SConstruct 66a3187a6714 Diff: http://reviews.m5sim.org/r/668/diff Testing --- Thanks, Steve ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
[m5-dev] Review Request: CPU: Fix a case where timing simple cpu faults can nest.
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/670/ --- Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- CPU: Fix a case where timing simple cpu faults can nest. If we fault, change the state to faulting so that we don't fault again in the same cycle. Diffs - src/cpu/simple/base.hh 3f49ed206f46 src/cpu/simple/timing.cc 3f49ed206f46 Diff: http://reviews.m5sim.org/r/670/diff Testing --- Thanks, Ali ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
[m5-dev] Review Request: ARM: Add vfpv3 support to native trace.
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/669/ --- Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- ARM: Add vfpv3 support to native trace. Diffs - src/arch/arm/nativetrace.hh 3f49ed206f46 src/arch/arm/nativetrace.cc 3f49ed206f46 util/statetrace/arch/arm/tracechild.hh 3f49ed206f46 util/statetrace/arch/arm/tracechild.cc 3f49ed206f46 Diff: http://reviews.m5sim.org/r/669/diff Testing --- Thanks, Ali ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
[m5-dev] Review Request: CPU: Add some useful debug message to the timing simple cpu.
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/671/ --- Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- CPU: Add some useful debug message to the timing simple cpu. Diffs - src/cpu/simple/timing.cc 3f49ed206f46 Diff: http://reviews.m5sim.org/r/671/diff Testing --- Thanks, Ali ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
[m5-dev] Review Request: Debug: Add a function to cause the simulator to create a checkpoint from GDB.
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/672/ --- Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- Debug: Add a function to cause the simulator to create a checkpoint from GDB. Diffs - src/sim/debug.hh 3f49ed206f46 src/sim/debug.cc 3f49ed206f46 Diff: http://reviews.m5sim.org/r/672/diff Testing --- Thanks, Ali ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: SConstruct: automatically update .hg/hgrc with style hooks
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/668/#review1180 --- Ship it! - Ali On 2011-05-02 12:34:56, Steve Reinhardt wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/668/ --- (Updated 2011-05-02 12:34:56) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- SConstruct: automatically update .hg/hgrc with style hooks Seems easier than pestering people about it. Note also that path is now absolute, so you don't get errors when invoking hg from subdirectories. Diffs - SConstruct 66a3187a6714 Diff: http://reviews.m5sim.org/r/668/diff Testing --- Thanks, Steve ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: Debug: Add a function to cause the simulator to create a checkpoint from GDB.
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/672/#review1182 --- Ship it! Looks fine. src/sim/debug.hh http://reviews.m5sim.org/r/672/#comment1620 Maybe put a note that the definition is in eventq.cc? - Nathan On 2011-05-02 15:42:09, Ali Saidi wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/672/ --- (Updated 2011-05-02 15:42:09) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- Debug: Add a function to cause the simulator to create a checkpoint from GDB. Diffs - src/sim/debug.hh 3f49ed206f46 src/sim/debug.cc 3f49ed206f46 Diff: http://reviews.m5sim.org/r/672/diff Testing --- Thanks, Ali ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: ARM: Add vfpv3 support to native trace.
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/669/#review1181 --- src/arch/arm/nativetrace.cc http://reviews.m5sim.org/r/669/#comment1619 You should divide by two here instead of shifting by 1. It's more obvious what you're doing, and the compiler will be smart enough to use a shift it it's faster. src/arch/arm/nativetrace.cc http://reviews.m5sim.org/r/669/#comment1618 spaces around the + util/statetrace/arch/arm/tracechild.cc http://reviews.m5sim.org/r/669/#comment1621 Hmm... I wonder how that got there? Good catch. util/statetrace/arch/arm/tracechild.cc http://reviews.m5sim.org/r/669/#comment1622 I don't know how easy this would be to accommodate, but you're going to be sending a bunch of extra zeros for int regs that aren't 64 bits wide. Can you make it so you send full 64 bit values only when the source is actually 64 bits wide? util/statetrace/arch/arm/tracechild.cc http://reviews.m5sim.org/r/669/#comment1623 The idea is to verify that you're not falling off of uregs. Maybe you could do something more flexible like sizeof(myregs.uregs) / sizeof (myregs.uregs[0]). util/statetrace/arch/arm/tracechild.cc http://reviews.m5sim.org/r/669/#comment1624 The same comment applies as in getRegs, except that you have to deal with an offset. It would probably be a good idea to define something in the enum to mark the start of the FP regs. You can move the assert to after the if and subtract out the offset right before indexing fpregs. util/statetrace/arch/arm/tracechild.cc http://reviews.m5sim.org/r/669/#comment1625 Just because libc would use a macro doesn't mean we have to. You should replace this with a constant of the appropriate type. util/statetrace/arch/arm/tracechild.cc http://reviews.m5sim.org/r/669/#comment1626 Is the cast actually necessary here? I can believe it is to avoid a warning, but you could try leaving it out if you haven't already. - Gabe On 2011-05-02 15:41:28, Ali Saidi wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/669/ --- (Updated 2011-05-02 15:41:28) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- ARM: Add vfpv3 support to native trace. Diffs - src/arch/arm/nativetrace.hh 3f49ed206f46 src/arch/arm/nativetrace.cc 3f49ed206f46 util/statetrace/arch/arm/tracechild.hh 3f49ed206f46 util/statetrace/arch/arm/tracechild.cc 3f49ed206f46 Diff: http://reviews.m5sim.org/r/669/diff Testing --- Thanks, Ali ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: SConstruct: automatically update .hg/hgrc with style hooks
On 2011-05-02 12:51:56, Nathan Binkert wrote: SConstruct, line 243 http://reviews.m5sim.org/r/668/diff/1/?file=12211#file12211line243 I know you didn't change this, but should we check for both hooks? (I added the pre-qrefresh one a while ago) Steve Reinhardt wrote: Doesn't make me much difference... seems pretty unlikely anyone would have one and not the other though. Nathan Binkert wrote: Old repos may not have the new hook. (I only found out that we could use pre-qrefresh a few months ago). Steve Reinhardt wrote: The only hitch is that if we just append the hook text when only one is missing we'll get duplicates, like this: [extensions] style = /home/stever/hg/amd/m5/util/style.py [hooks] pretxncommit.style = python:style.check_style # The following lines were automatically added by m5/SConstruct # to provide the m5 style-checking hooks [extensions] style = /home/stever/hg/amd/m5/util/style.py [hooks] pretxncommit.style = python:style.check_style pre-qrefresh.style = python:style.check_style # End of SConstruct additions I don't think it's a problem (my very limited testing hasn't shown anything), but it is a little weird. If you think this is OK, then I'll go ahead and test for both. Otherwise I could test for both, print a warning if they're only half there, and only do the auto append if they're both missing. I thought about that. It's probably fine. A warning is probably worth it. Though, again, if you don't stop output for the user, nobody will notice it, so maybe it doesn't matter - Nathan --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/668/#review1173 --- On 2011-05-02 12:34:56, Steve Reinhardt wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/668/ --- (Updated 2011-05-02 12:34:56) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- SConstruct: automatically update .hg/hgrc with style hooks Seems easier than pestering people about it. Note also that path is now absolute, so you don't get errors when invoking hg from subdirectories. Diffs - SConstruct 66a3187a6714 Diff: http://reviews.m5sim.org/r/668/diff Testing --- Thanks, Steve ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: SConstruct: automatically update .hg/hgrc with style hooks
On 2011-05-02 12:51:56, Nathan Binkert wrote: SConstruct, line 247 http://reviews.m5sim.org/r/668/diff/1/?file=12211#file12211line247 Should we prompt the user for permission? That way the user would at least know that something happened. Steve Reinhardt wrote: I doubt most people care. How many would really say no? Most people just want things to be automatic. If they really do care they can always edit it out later (the comments will let them know where the hooks came from). Nathan Binkert wrote: I agree that nobody would say no, but most people also would never notice that it had happened because the message would just fly by if there was no prompt. I guess that you're arguing that that would be a good thing :) Gabe Black wrote: Changing config files behind peoples back is a really bad idea in my opinion. I know I've stopped using entire distros (Suse) because they mucked with config files behind my back and perpetually broke my system. My configs are mine, and the minimal level of respect for that would be if we asked permission before we let ourselves in. I think leaving well enough alone and having the user go in and fix it themselves is actually the best approach. This isn't a config file in your homedir, it's the config file in the M5 repo and it will only be changed if you don't have it set up right, so it's not all that bad. I have the same leaning that you do, so I think I'd rather see a prompt for the user, but I don't feel that strongly about it. - Nathan --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/668/#review1173 --- On 2011-05-02 12:34:56, Steve Reinhardt wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/668/ --- (Updated 2011-05-02 12:34:56) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- SConstruct: automatically update .hg/hgrc with style hooks Seems easier than pestering people about it. Note also that path is now absolute, so you don't get errors when invoking hg from subdirectories. Diffs - SConstruct 66a3187a6714 Diff: http://reviews.m5sim.org/r/668/diff Testing --- Thanks, Steve ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: CPU: Add some useful debug message to the timing simple cpu.
--- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/671/#review1184 --- Generally speaking, I'd like to see us move towards using the same traceflags across ISAs and CPUs. If I'm using the SimpleCPU, being able to select DPRINTFs specific to the SimpleCPU isn't that useful. I'm not going to hit an O3 DPRINTF whether I have them turned on or not. Instead, it might be better to, say, use a Fetch traceflag to go with Fetch actions. Then you can be more specific and have to remember fewer particulars of the thing you're working with. You could have a compound traceflag called SimpleCPU (or just CPU?) which would turn on all the basics. It might get a little tricky if you have multiple CPU types and wanted to turn on only the ones in the SimpleCPU, but I expect that to be relatively uncommon. I'd encourage you to think about changing things around like this, but I understand it expands the scope of what you were trying to do significantly. I'm ok if you leave it as is. - Gabe On 2011-05-02 15:41:50, Ali Saidi wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/671/ --- (Updated 2011-05-02 15:41:50) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- CPU: Add some useful debug message to the timing simple cpu. Diffs - src/cpu/simple/timing.cc 3f49ed206f46 Diff: http://reviews.m5sim.org/r/671/diff Testing --- Thanks, Ali ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: ARM: Add vfpv3 support to native trace.
On 2011-05-02 16:42:25, Gabe Black wrote: util/statetrace/arch/arm/tracechild.cc, line 151 http://reviews.m5sim.org/r/669/diff/1/?file=12215#file12215line151 Is the cast actually necessary here? I can believe it is to avoid a warning, but you could try leaving it out if you haven't already. might as well be explicit On 2011-05-02 16:42:25, Gabe Black wrote: util/statetrace/arch/arm/tracechild.cc, line 129 http://reviews.m5sim.org/r/669/diff/1/?file=12215#file12215line129 Just because libc would use a macro doesn't mean we have to. You should replace this with a constant of the appropriate type. I disagree... This will transition nicely as soon as libc gets it's act together. On 2011-05-02 16:42:25, Gabe Black wrote: util/statetrace/arch/arm/tracechild.cc, line 79 http://reviews.m5sim.org/r/669/diff/1/?file=12215#file12215line79 I don't know how easy this would be to accommodate, but you're going to be sending a bunch of extra zeros for int regs that aren't 64 bits wide. Can you make it so you send full 64 bit values only when the source is actually 64 bits wide? There is no reason to worry about sending 4 bytes down the wire. The speed issue isn't about sending 4 bytes, it's all about having to put a breakpoint after every instruction. On 2011-05-02 16:42:25, Gabe Black wrote: util/statetrace/arch/arm/tracechild.cc, line 104 http://reviews.m5sim.org/r/669/diff/1/?file=12215#file12215line104 The idea is to verify that you're not falling off of uregs. Maybe you could do something more flexible like sizeof(myregs.uregs) / sizeof (myregs.uregs[0]). uregs is never going to get smaller than it is now and I don't see a reason to come up with a crazy assert to try and prove that it isn't. On 2011-05-02 16:42:25, Gabe Black wrote: util/statetrace/arch/arm/tracechild.cc, line 111 http://reviews.m5sim.org/r/669/diff/1/?file=12215#file12215line111 The same comment applies as in getRegs, except that you have to deal with an offset. It would probably be a good idea to define something in the enum to mark the start of the FP regs. You can move the assert to after the if and subtract out the offset right before indexing fpregs. There are clearly 32 float registers defined in the enum and in the struct. The assert just verifies that we're actually accessing a floating point register when we should be. We don't need to verify the structure size it's correct by construction. On 2011-05-02 16:42:25, Gabe Black wrote: src/arch/arm/nativetrace.cc, line 123 http://reviews.m5sim.org/r/669/diff/1/?file=12213#file12213line123 You should divide by two here instead of shifting by 1. It's more obvious what you're doing, and the compiler will be smart enough to use a shift it it's faster. Personally, I like the look of the shift better, although I'm sure that the compiler can figure it out. On 2011-05-02 16:42:25, Gabe Black wrote: src/arch/arm/nativetrace.cc, line 124 http://reviews.m5sim.org/r/669/diff/1/?file=12213#file12213line124 spaces around the + done - Ali --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/669/#review1181 --- On 2011-05-02 15:41:28, Ali Saidi wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/669/ --- (Updated 2011-05-02 15:41:28) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- ARM: Add vfpv3 support to native trace. Diffs - src/arch/arm/nativetrace.hh 3f49ed206f46 src/arch/arm/nativetrace.cc 3f49ed206f46 util/statetrace/arch/arm/tracechild.hh 3f49ed206f46 util/statetrace/arch/arm/tracechild.cc 3f49ed206f46 Diff: http://reviews.m5sim.org/r/669/diff Testing --- Thanks, Ali ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: ARM: Add vfpv3 support to native trace.
On 2011-05-02 16:42:25, Gabe Black wrote: util/statetrace/arch/arm/tracechild.cc, line 79 http://reviews.m5sim.org/r/669/diff/1/?file=12215#file12215line79 I don't know how easy this would be to accommodate, but you're going to be sending a bunch of extra zeros for int regs that aren't 64 bits wide. Can you make it so you send full 64 bit values only when the source is actually 64 bits wide? Ali Saidi wrote: There is no reason to worry about sending 4 bytes down the wire. The speed issue isn't about sending 4 bytes, it's all about having to put a breakpoint after every instruction. The reason I implemented sending diffs of the state instead of sending the whole state is that what you send over the wire -does- matter, significantly. Breakpoints are slow too, but that doesn't mean everything else is irrelevant. On 2011-05-02 16:42:25, Gabe Black wrote: util/statetrace/arch/arm/tracechild.cc, line 104 http://reviews.m5sim.org/r/669/diff/1/?file=12215#file12215line104 The idea is to verify that you're not falling off of uregs. Maybe you could do something more flexible like sizeof(myregs.uregs) / sizeof (myregs.uregs[0]). Ali Saidi wrote: uregs is never going to get smaller than it is now and I don't see a reason to come up with a crazy assert to try and prove that it isn't. uregs changing size is irrelevant. That formula will be exactly right all the time and doesn't depend on the coincidence that the CPSR is last. On 2011-05-02 16:42:25, Gabe Black wrote: util/statetrace/arch/arm/tracechild.cc, line 111 http://reviews.m5sim.org/r/669/diff/1/?file=12215#file12215line111 The same comment applies as in getRegs, except that you have to deal with an offset. It would probably be a good idea to define something in the enum to mark the start of the FP regs. You can move the assert to after the if and subtract out the offset right before indexing fpregs. Ali Saidi wrote: There are clearly 32 float registers defined in the enum and in the struct. The assert just verifies that we're actually accessing a floating point register when we should be. We don't need to verify the structure size it's correct by construction. I wrote the original assert and know what it's for, verifying the index and not the structure. Again, it assumes F0 is the first FP reg which is arbitrary. On 2011-05-02 16:42:25, Gabe Black wrote: util/statetrace/arch/arm/tracechild.cc, line 129 http://reviews.m5sim.org/r/669/diff/1/?file=12215#file12215line129 Just because libc would use a macro doesn't mean we have to. You should replace this with a constant of the appropriate type. Ali Saidi wrote: I disagree... This will transition nicely as soon as libc gets it's act together. The fact that gcc uses macros is an unfortunate historical artifact, not a valid design decision. The transition won't be that nice either since it'll leave macro cruft in the source. We should either use their macro because we have to, or use our own thing that doesn't suck because it makes sense. Here we're combining the downsides of both of those approaches. On 2011-05-02 16:42:25, Gabe Black wrote: util/statetrace/arch/arm/tracechild.cc, line 151 http://reviews.m5sim.org/r/669/diff/1/?file=12215#file12215line151 Is the cast actually necessary here? I can believe it is to avoid a warning, but you could try leaving it out if you haven't already. Ali Saidi wrote: might as well be explicit It's not a huge thing, but if it's not required it's mostly visual noise. People will usually not care too much about exactly what type is being used, and if they do they can look at the function return type. - Gabe --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/669/#review1181 --- On 2011-05-02 15:41:28, Ali Saidi wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/669/ --- (Updated 2011-05-02 15:41:28) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- ARM: Add vfpv3 support to native trace. Diffs - src/arch/arm/nativetrace.hh 3f49ed206f46 src/arch/arm/nativetrace.cc 3f49ed206f46 util/statetrace/arch/arm/tracechild.hh 3f49ed206f46 util/statetrace/arch/arm/tracechild.cc 3f49ed206f46 Diff: http://reviews.m5sim.org/r/669/diff Testing --- Thanks, Ali ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: ARM: Add vfpv3 support to native trace.
On 2011-05-02 16:42:25, Gabe Black wrote: util/statetrace/arch/arm/tracechild.cc, line 79 http://reviews.m5sim.org/r/669/diff/1/?file=12215#file12215line79 I don't know how easy this would be to accommodate, but you're going to be sending a bunch of extra zeros for int regs that aren't 64 bits wide. Can you make it so you send full 64 bit values only when the source is actually 64 bits wide? Ali Saidi wrote: There is no reason to worry about sending 4 bytes down the wire. The speed issue isn't about sending 4 bytes, it's all about having to put a breakpoint after every instruction. Gabe Black wrote: The reason I implemented sending diffs of the state instead of sending the whole state is that what you send over the wire -does- matter, significantly. Breakpoints are slow too, but that doesn't mean everything else is irrelevant. I fully appreciate that it wouldn't be a good idea to send 1KB of data over the wire, but we're well past bike shedding arguing about 4 bytes. There is no reason to add complexity and control flow to try and avoid it. On 2011-05-02 16:42:25, Gabe Black wrote: util/statetrace/arch/arm/tracechild.cc, line 104 http://reviews.m5sim.org/r/669/diff/1/?file=12215#file12215line104 The idea is to verify that you're not falling off of uregs. Maybe you could do something more flexible like sizeof(myregs.uregs) / sizeof (myregs.uregs[0]). Ali Saidi wrote: uregs is never going to get smaller than it is now and I don't see a reason to come up with a crazy assert to try and prove that it isn't. Gabe Black wrote: uregs changing size is irrelevant. That formula will be exactly right all the time and doesn't depend on the coincidence that the CPSR is last. Only if you assume that the integer registers are sent first, which it seems like you're unwilling to make any assumptions about the code, so perhaps we should somehow verify that? On 2011-05-02 16:42:25, Gabe Black wrote: util/statetrace/arch/arm/tracechild.cc, line 111 http://reviews.m5sim.org/r/669/diff/1/?file=12215#file12215line111 The same comment applies as in getRegs, except that you have to deal with an offset. It would probably be a good idea to define something in the enum to mark the start of the FP regs. You can move the assert to after the if and subtract out the offset right before indexing fpregs. Ali Saidi wrote: There are clearly 32 float registers defined in the enum and in the struct. The assert just verifies that we're actually accessing a floating point register when we should be. We don't need to verify the structure size it's correct by construction. Gabe Black wrote: I wrote the original assert and know what it's for, verifying the index and not the structure. Again, it assumes F0 is the first FP reg which is arbitrary. No it's not! F0 Is the first floating point register. 0 is the first whole number so it's first. Would you rather some uglyiness of START_FP, F0 = START_FP? Why are we arguing about code that is correct by inspection, has been extensively tested, and works?! On 2011-05-02 16:42:25, Gabe Black wrote: util/statetrace/arch/arm/tracechild.cc, line 129 http://reviews.m5sim.org/r/669/diff/1/?file=12215#file12215line129 Just because libc would use a macro doesn't mean we have to. You should replace this with a constant of the appropriate type. Ali Saidi wrote: I disagree... This will transition nicely as soon as libc gets it's act together. Gabe Black wrote: The fact that gcc uses macros is an unfortunate historical artifact, not a valid design decision. The transition won't be that nice either since it'll leave macro cruft in the source. We should either use their macro because we have to, or use our own thing that doesn't suck because it makes sense. Here we're combining the downsides of both of those approaches. Sure, but if for some reason the value changes when the support actually makes it into libc it will be correct. - Ali --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/669/#review1181 --- On 2011-05-02 15:41:28, Ali Saidi wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/669/ --- (Updated 2011-05-02 15:41:28) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- ARM: Add vfpv3 support to native trace. Diffs - src/arch/arm/nativetrace.hh 3f49ed206f46 src/arch/arm/nativetrace.cc 3f49ed206f46 util/statetrace/arch/arm/tracechild.hh 3f49ed206f46 util/statetrace/arch/arm/tracechild.cc 3f49ed206f46
Re: [m5-dev] Review Request: SConstruct: automatically update .hg/hgrc with style hooks
On 2011-05-02 12:51:56, Nathan Binkert wrote: SConstruct, line 247 http://reviews.m5sim.org/r/668/diff/1/?file=12211#file12211line247 Should we prompt the user for permission? That way the user would at least know that something happened. Steve Reinhardt wrote: I doubt most people care. How many would really say no? Most people just want things to be automatic. If they really do care they can always edit it out later (the comments will let them know where the hooks came from). Nathan Binkert wrote: I agree that nobody would say no, but most people also would never notice that it had happened because the message would just fly by if there was no prompt. I guess that you're arguing that that would be a good thing :) Gabe Black wrote: Changing config files behind peoples back is a really bad idea in my opinion. I know I've stopped using entire distros (Suse) because they mucked with config files behind my back and perpetually broke my system. My configs are mine, and the minimal level of respect for that would be if we asked permission before we let ourselves in. I think leaving well enough alone and having the user go in and fix it themselves is actually the best approach. Nathan Binkert wrote: This isn't a config file in your homedir, it's the config file in the M5 repo and it will only be changed if you don't have it set up right, so it's not all that bad. I have the same leaning that you do, so I think I'd rather see a prompt for the user, but I don't feel that strongly about it. Yea, I would feel differently about ~/.hgrc, but this is just a local config file, and it's one that will need updating each time you clone a new repo. I don't mind making the warning a little more prominent, but since the status quo is that we won't even try to compile if the hooks aren't there, it seems overkill to me to make too big a deal of it... is someone going to be so offended about this that they're going to go try a different simulator? Plus if we could find a way to get these hooks to be part of the m5 repo so that they were automatically in place whenever you cloned, we would have done that, so I don't see how this is much different. - Steve --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/668/#review1173 --- On 2011-05-02 12:34:56, Steve Reinhardt wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/668/ --- (Updated 2011-05-02 12:34:56) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- SConstruct: automatically update .hg/hgrc with style hooks Seems easier than pestering people about it. Note also that path is now absolute, so you don't get errors when invoking hg from subdirectories. Diffs - SConstruct 66a3187a6714 Diff: http://reviews.m5sim.org/r/668/diff Testing --- Thanks, Steve ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
Re: [m5-dev] Review Request: ARM: Add vfpv3 support to native trace.
Quoting Ali Saidi sa...@umich.edu: On 2011-05-02 16:42:25, Gabe Black wrote: util/statetrace/arch/arm/tracechild.cc, line 79 http://reviews.m5sim.org/r/669/diff/1/?file=12215#file12215line79 I don't know how easy this would be to accommodate, but you're going to be sending a bunch of extra zeros for int regs that aren't 64 bits wide. Can you make it so you send full 64 bit values only when the source is actually 64 bits wide? Ali Saidi wrote: There is no reason to worry about sending 4 bytes down the wire. The speed issue isn't about sending 4 bytes, it's all about having to put a breakpoint after every instruction. Gabe Black wrote: The reason I implemented sending diffs of the state instead of sending the whole state is that what you send over the wire -does- matter, significantly. Breakpoints are slow too, but that doesn't mean everything else is irrelevant. I fully appreciate that it wouldn't be a good idea to send 1KB of data over the wire, but we're well past bike shedding arguing about 4 bytes. There is no reason to add complexity and control flow to try and avoid it. I don't think it's necessary, although I also don't think it would be that hard. Also, I don't like our tendency to characterize disagreement as bike shedding. If I don't agree, I don't agree. I'm not obligated to change my mind or just stand by while you do whatever you want. This is my code and by the convention we've stated on several occasions that means it's my decision. You're bike shedding by continuing to argue after I've made my decision. On 2011-05-02 16:42:25, Gabe Black wrote: util/statetrace/arch/arm/tracechild.cc, line 104 http://reviews.m5sim.org/r/669/diff/1/?file=12215#file12215line104 The idea is to verify that you're not falling off of uregs. Maybe you could do something more flexible like sizeof(myregs.uregs) / sizeof (myregs.uregs[0]). Ali Saidi wrote: uregs is never going to get smaller than it is now and I don't see a reason to come up with a crazy assert to try and prove that it isn't. Gabe Black wrote: uregs changing size is irrelevant. That formula will be exactly right all the time and doesn't depend on the coincidence that the CPSR is last. Only if you assume that the integer registers are sent first, which it seems like you're unwilling to make any assumptions about the code, so perhaps we should somehow verify that? Depending on the constants in an enum or the members of a data structure being in a particular order for all of time is dangerous and in this case pointless. I don't appreciate your sarcasm or you treating my attempts to protect the quality of my code like some sort of unreasonable hassle. Do you think I like taking my time to reverse engineer and review all this code somebody else wrote, and then having to fight over why it's wrong? It's an enormous waste of energy, and there are so many better uses for my time. And yet here we are. Again. On 2011-05-02 16:42:25, Gabe Black wrote: util/statetrace/arch/arm/tracechild.cc, line 111 http://reviews.m5sim.org/r/669/diff/1/?file=12215#file12215line111 The same comment applies as in getRegs, except that you have to deal with an offset. It would probably be a good idea to define something in the enum to mark the start of the FP regs. You can move the assert to after the if and subtract out the offset right before indexing fpregs. Ali Saidi wrote: There are clearly 32 float registers defined in the enum and in the struct. The assert just verifies that we're actually accessing a floating point register when we should be. We don't need to verify the structure size it's correct by construction. Gabe Black wrote: I wrote the original assert and know what it's for, verifying the index and not the structure. Again, it assumes F0 is the first FP reg which is arbitrary. No it's not! F0 Is the first floating point register. 0 is the first whole number so it's first. Would you rather some uglyiness of START_FP, F0 = START_FP? Why are we arguing about code that is correct by inspection, has been extensively tested, and works?! Yes, put that in my code. Why are we arguing about what you want to do to my code? How could anything be wrong with code that works right this moment? Why shouldn't I just let you do whatever you want because it's expedient? On 2011-05-02 16:42:25, Gabe Black wrote: util/statetrace/arch/arm/tracechild.cc, line 129 http://reviews.m5sim.org/r/669/diff/1/?file=12215#file12215line129 Just because libc would use a macro doesn't mean we have to. You should replace this with a constant of the appropriate type. Ali Saidi wrote: I disagree... This will transition nicely as soon as libc gets it's act together. Gabe Black wrote: The fact that gcc uses macros is an unfortunate historical artifact, not a valid design decision. The
Re: [m5-dev] Review Request: SConstruct: automatically update .hg/hgrc with style hooks
On 2011-05-02 12:51:56, Nathan Binkert wrote: SConstruct, line 247 http://reviews.m5sim.org/r/668/diff/1/?file=12211#file12211line247 Should we prompt the user for permission? That way the user would at least know that something happened. Steve Reinhardt wrote: I doubt most people care. How many would really say no? Most people just want things to be automatic. If they really do care they can always edit it out later (the comments will let them know where the hooks came from). Nathan Binkert wrote: I agree that nobody would say no, but most people also would never notice that it had happened because the message would just fly by if there was no prompt. I guess that you're arguing that that would be a good thing :) Gabe Black wrote: Changing config files behind peoples back is a really bad idea in my opinion. I know I've stopped using entire distros (Suse) because they mucked with config files behind my back and perpetually broke my system. My configs are mine, and the minimal level of respect for that would be if we asked permission before we let ourselves in. I think leaving well enough alone and having the user go in and fix it themselves is actually the best approach. Nathan Binkert wrote: This isn't a config file in your homedir, it's the config file in the M5 repo and it will only be changed if you don't have it set up right, so it's not all that bad. I have the same leaning that you do, so I think I'd rather see a prompt for the user, but I don't feel that strongly about it. Steve Reinhardt wrote: Yea, I would feel differently about ~/.hgrc, but this is just a local config file, and it's one that will need updating each time you clone a new repo. I don't mind making the warning a little more prominent, but since the status quo is that we won't even try to compile if the hooks aren't there, it seems overkill to me to make too big a deal of it... is someone going to be so offended about this that they're going to go try a different simulator? Plus if we could find a way to get these hooks to be part of the m5 repo so that they were automatically in place whenever you cloned, we would have done that, so I don't see how this is much different. I'm just afraid of this happening, causing some problem, somebody spending a long time and discovering (or not discovering) that some config file was changed without their knowledge and that's what was breaking everything. I can't imagine somebody not being a little miffed about that. But if you think it's really really safe and you've made it obvious what happened then I guess it could be ok. Maybe make it stop scons after that warning prints if those lines aren't there so it's harder to miss? Then they can just run it again and it'll go through, but they at least know what happened. And yes, I have heard of people that have tried to use M5, got frustrated and gone elsewhere. It is very possible to be too annoying or difficult and to drive away users. - Gabe --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/668/#review1173 --- On 2011-05-02 12:34:56, Steve Reinhardt wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/668/ --- (Updated 2011-05-02 12:34:56) Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert. Summary --- SConstruct: automatically update .hg/hgrc with style hooks Seems easier than pestering people about it. Note also that path is now absolute, so you don't get errors when invoking hg from subdirectories. Diffs - SConstruct 66a3187a6714 Diff: http://reviews.m5sim.org/r/668/diff Testing --- Thanks, Steve ___ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
