[gem5-dev] Re: review flurry

2020-04-28 Thread Gabe Black via gem5-dev
Yes, I also want to say thank you to everybody for getting through a lot of
reviews. We do still want to be careful since while our testing has come a
long way, we still have a better chance of catching bugs as they come in
than once they're floating around our big codebase somewhere. But still,
it's been very nice to get some many CLs taken care of!

Gabe

On Tue, Apr 28, 2020 at 9:05 AM Jason Lowe-Power via gem5-dev <
gem5-dev@gem5.org> wrote:

> Hey Gabe,
>
> While I agree that we need to have as many eyes on changesets as possible,
> not having enough reviews is not a new phenomenon. IIRC, this has happened
> with a number of your commits as well ;). It's just going to happen
> sometimes :).
>
> Actually, I want to thank everyone that's working so hard to get through
> our backlog of gerrit changes! This flurry of reviews and commits is super
> exciting!
>
> Remember, we'll have a two week time period for deep testing on the
> "staging" branch before we make the final release. The purpose of this time
> period is to catch all of the remaining bugs that we can.
>
> Cheers,
> Jason
>
> On Mon, Apr 27, 2020 at 12:10 PM Gabe Black via gem5-dev <
> gem5-dev@gem5.org>
> wrote:
>
> > Hey everybody, lets slow down here a little so that I have a chance to
> > actually see a review before the change gets checked in. I think we have
> at
> > least one bug checked in already. If a change hangs around for months
> > that's too long, but at least a day so everyone has a chance to see
> what's
> > going is a reasonable minimum.
> >
> > Gabe
> > ___
> > gem5-dev mailing list -- gem5-dev@gem5.org
> > To unsubscribe send an email to gem5-dev-le...@gem5.org
> > %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
> >
> ___
> gem5-dev mailing list -- gem5-dev@gem5.org
> To unsubscribe send an email to gem5-dev-le...@gem5.org
> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
>
___
gem5-dev mailing list -- gem5-dev@gem5.org
To unsubscribe send an email to gem5-dev-le...@gem5.org
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s


[gem5-dev] Change in gem5/gem5[develop]: arm: Fix some bugs in the aapcs64 implementation.

2020-04-28 Thread Gabe Black (Gerrit) via gem5-dev
Gabe Black has submitted this change. (  
https://gem5-review.googlesource.com/c/public/gem5/+/28267 )


Change subject: arm: Fix some bugs in the aapcs64 implementation.
..

arm: Fix some bugs in the aapcs64 implementation.

The templates which checked for short vectors, and our approximation of
HFA, HVA and HXA types were not correct. This change actually simplifies
them along with getting them to produce correct results. In the case of
HXA, there was a logic bug where an && was used where an || was
intended.

There may still be bugs in the actual collection of arguments and
setting of return values since those aspects are harder to test.

Change-Id: Ice3177205a98c678ecb43ba600813b3909c44e6b
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/28267
Reviewed-by: Nikos Nikoleris 
Maintainer: Giacomo Travaglini 
Tested-by: kokoro 
---
M src/arch/arm/aapcs64.hh
1 file changed, 16 insertions(+), 30 deletions(-)

Approvals:
  Nikos Nikoleris: Looks good to me, approved
  Giacomo Travaglini: Looks good to me, approved
  kokoro: Regressions pass



diff --git a/src/arch/arm/aapcs64.hh b/src/arch/arm/aapcs64.hh
index 203846d..30597f5 100644
--- a/src/arch/arm/aapcs64.hh
+++ b/src/arch/arm/aapcs64.hh
@@ -70,24 +70,17 @@

 // A short vector is a machine type that is composed of repeated instances  
of
 // one fundamental integral or floating- point type. It may be 8 or 16  
bytes

-// in total size. We represent it here as an opaque blob of data with an
-// appropriate alignment requirement.
+// in total size.

-template 
-using Aapcs64ShortVectorCandidate =
-alignas(sizeof(T) * count) uint8_t [sizeof(T) * count];
-
-template 
-using Aapcs64ShortVector = Aapcs64ShortVectorCandidate::value || std::is_floating_point::value) &&
-(sizeof(T) * count == 8 || sizeof(T) * count == 16)>::type>;
-
-template 
+template 
 struct IsAapcs64ShortVector : public std::false_type {};

 template 
-struct IsAapcs64ShortVector> : public  
std::true_type

+struct IsAapcs64ShortVector::value || std::is_floating_point::value) &&
+(sizeof(E) * N == 8 || sizeof(E) * N == 16)>::type> :
+public std::true_type
 {};

 /*
@@ -114,38 +107,31 @@
 // we can't actually detect that or manipulate that with templates.  
Instead,

 // we approximate that by detecting only arrays with that property.

-template 
-using Aapcs64HomogeneousAggregate = T[count];
-
 // An Homogeneous Floating-Point Aggregate (HFA) is an Homogeneous  
Aggregate
 // with a Fundemental Data Type that is a Floating-Point type and at most  
four

 // uniquely addressable members.

-template 
-using Aapcs64Hfa = Aapcs64HomogeneousAggregate::value &&
-  count <= 4>::type>;
-
 template 
 struct IsAapcs64Hfa : public std::false_type {};

 template 
-struct IsAapcs64Hfa> : public std::true_type {};
+struct IsAapcs64Hfa::value &&
+N <= 4>::type> : public std::true_type
+{};

 // An Homogeneous Short-Vector Aggregate (HVA) is an Homogeneous Aggregate  
with

 // a Fundamental Data Type that is a Short-Vector type and at most four
 // uniquely addressable members.

-template 
-using Aapcs64Hva = Aapcs64HomogeneousAggregate::value &&
-  count <= 4>::type>;
-
 template 
 struct IsAapcs64Hva : public std::false_type {};

 template 
-struct IsAapcs64Hva> : public std::true_type {};
+struct IsAapcs64Hva::value &&
+N <= 4>::type> : public std::true_type
+{};

 // A shorthand to test if a type is an HVA or an HFA.
 template 
@@ -153,7 +139,7 @@

 template 
 struct IsAapcs64Hxa::value && IsAapcs64Hva::value>::type> :
+IsAapcs64Hfa::value || IsAapcs64Hva::value>::type> :
 public std::true_type
 {};


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


Gerrit-Project: public/gem5
Gerrit-Branch: develop
Gerrit-Change-Id: Ice3177205a98c678ecb43ba600813b3909c44e6b
Gerrit-Change-Number: 28267
Gerrit-PatchSet: 2
Gerrit-Owner: Gabe Black 
Gerrit-Reviewer: Bobby R. Bruce 
Gerrit-Reviewer: Gabe Black 
Gerrit-Reviewer: Giacomo Travaglini 
Gerrit-Reviewer: Jason Lowe-Power 
Gerrit-Reviewer: Nikos Nikoleris 
Gerrit-Reviewer: Nikos Nikoleris 
Gerrit-Reviewer: kokoro 
Gerrit-MessageType: merged
___
gem5-dev mailing list -- gem5-dev@gem5.org
To unsubscribe send an email to gem5-dev-le...@gem5.org
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s


[gem5-dev] Re: review flurry

2020-04-28 Thread Jason Lowe-Power via gem5-dev
Hey Gabe,

While I agree that we need to have as many eyes on changesets as possible,
not having enough reviews is not a new phenomenon. IIRC, this has happened
with a number of your commits as well ;). It's just going to happen
sometimes :).

Actually, I want to thank everyone that's working so hard to get through
our backlog of gerrit changes! This flurry of reviews and commits is super
exciting!

Remember, we'll have a two week time period for deep testing on the
"staging" branch before we make the final release. The purpose of this time
period is to catch all of the remaining bugs that we can.

Cheers,
Jason

On Mon, Apr 27, 2020 at 12:10 PM Gabe Black via gem5-dev 
wrote:

> Hey everybody, lets slow down here a little so that I have a chance to
> actually see a review before the change gets checked in. I think we have at
> least one bug checked in already. If a change hangs around for months
> that's too long, but at least a day so everyone has a chance to see what's
> going is a reasonable minimum.
>
> Gabe
> ___
> gem5-dev mailing list -- gem5-dev@gem5.org
> To unsubscribe send an email to gem5-dev-le...@gem5.org
> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
>
___
gem5-dev mailing list -- gem5-dev@gem5.org
To unsubscribe send an email to gem5-dev-le...@gem5.org
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s


[gem5-dev] Change in gem5/gem5[develop]: configs: Add missing requestToMemory MessageBuffers

2020-04-28 Thread Matthew Poremba (Gerrit) via gem5-dev
Matthew Poremba has submitted this change. (  
https://gem5-review.googlesource.com/c/public/gem5/+/28187 )


Change subject: configs: Add missing requestToMemory MessageBuffers
..

configs: Add missing requestToMemory MessageBuffers

In commit 53b6e21 two protocol config files were missed when the new
requestToMemory MessageBuffers were added. This fixes the issue such
that all Ruby protocols are working again.

Change-Id: Iaa04c792eaf6d659ba13c19f003e7e31b71ffdb4
JIRA: https://gem5.atlassian.net/browse/GEM5-468
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/28187
Reviewed-by: Bobby R. Bruce 
Reviewed-by: Jason Lowe-Power 
Reviewed-by: Ciro Santilli 
Maintainer: Jason Lowe-Power 
Tested-by: kokoro 
---
M configs/ruby/MESI_Three_Level.py
M configs/ruby/MOESI_AMD_Base.py
2 files changed, 3 insertions(+), 0 deletions(-)

Approvals:
  Jason Lowe-Power: Looks good to me, approved; Looks good to me, approved
  Ciro Santilli: Looks good to me, but someone else must approve
  Bobby R. Bruce: Looks good to me, approved
  kokoro: Regressions pass



diff --git a/configs/ruby/MESI_Three_Level.py  
b/configs/ruby/MESI_Three_Level.py

index c05dca3..fdebea4 100644
--- a/configs/ruby/MESI_Three_Level.py
+++ b/configs/ruby/MESI_Three_Level.py
@@ -229,6 +229,7 @@
 dir_cntrl.responseToDir.slave = ruby_system.network.master
 dir_cntrl.responseFromDir = MessageBuffer()
 dir_cntrl.responseFromDir.master = ruby_system.network.slave
+dir_cntrl.requestToMemory = MessageBuffer()
 dir_cntrl.responseFromMemory = MessageBuffer()

 for i, dma_port in enumerate(dma_ports):
diff --git a/configs/ruby/MOESI_AMD_Base.py b/configs/ruby/MOESI_AMD_Base.py
index 5b2d912..aa9dd50 100644
--- a/configs/ruby/MOESI_AMD_Base.py
+++ b/configs/ruby/MOESI_AMD_Base.py
@@ -277,6 +277,8 @@

 dir_cntrl.triggerQueue = MessageBuffer(ordered = True)
 dir_cntrl.L3triggerQueue = MessageBuffer(ordered = True)
+
+dir_cntrl.requestToMemory = MessageBuffer()
 dir_cntrl.responseFromMemory = MessageBuffer()

 exec("system.dir_cntrl%d = dir_cntrl" % i)

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


Gerrit-Project: public/gem5
Gerrit-Branch: develop
Gerrit-Change-Id: Iaa04c792eaf6d659ba13c19f003e7e31b71ffdb4
Gerrit-Change-Number: 28187
Gerrit-PatchSet: 2
Gerrit-Owner: Matthew Poremba 
Gerrit-Reviewer: Bobby R. Bruce 
Gerrit-Reviewer: Ciro Santilli 
Gerrit-Reviewer: Jason Lowe-Power 
Gerrit-Reviewer: Matthew Poremba 
Gerrit-Reviewer: kokoro 
Gerrit-MessageType: merged
___
gem5-dev mailing list -- gem5-dev@gem5.org
To unsubscribe send an email to gem5-dev-le...@gem5.org
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s


[gem5-dev] Change in gem5/gem5[develop]: arch-arm: Fix clasta/b and lasta/b simd instructions

2020-04-28 Thread Jordi Vaquero (Gerrit) via gem5-dev
Jordi Vaquero has submitted this change. (  
https://gem5-review.googlesource.com/c/public/gem5/+/28247 )


Change subject: arch-arm: Fix clasta/b and lasta/b simd instructions
..

arch-arm: Fix clasta/b and lasta/b simd instructions

The simd version of this instructions required zeroing the result
vector except for the first element, that contains the result.

Change-Id: I231ad3c44d89f34acae26d299ab676e2ed09acdc
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/28247
Reviewed-by: Giacomo Travaglini 
Maintainer: Giacomo Travaglini 
Tested-by: kokoro 
---
M src/arch/arm/isa/insts/sve.isa
1 file changed, 10 insertions(+), 1 deletion(-)

Approvals:
  Giacomo Travaglini: Looks good to me, approved; Looks good to me, approved
  kokoro: Regressions pass



diff --git a/src/arch/arm/isa/insts/sve.isa b/src/arch/arm/isa/insts/sve.isa
index 06ff728..aa4f194 100644
--- a/src/arch/arm/isa/insts/sve.isa
+++ b/src/arch/arm/isa/insts/sve.isa
@@ -2432,7 +2432,16 @@
 elif destType == DstRegType.SimdFpScalar:
 code += ''' else {
 AA64FpDest_x[0] = AA64FpDestMerge_x[0];
-}'''
+}
+'''
+if destType == DstRegType.SimdFpScalar:
+# This section will extend zeros to the simdFP scalar
+# intructions for lasta/b and Clasta/b
+code += '''
+for (int i = 1; i < eCount; ++i) {
+AA64FpDest_x[i] = (Element)0x0;
+}
+'''
 iop = InstObjParams(name, 'Sve' + Name, 'SveSelectOp',
 {'code': code, 'op_class': opClass,
  'isCond': 'true' if isCond else 'false',

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


Gerrit-Project: public/gem5
Gerrit-Branch: develop
Gerrit-Change-Id: I231ad3c44d89f34acae26d299ab676e2ed09acdc
Gerrit-Change-Number: 28247
Gerrit-PatchSet: 2
Gerrit-Owner: Jordi Vaquero 
Gerrit-Reviewer: Ciro Santilli 
Gerrit-Reviewer: Giacomo Travaglini 
Gerrit-Reviewer: Jordi Vaquero 
Gerrit-Reviewer: kokoro 
Gerrit-MessageType: merged
___
gem5-dev mailing list -- gem5-dev@gem5.org
To unsubscribe send an email to gem5-dev-le...@gem5.org
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s


[gem5-dev] Change in gem5/gem5[develop]: sim, arch-arm: Restore capability of running without a kernel

2020-04-28 Thread Giacomo Travaglini (Gerrit) via gem5-dev
Giacomo Travaglini has submitted this change. (  
https://gem5-review.googlesource.com/c/public/gem5/+/28147 )


Change subject: sim, arch-arm: Restore capability of running without a  
kernel

..

sim, arch-arm: Restore capability of running without a kernel

The following patch:

https://gem5-review.googlesource.com/c/public/gem5/+/24283

Removed the capability of starting a gem5 simulation without
a kernel object. This patch is restoring it

Change-Id: I6d751bac386cbb250b3593bb12463140dc964ab3
Signed-off-by: Giacomo Travaglini 
Reviewed-by: Nikos Nikoleris 
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/28147
Reviewed-by: Gabe Black 
Tested-by: kokoro 
---
M src/arch/arm/fs_workload.cc
M src/sim/kernel_workload.cc
2 files changed, 35 insertions(+), 26 deletions(-)

Approvals:
  Gabe Black: Looks good to me, approved
  Giacomo Travaglini: Looks good to me, approved
  kokoro: Regressions pass



diff --git a/src/arch/arm/fs_workload.cc b/src/arch/arm/fs_workload.cc
index 550d8de..09c7bb2 100644
--- a/src/arch/arm/fs_workload.cc
+++ b/src/arch/arm/fs_workload.cc
@@ -69,9 +69,13 @@
 }
 }

-FsWorkload::FsWorkload(Params *p) : KernelWorkload(*p),
-kernelEntry((kernelObj->entryPoint() & loadAddrMask()) +  
loadAddrOffset())

+FsWorkload::FsWorkload(Params *p) : KernelWorkload(*p)
 {
+if (kernelObj) {
+kernelEntry = (kernelObj->entryPoint() & loadAddrMask()) +
+loadAddrOffset();
+}
+
 bootLoaders.reserve(p->boot_loader.size());
 for (const auto  : p->boot_loader) {
 std::unique_ptr bl_obj;
diff --git a/src/sim/kernel_workload.cc b/src/sim/kernel_workload.cc
index f107d69..415ff96 100644
--- a/src/sim/kernel_workload.cc
+++ b/src/sim/kernel_workload.cc
@@ -38,39 +38,44 @@
 if (!Loader::debugSymbolTable)
 Loader::debugSymbolTable = new Loader::SymbolTable;

-kernelObj = Loader::createObjectFile(params().object_file);
-inform("kernel located at: %s", params().object_file);
+if (params().object_file == "") {
+inform("No kernel set for full system simulation. "
+   "Assuming you know what you're doing.");
+} else {
+kernelObj = Loader::createObjectFile(params().object_file);
+inform("kernel located at: %s", params().object_file);

-fatal_if(!kernelObj,
-"Could not load kernel file %s", params().object_file);
+fatal_if(!kernelObj,
+"Could not load kernel file %s", params().object_file);

-image = kernelObj->buildImage();
+image = kernelObj->buildImage();

-_start = image.minAddr();
-_end = image.maxAddr();
+_start = image.minAddr();
+_end = image.maxAddr();

-// If load_addr_mask is set to 0x0, then calculate the smallest mask to
-// cover all kernel addresses so gem5 can relocate the kernel to a new
-// offset.
-if (_loadAddrMask == 0)
-_loadAddrMask = mask(findMsbSet(_end - _start) + 1);
+// If load_addr_mask is set to 0x0, then calculate the smallest  
mask to
+// cover all kernel addresses so gem5 can relocate the kernel to a  
new

+// offset.
+if (_loadAddrMask == 0)
+_loadAddrMask = mask(findMsbSet(_end - _start) + 1);

-image.move([this](Addr a) {
-return (a & _loadAddrMask) + _loadAddrOffset;
-});
+image.move([this](Addr a) {
+return (a & _loadAddrMask) + _loadAddrOffset;
+});

-// load symbols
-fatal_if(!kernelObj->loadGlobalSymbols(kernelSymtab),
-"Could not load kernel symbols.");
+// load symbols
+fatal_if(!kernelObj->loadGlobalSymbols(kernelSymtab),
+"Could not load kernel symbols.");

-fatal_if(!kernelObj->loadLocalSymbols(kernelSymtab),
-"Could not load kernel local symbols.");
+fatal_if(!kernelObj->loadLocalSymbols(kernelSymtab),
+"Could not load kernel local symbols.");

-fatal_if(!kernelObj->loadGlobalSymbols(Loader::debugSymbolTable),
-"Could not load kernel symbols.");
+fatal_if(!kernelObj->loadGlobalSymbols(Loader::debugSymbolTable),
+"Could not load kernel symbols.");

-fatal_if(!kernelObj->loadLocalSymbols(Loader::debugSymbolTable),
-"Could not load kernel local symbols.");
+fatal_if(!kernelObj->loadLocalSymbols(Loader::debugSymbolTable),
+"Could not load kernel local symbols.");
+}

 // Loading only needs to happen once and after memory system is
 // connected so it will happen in initState()

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


Gerrit-Project: public/gem5
Gerrit-Branch: develop
Gerrit-Change-Id: I6d751bac386cbb250b3593bb12463140dc964ab3
Gerrit-Change-Number: 28147
Gerrit-PatchSet: 2

[gem5-dev] Change in gem5/gem5[develop]: configs: Do not require args.kernel to be set in baremetal.py

2020-04-28 Thread Giacomo Travaglini (Gerrit) via gem5-dev
Giacomo Travaglini has submitted this change. (  
https://gem5-review.googlesource.com/c/public/gem5/+/28148 )


Change subject: configs: Do not require args.kernel to be set in  
baremetal.py

..

configs: Do not require args.kernel to be set in baremetal.py

This is allowing to us run baremetal.py with the --dtb-gen option
without needing to specify a --kernel argument

Change-Id: I98f1bc865d2f4e2230b1a85453efe83d95ec8a55
Signed-off-by: Giacomo Travaglini 
Reviewed-by: Nikos Nikoleris 
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/28148
Reviewed-by: Jason Lowe-Power 
Maintainer: Jason Lowe-Power 
Tested-by: kokoro 
---
M configs/example/arm/baremetal.py
1 file changed, 4 insertions(+), 2 deletions(-)

Approvals:
  Jason Lowe-Power: Looks good to me, approved; Looks good to me, approved
  kokoro: Regressions pass



diff --git a/configs/example/arm/baremetal.py  
b/configs/example/arm/baremetal.py

index 24f40ec..412625d 100644
--- a/configs/example/arm/baremetal.py
+++ b/configs/example/arm/baremetal.py
@@ -89,6 +89,8 @@
 print("Error: Bootscript %s does not exist" % args.readfile)
 sys.exit(1)

+object_file = args.kernel if args.kernel else ""
+
 cpu_class = cpu_types[args.cpu][0]
 mem_mode = cpu_class.memory_mode()
 # Only simulate caches when using a timing CPU (e.g., the HPI model)
@@ -111,7 +113,7 @@
 stdout=args.semi_stdout,
 stderr=args.semi_stderr,
 files_root_dir=args.semi_path,
-cmd_line = " ".join([ args.kernel ] + args.args)
+cmd_line = " ".join([ object_file ] + args.args)
 )

 # Add the PCI devices we need for this system. The base system
@@ -162,7 +164,7 @@

 workload_class = workloads.workload_list.get(args.workload)
 system.workload = workload_class(
-args.kernel, system)
+object_file, system)

 return system


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


Gerrit-Project: public/gem5
Gerrit-Branch: develop
Gerrit-Change-Id: I98f1bc865d2f4e2230b1a85453efe83d95ec8a55
Gerrit-Change-Number: 28148
Gerrit-PatchSet: 2
Gerrit-Owner: Giacomo Travaglini 
Gerrit-Reviewer: Giacomo Travaglini 
Gerrit-Reviewer: Jason Lowe-Power 
Gerrit-Reviewer: Nikos Nikoleris 
Gerrit-Reviewer: kokoro 
Gerrit-MessageType: merged
___
gem5-dev mailing list -- gem5-dev@gem5.org
To unsubscribe send an email to gem5-dev-le...@gem5.org
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s