[gem5-dev] Change in gem5/gem5[develop]: cpu: Remove "lane" accessors from the ExecContext classes.

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


Change subject: cpu: Remove "lane" accessors from the ExecContext classes.
..

cpu: Remove "lane" accessors from the ExecContext classes.

These are not used by instructions. If something other than instructions
needs that style of access, it would use the ThreadContext, not the
ExecContext.

Change-Id: Ic74dcfd34f8bb0786bd2688b44d0d90714503637
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/41897
Reviewed-by: Gabe Black 
Maintainer: Gabe Black 
Tested-by: kokoro 
---
M src/cpu/checker/cpu.hh
M src/cpu/exec_context.hh
M src/cpu/minor/exec_context.hh
M src/cpu/o3/dyn_inst.hh
M src/cpu/simple/exec_context.hh
5 files changed, 0 insertions(+), 316 deletions(-)

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



diff --git a/src/cpu/checker/cpu.hh b/src/cpu/checker/cpu.hh
index 42a38fc..0900125 100644
--- a/src/cpu/checker/cpu.hh
+++ b/src/cpu/checker/cpu.hh
@@ -218,79 +218,6 @@
 return thread->getWritableVecReg(reg);
 }

-/** Vector Register Lane Interfaces. */
-/** @{ */
-/** Reads source vector 8bit operand. */
-virtual ConstVecLane8
-readVec8BitLaneOperand(const StaticInst *si, int idx) const override
-{
-const RegId& reg = si->destRegIdx(idx);
-assert(reg.isVecReg());
-return thread->readVec8BitLaneReg(reg);
-}
-
-/** Reads source vector 16bit operand. */
-virtual ConstVecLane16
-readVec16BitLaneOperand(const StaticInst *si, int idx) const override
-{
-const RegId& reg = si->destRegIdx(idx);
-assert(reg.isVecReg());
-return thread->readVec16BitLaneReg(reg);
-}
-
-/** Reads source vector 32bit operand. */
-virtual ConstVecLane32
-readVec32BitLaneOperand(const StaticInst *si, int idx) const override
-{
-const RegId& reg = si->destRegIdx(idx);
-assert(reg.isVecReg());
-return thread->readVec32BitLaneReg(reg);
-}
-
-/** Reads source vector 64bit operand. */
-virtual ConstVecLane64
-readVec64BitLaneOperand(const StaticInst *si, int idx) const override
-{
-const RegId& reg = si->destRegIdx(idx);
-assert(reg.isVecReg());
-return thread->readVec64BitLaneReg(reg);
-}
-
-/** Write a lane of the destination vector operand. */
-template 
-void
-setVecLaneOperandT(const StaticInst *si, int idx, const LD& val)
-{
-const RegId& reg = si->destRegIdx(idx);
-assert(reg.isVecReg());
-return thread->setVecLane(reg, val);
-}
-virtual void
-setVecLaneOperand(const StaticInst *si, int idx,
-const LaneData& val) override
-{
-setVecLaneOperandT(si, idx, val);
-}
-virtual void
-setVecLaneOperand(const StaticInst *si, int idx,
-const LaneData& val) override
-{
-setVecLaneOperandT(si, idx, val);
-}
-virtual void
-setVecLaneOperand(const StaticInst *si, int idx,
-const LaneData& val) override
-{
-setVecLaneOperandT(si, idx, val);
-}
-virtual void
-setVecLaneOperand(const StaticInst *si, int idx,
-const LaneData& val) override
-{
-setVecLaneOperandT(si, idx, val);
-}
-/** @} */
-
 TheISA::VecElem
 readVecElemOperand(const StaticInst *si, int idx) const override
 {
diff --git a/src/cpu/exec_context.hh b/src/cpu/exec_context.hh
index 42dafbc..3c40f31 100644
--- a/src/cpu/exec_context.hh
+++ b/src/cpu/exec_context.hh
@@ -117,36 +117,6 @@
 const TheISA::VecRegContainer& val) = 0;
 /** @} */

-/** Vector Register Lane Interfaces. */
-/** @{ */
-/** Reads source vector 8bit operand. */
-virtual ConstVecLane8 readVec8BitLaneOperand(
-const StaticInst *si, int idx) const = 0;
-
-/** Reads source vector 16bit operand. */
-virtual ConstVecLane16 readVec16BitLaneOperand(
-const StaticInst *si, int idx) const = 0;
-
-/** Reads source vector 32bit operand. */
-virtual ConstVecLane32 readVec32BitLaneOperand(
-const StaticInst *si, int idx) const = 0;
-
-/** Reads source vector 64bit operand. */
-virtual ConstVecLane64 readVec64BitLaneOperand(
-const StaticInst *si, int idx) const = 0;
-
-/** Write a lane of the destination vector operand. */
-/** @{ */
-virtual void setVecLaneOperand(const StaticInst *si, int idx,
-const LaneData& val) = 0;
-virtual void setVecLaneOperand(const StaticInst *si, int idx,
-const LaneData& val) = 0;
-virtual void setVecLaneOperand(const StaticInst *si, int idx,
-const LaneData& val) = 0;
-virtual void setVecLaneOperand(const StaticInst *si, int idx,
-const LaneData& val) = 0;
-/** @} */
-
 /**

[gem5-dev] Change in gem5/gem5[release-staging-v21-0]: gpu-compute: Fix accidental execution when stopped at barrier

2021-03-04 Thread Matt Sinclair (Gerrit) via gem5-dev 
Matt Sinclair has submitted this change. (  
https://gem5-review.googlesource.com/c/public/gem5/+/41573 )


Change subject: gpu-compute: Fix accidental execution when stopped at  
barrier

..

gpu-compute: Fix accidental execution when stopped at barrier

Due the compute unit pipeline being executed in reverse order, there
exists a scenario where a compute unit will execute an extra
instruction when it's supposed to be stopped at a barrier. It occurs
as follows:

* The ScheduleStage sets a barrier instruction ready to execute.

* The ScoreboardCheckStage adds another instruction to the readyList.
This is where the barrier is checked, but because the barrier isn't
executing yet, the instruction can be passed along to ScheduleStage

* The barrier executes, and stalls

* The ScheduleStage sees that there's a new instruction and schedules
it to be executed.

* Only now will the ScoreboardCheckStage realize a barrier is active
and stall accordingly

* The subsequent instruction executes

This patch sets the wavefront status to be S_BARRIER in ScheduleStage
instead of in the barrier instruction execution in order to have
ScoreboardCheckStage realize that we're going to execute a barrier,
preventing it from marking another instruciton as ready.

Change-Id: Ib683e2c68f361d7ee60a3beaf53b4b6c888c9f8d
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/41573
Reviewed-by: Matt Sinclair 
Reviewed-by: Alexandru Duțu 
Maintainer: Matt Sinclair 
Tested-by: kokoro 
---
M src/arch/gcn3/insts/instructions.cc
M src/gpu-compute/schedule_stage.cc
2 files changed, 3 insertions(+), 2 deletions(-)

Approvals:
  Alexandru Duțu: Looks good to me, approved
  Matt Sinclair: Looks good to me, but someone else must approve; Looks  
good to me, approved

  kokoro: Regressions pass



diff --git a/src/arch/gcn3/insts/instructions.cc  
b/src/arch/gcn3/insts/instructions.cc

index 29de1a8..bde87ef 100644
--- a/src/arch/gcn3/insts/instructions.cc
+++ b/src/arch/gcn3/insts/instructions.cc
@@ -4114,8 +4114,6 @@

 if (wf->hasBarrier()) {
 int bar_id = wf->barrierId();
-assert(wf->getStatus() != Wavefront::S_BARRIER);
-wf->setStatus(Wavefront::S_BARRIER);
 cu->incNumAtBarrier(bar_id);
 DPRINTF(GPUSync, "CU[%d] WF[%d][%d] Wave[%d] - Stalling at "
 "barrier Id%d. %d waves now at barrier, %d waves "
diff --git a/src/gpu-compute/schedule_stage.cc  
b/src/gpu-compute/schedule_stage.cc

index 8a2ea18..ace6d0c 100644
--- a/src/gpu-compute/schedule_stage.cc
+++ b/src/gpu-compute/schedule_stage.cc
@@ -314,6 +314,9 @@
 computeUnit.insertInPipeMap(wf);
 wavesInSch.emplace(wf->wfDynId);
 schList.at(exeType).push_back(std::make_pair(gpu_dyn_inst,  
RFBUSY));

+if (wf->isOldestInstBarrier() && wf->hasBarrier()) {
+wf->setStatus(Wavefront::S_BARRIER);
+}
 if (wf->isOldestInstWaitcnt()) {
 wf->setStatus(Wavefront::S_WAITCNT);
 }

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


Gerrit-Project: public/gem5
Gerrit-Branch: release-staging-v21-0
Gerrit-Change-Id: Ib683e2c68f361d7ee60a3beaf53b4b6c888c9f8d
Gerrit-Change-Number: 41573
Gerrit-PatchSet: 4
Gerrit-Owner: Kyle Roarty 
Gerrit-Reviewer: Alexandru Duțu 
Gerrit-Reviewer: Matt Sinclair 
Gerrit-Reviewer: Matthew Poremba 
Gerrit-Reviewer: kokoro 
Gerrit-CC: Bobby R. Bruce 
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: RFC: run python Black on gem5 python code

2021-03-04 Thread Giacomo Travaglini via gem5-dev 


> -Original Message-
> From: Jason Lowe-Power 
> Sent: 04 March 2021 15:27
> To: gem5 Developer List 
> Cc: Gabe Black ; Giacomo Travaglini
> 
> Subject: Re: [gem5-dev] Re: RFC: run python Black on gem5 python code
>
> Hi all,
>
>
> Some responses inline below. The other thing I'll mention is that I believe we
> can integrate it in a way where it's completely invisible to most developers.
> You can edit a file with whatever style you want, then after you run black it 
> will
> be in the PEP8 style. From my experience, the only things that need to be
> manually changed are quoted strings that are more than 80 characters.
>

I would be very careful about hiding this to the developers at the point of 
automatically changing their code
without having them noticing it.

I understand you want developers to commit without the hassle of explicitly 
dealing with style errors.
On the other hand though we are already doing this for C++ so I don't see a 
reason why we shouldn't do the same
for the python world.

Other options could be to run the linter as a pre-push hook, or to run the 
linter as part of our presubmit Jenkins script.

> On Thu, Mar 4, 2021 at 6:49 AM Giacomo Travaglini via gem5-dev  d...@gem5.org  > wrote:
>
>
> Hi Jason,
>
> This is great news. When you say it is not configurable, do you mean it
> is not possible to suppress warning/errors or do you
> mean it is not possible to come up with custom rules?
>
>
>
> There is a way to suppress changes by modifying the file with a comment `#
> fmt: off`. You can also exclude files with a dotfile (like .gitignore).
>
>
>
> As I presume it's the first case, I am wondering if a different linter
> might provide such capabilities:
>
> https://books.agiliq.com/projects/essential-python-
> tools/en/latest/linters.html
>
> Is there a reason to prefer black over other linters?
>
>
>
> It's owned by the python software foundation, which makes it seem "official".
> Though the real reason is that it's the first one I looked at and the only 
> one I
> investigated.

I actually thought pycodestyle was the "official" linter (I am not sure if the 
"official" word makes completely sense though)

I was asking this because for example in pycodestyle (and prob other linters as 
well) there are error codes 
https://pycodestyle.pycqa.org/en/latest/intro.html#error-codes you could use 
include or exclude lists of errors (to ignore specific types of warnings).

Kind regards

Giacomo

>
>
>
> Kind regards
>
> Giacomo
>
> > -Original Message-
> > From: Gabe Black via gem5-dev   >
> > Sent: 04 March 2021 03:59
> > To: gem5 Developer List mailto:gem5-
> d...@gem5.org> >
> > Cc: Gabe Black   >
> > Subject: [gem5-dev] Re: RFC: run python Black on gem5 python code
> >
> > I'm a little worried about the no exceptions part of that, since we
> might have
> > some weird restrictions that we have to do weird things to work
> around, but I
> > can't really think of an example of that off hand. I'd want to look at it
> to see
> > how much wiggle room there is in the style, since I think ironclad
> rules which
> > make no accommodation for occasional common sense are maybe
> more
> > trouble than they're worth. I'm not opposed to having at least some
> stated
> > standard for python though, and the "official" one seems like a
> pretty
> > reasonable choice. I guess it's fine with me, up until it causes me
> some sort of
> > problem :-)
>
>
>
> I actually think it's a feature not a bug. With an "uncompromising Python code
> formatter" there can be no arguments in code review about the style.
>
>
> >
> > Maybe the right thing to do would be to give it a shot but not make
> it
> > compulsory until we have a feeling for how much trouble it is.
> >
> >
> > Gabe
> >
> > On Wed, Mar 3, 2021 at 11:24 AM Bobby Bruce via gem5-dev  > d...@gem5.org    d...@gem5.org  > > wrote:
> >
> >
> > Sounds like a good idea to me.
> > ---
> >
> > Dr. Bobby R. Bruce
> > Room 2235,
> > Kemper Hall, UC Davis
> > Davis,
> > CA, 95616
> >
> >
> > web: https://www.bobbybruce.net
> >
> >
> >
> > On Wed, Mar 3, 2021 at 10:11 AM Daniel Carvalho via gem5-dev
> > mailto:gem5-dev@gem5.org>
>  > > wrote:
> >
> >
> > +1
> >
> >
> > Em quarta-feira, 3 de março de 2021 14:35:57 BRT, Jason
> > Lowe-Power via gem5-dev mailto:gem5-
> d...@gem5.org>  
> > d...@gem5.org  > > escreveu:
> >
> >
> > Hi all,
> >
> > Right now, we don't have an official style guide for python.
> > Our style guide
> >
> (http://www.gem5.org/documentation/general_docs/development/coding_st
> > yle/) is very C++ focused.
> >
> > I would like to propose adopting a relatively strict PEP 8 style
> > guide: https://www.python.org/dev/peps/pep-0008. This is the
> "official" style
> > guide for python (as

[gem5-dev] Re: vector register indexing modes and renaming?

2021-03-04 Thread Giacomo Travaglini via gem5-dev 


> -Original Message-
> From: Gabe Black 
> Sent: 04 March 2021 04:04
> To: Giacomo Travaglini 
> Cc: gem5 Developer List 
> Subject: Re: [gem5-dev] vector register indexing modes and renaming?
>
>
>
> On Mon, Mar 1, 2021 at 6:48 AM Giacomo Travaglini
> mailto:giacomo.travagl...@arm.com> >
> wrote:
>
>
>
> > -Original Message-
> > From: Gabe Black   >
> > Sent: 27 February 2021 05:47
> > To: Giacomo Travaglini   >
> > Cc: gem5 Developer List mailto:gem5-
> d...@gem5.org> >
> > Subject: Re: [gem5-dev] vector register indexing modes and
> renaming?
> >
> > Another question/clarification:
> >
> > Does any data actually get shared between the two rename modes? I
> think you
> > said there is not, but now I can't find that.
>
> Data *do* get shared, even if in gem5 we have separate physical
> registers.
> In fact, when rename mode changes [1], (meta)data is copied from one
> register file to the other.
> For example, say we have an AArch64 kernel running at EL1 and my
> AArch32 (basically armv7) floating point application running at EL0.
>
> My application will be using vector elements; however, every time
> there is an exception to AArch64, cpu will switch
> Rename mode and data will be copied / mapping will be adjusted. Any
> FP & SIMD operation at this point will use vector registers.
> When the kernel finishes its stuff, and goes back to AArch32, vector
> elements will be repopulated.
>
>
>
>
> Ok, I thought that was what you said, and I couldn't think of another reason 
> to
> go through all the trouble of copying things around.
>
>
>
>
> > Would it work just as well to have
> > two register files which operate entirely independently?
>
> As I mentioned before, they operate independently, but they sync up
> when we pass from one mode
> To the other. Another way to look at it is that they are mutually
> exclusive.
>
>
>
>
> Would it make sense to trigger the syncing between them explicitly from ARM
> code, rather than forcing the O3 to notice and do the copying? Then the
> copying, etc, wouldn't have to be generic, since it would be triggered by an
> ARM architectural mechanism.
>
>

I understand what you are saying, but just to be clear: I believe the copying 
per se is already ISA agnostic.
The O3 copying is really copying elements into vectors and viceversa; the O3 
model doesn't really
have any notion of the Arm architecture (but I do agree the *need* for copying 
is Arm oriented)

If your concern was more about the triggering mechanism, you can probably start 
everything from the Arm side, but I don't really know what’s the cleanest 
solution there. A quick and dirty implementation would extend the ExecContext 
interface as execution mode changes are often (but not always) triggered by 
instructions.
(SVC = syscall and ERET).
That is far from being a better solution; as you would add o3 specific logic to 
other cpus as well (probably empty stubs). You would also end up with o3 code 
in arch.

>
>
> > From what I can tell
> > the "V" registers of Neon in aarch64 overlap with the SVE registers,
> and the "Q"
> > registers of armv7 Neon overlap with the "S", "D", "Q" registers of
> the same,
> > but I think "V" and "Q" are independent? Maybe reused but not
> guaranteed to
> > alias?
> >
>
> I would say the rule of thumb for understanding AArch64-AArch32
> mapping (and it's the underlying cause of using different renaming modes) is 
> to
> bear in mind that AArch64, differently from AArch32, uses an unpacked
> approach for FP & SIMD registers.
> Prior to Armv8, smaller FP registers were packed into bigger registers
> [2]. Having for example 32 double precision registers (D0-D31) meant having a
> maximum of 16 quad word registers (Q0-Q15).
> This setup has been abandoned in Armv8 [3]. As an example, S1, or D1
> are not packed anymore in Q0. Those are in fact the 32/64 LSBits of Q1.
> This means the newly added (V16-V31) are not accessible in AArch32.
>
> So to answer your question regarding V and Q. Until Q/V15, they alias
> perfectly; V16-V31 are simply not
> Defined/accessible in AArch32 so they are not aliased.
>
> All AArch32 SIMD data is accessible from AArch64. It just won't stick to
> the same naming. AArch32 D1 and AArch64 D1 hold different data.
> If I really wanted to access AArch32 D1 from AArch64 I would have to
> read the 64 MSB of V0. This is a software and not an hardware problem (I just
> posted this example to stress the difference between aliasing and 
> reachability)
>
>
>
>
> Gotcha, makes sense.
>
>
>
> Richard kindly pointed me to the following SVE tutorial:
>
> https://gitlab.com/arm-hpc/training/arm-sve-tools
>
> But I believe it is worth noting we are actually interested on testing
> armv7 (AArch32) SIMD as well, so that won't probably be enough.
> I will dig more, and I will keep you posted
>
>
>
> Ok great, I'll take a look. Having *something* to test with will be a big leg 
> up,
> even if it isn't

[gem5-dev] Re: RFC: run python Black on gem5 python code

2021-03-04 Thread Jason Lowe-Power via gem5-dev 
Hi all,

Some responses inline below. The other thing I'll mention is that I believe
we can integrate it in a way where it's completely invisible to most
developers. You can edit a file with whatever style you want, then after
you run black it will be in the PEP8 style. From my experience, the only
things that need to be manually changed are quoted strings that are more
than 80 characters.

On Thu, Mar 4, 2021 at 6:49 AM Giacomo Travaglini via gem5-dev <
gem5-dev@gem5.org> wrote:

> Hi Jason,
>
> This is great news. When you say it is not configurable, do you mean it is
> not possible to suppress warning/errors or do you
> mean it is not possible to come up with custom rules?
>

There is a way to suppress changes by modifying the file with a comment `#
fmt: off`. You can also exclude files with a dotfile (like .gitignore).


>
> As I presume it's the first case, I am wondering if a different linter
> might provide such capabilities:
>
>
> https://books.agiliq.com/projects/essential-python-tools/en/latest/linters.html
>
> Is there a reason to prefer black over other linters?
>

It's owned by the python software foundation, which makes it seem
"official". Though the real reason is that it's the first one I looked at
and the only one I investigated.


>
> Kind regards
>
> Giacomo
>
> > -Original Message-
> > From: Gabe Black via gem5-dev 
> > Sent: 04 March 2021 03:59
> > To: gem5 Developer List 
> > Cc: Gabe Black 
> > Subject: [gem5-dev] Re: RFC: run python Black on gem5 python code
> >
> > I'm a little worried about the no exceptions part of that, since we
> might have
> > some weird restrictions that we have to do weird things to work around,
> but I
> > can't really think of an example of that off hand. I'd want to look at
> it to see
> > how much wiggle room there is in the style, since I think ironclad rules
> which
> > make no accommodation for occasional common sense are maybe more
> > trouble than they're worth. I'm not opposed to having at least some
> stated
> > standard for python though, and the "official" one seems like a pretty
> > reasonable choice. I guess it's fine with me, up until it causes me some
> sort of
> > problem :-)
>

I actually think it's a feature not a bug. With an "uncompromising Python
code formatter" there can be no arguments in code review about the style.


> >
> > Maybe the right thing to do would be to give it a shot but not make it
> > compulsory until we have a feeling for how much trouble it is.
> >
> >
> > Gabe
> >
> > On Wed, Mar 3, 2021 at 11:24 AM Bobby Bruce via gem5-dev  > d...@gem5.org  > wrote:
> >
> >
> > Sounds like a good idea to me.
> > ---
> >
> > Dr. Bobby R. Bruce
> > Room 2235,
> > Kemper Hall, UC Davis
> > Davis,
> > CA, 95616
> >
> >
> > web: https://www.bobbybruce.net
> >
> >
> >
> > On Wed, Mar 3, 2021 at 10:11 AM Daniel Carvalho via gem5-dev
> > mailto:gem5-dev@gem5.org> > wrote:
> >
> >
> > +1
> >
> >
> > Em quarta-feira, 3 de março de 2021 14:35:57 BRT, Jason
> > Lowe-Power via gem5-dev mailto:gem5-
> > d...@gem5.org> > escreveu:
> >
> >
> > Hi all,
> >
> > Right now, we don't have an official style guide for python.
> > Our style guide
> > (http://www.gem5.org/documentation/general_docs/development/coding_st
> > yle/) is very C++ focused.
> >
> > I would like to propose adopting a relatively strict PEP 8 style
> > guide: https://www.python.org/dev/peps/pep-0008. This is the "official"
> style
> > guide for python (as much as there is anything official). I say
> "relatively strict"
> > to mean that we will limit our exceptions *as much as possible*.
> >
> > To implement this, Andreas S. recently pointed me to the
> > "Black" package (https://pypi.org/project/black/) which automatically
> fixes
> > code style. I just tried it out with gem5art (patch coming soon) and
> found that
> > it works really well. The only downside is that it's not configurable at
> all.
> > Adding special cases would be almost impossible.
> >
> > Concrete and specific proposal:
> > - Adopt PEP 8 as our official style guide
> > - Use black to reformat all python code in src/
> > - Use black to reformat code in configs/
> > - Use black to reformat other python code
> >
> > - Use black as part of our commit hook to make sure all future
> > python is formatted to PEP 8
> >
> > Let me know what you think!
> >
> > Cheers,
> > Jason
> >
> > ___
> > gem5-dev mailing list -- gem5-dev@gem5.org  > d...@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  > d...@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: RFC: run python Black on gem5 python code

2021-03-04 Thread Giacomo Travaglini via gem5-dev 
Hi Jason,

This is great news. When you say it is not configurable, do you mean it is not 
possible to suppress warning/errors or do you
mean it is not possible to come up with custom rules?

As I presume it's the first case, I am wondering if a different linter might 
provide such capabilities:

https://books.agiliq.com/projects/essential-python-tools/en/latest/linters.html

Is there a reason to prefer black over other linters?

Kind regards

Giacomo

> -Original Message-
> From: Gabe Black via gem5-dev 
> Sent: 04 March 2021 03:59
> To: gem5 Developer List 
> Cc: Gabe Black 
> Subject: [gem5-dev] Re: RFC: run python Black on gem5 python code
>
> I'm a little worried about the no exceptions part of that, since we might have
> some weird restrictions that we have to do weird things to work around, but I
> can't really think of an example of that off hand. I'd want to look at it to 
> see
> how much wiggle room there is in the style, since I think ironclad rules which
> make no accommodation for occasional common sense are maybe more
> trouble than they're worth. I'm not opposed to having at least some stated
> standard for python though, and the "official" one seems like a pretty
> reasonable choice. I guess it's fine with me, up until it causes me some sort 
> of
> problem :-)
>
> Maybe the right thing to do would be to give it a shot but not make it
> compulsory until we have a feeling for how much trouble it is.
>
>
> Gabe
>
> On Wed, Mar 3, 2021 at 11:24 AM Bobby Bruce via gem5-dev  d...@gem5.org  > wrote:
>
>
> Sounds like a good idea to me.
> ---
>
> Dr. Bobby R. Bruce
> Room 2235,
> Kemper Hall, UC Davis
> Davis,
> CA, 95616
>
>
> web: https://www.bobbybruce.net
>
>
>
> On Wed, Mar 3, 2021 at 10:11 AM Daniel Carvalho via gem5-dev
> mailto:gem5-dev@gem5.org> > wrote:
>
>
> +1
>
>
> Em quarta-feira, 3 de março de 2021 14:35:57 BRT, Jason
> Lowe-Power via gem5-dev mailto:gem5-
> d...@gem5.org> > escreveu:
>
>
> Hi all,
>
> Right now, we don't have an official style guide for python.
> Our style guide
> (http://www.gem5.org/documentation/general_docs/development/coding_st
> yle/) is very C++ focused.
>
> I would like to propose adopting a relatively strict PEP 8 style
> guide: https://www.python.org/dev/peps/pep-0008. This is the "official" style
> guide for python (as much as there is anything official). I say "relatively 
> strict"
> to mean that we will limit our exceptions *as much as possible*.
>
> To implement this, Andreas S. recently pointed me to the
> "Black" package (https://pypi.org/project/black/) which automatically fixes
> code style. I just tried it out with gem5art (patch coming soon) and found 
> that
> it works really well. The only downside is that it's not configurable at all.
> Adding special cases would be almost impossible.
>
> Concrete and specific proposal:
> - Adopt PEP 8 as our official style guide
> - Use black to reformat all python code in src/
> - Use black to reformat code in configs/
> - Use black to reformat other python code
>
> - Use black as part of our commit hook to make sure all future
> python is formatted to PEP 8
>
> Let me know what you think!
>
> Cheers,
> Jason
>
> ___
> gem5-dev mailing list -- gem5-dev@gem5.org  d...@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  d...@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  d...@gem5.org>
> To unsubscribe send an email to gem5-dev-le...@gem5.org
> 
> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s

IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.
___
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: Switch the AAPCS ABIs to .as<>() instead of .laneView<>().

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


Change subject: arch-arm: Switch the AAPCS ABIs to .as<>() instead  
of .laneView<>().

..

arch-arm: Switch the AAPCS ABIs to .as<>() instead of .laneView<>().

Change-Id: I9e9c7163db4c061af00111b8dc959c364c6b7ae6
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/41898
Reviewed-by: Giacomo Travaglini 
Maintainer: Giacomo Travaglini 
Tested-by: kokoro 
---
M src/arch/arm/aapcs32.hh
M src/arch/arm/aapcs64.hh
2 files changed, 6 insertions(+), 6 deletions(-)

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



diff --git a/src/arch/arm/aapcs32.hh b/src/arch/arm/aapcs32.hh
index a1345bd..c450237 100644
--- a/src/arch/arm/aapcs32.hh
+++ b/src/arch/arm/aapcs32.hh
@@ -463,7 +463,7 @@

 RegId id(VecRegClass, 0);
 auto reg = tc->readVecReg(id);
-reg.laneView() = f;
+reg.as()[0] = f;
 tc->setVecReg(id, reg);
 };
 };
@@ -487,7 +487,7 @@

 RegId id(VecRegClass, reg);
 auto val = tc->readVecReg(id);
-return val.laneView(lane);
+return val.as()[lane];
 }

 return loadFromStack(tc, state);
@@ -558,7 +558,7 @@

 RegId id(VecRegClass, reg);
 auto val = tc->readVecReg(id);
-ha[i] = val.laneView(lane);
+ha[i] = val.as()[lane];
 }
 return ha;
 }
@@ -605,7 +605,7 @@

 RegId id(VecRegClass, reg);
 auto val = tc->readVecReg(id);
-val.laneView(lane) = ha[i];
+val.as()[lane] = ha[i];
 tc->setVecReg(id, val);
 }
 }
diff --git a/src/arch/arm/aapcs64.hh b/src/arch/arm/aapcs64.hh
index fb7b8f8..ddd5606 100644
--- a/src/arch/arm/aapcs64.hh
+++ b/src/arch/arm/aapcs64.hh
@@ -186,7 +186,7 @@
 {
 if (state.nsrn <= state.MAX_SRN) {
 RegId id(VecRegClass, state.nsrn++);
-return tc->readVecReg(id).laneView();
+return tc->readVecReg(id).as()[0];
 }

 return loadFromStack(tc, state);
@@ -203,7 +203,7 @@
 {
 RegId id(VecRegClass, 0);
 auto reg = tc->readVecReg(id);
-reg.laneView() = f;
+reg.as()[0] = f;
 tc->setVecReg(id, reg);
 }
 };

--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/41898
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: I9e9c7163db4c061af00111b8dc959c364c6b7ae6
Gerrit-Change-Number: 41898
Gerrit-PatchSet: 2
Gerrit-Owner: Gabe Black 
Gerrit-Reviewer: Andreas Sandberg 
Gerrit-Reviewer: Gabe Black 
Gerrit-Reviewer: Giacomo Travaglini 
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]: configs, mem: MemInterface generating its own controller

2021-03-04 Thread Giacomo Travaglini (Gerrit) via gem5-dev 
Giacomo Travaglini has uploaded this change for review. (  
https://gem5-review.googlesource.com/c/public/gem5/+/42074 )



Change subject: configs, mem: MemInterface generating its own controller
..

configs, mem: MemInterface generating its own controller

We are adding a controller method to MemInterface objects making
them able to generate the appropriate memory controller.

This will bring the following benefits

a) Semplification: It will simplify MemConfig.config_mem
b) Reusability: Scripts not using config_mem
won't have to duplicate the if...else checks
c) Modularity: Users will be able to define their own
dram interfaces without needing to handle the mem_ctrl
mapping in the shared MemConfig.py module

Change-Id: I4b836fd7c91675cf7aacc644f25989484d5be3ec
Signed-off-by: Giacomo Travaglini 
---
M configs/common/MemConfig.py
M src/mem/DRAMInterface.py
M src/mem/SimpleMemory.py
M src/mem/qos/QoSMemSinkInterface.py
4 files changed, 37 insertions(+), 22 deletions(-)



diff --git a/configs/common/MemConfig.py b/configs/common/MemConfig.py
index 6e78be5..7c32ea7 100644
--- a/configs/common/MemConfig.py
+++ b/configs/common/MemConfig.py
@@ -1,4 +1,4 @@
-# Copyright (c) 2013, 2017, 2020 ARM Limited
+# Copyright (c) 2013, 2017, 2020-2021 Arm Limited
 # All rights reserved.
 #
 # The license below extends only to copyright in the software and shall
@@ -218,24 +218,7 @@
 "latency to 1ns.")

 # Create the controller that will drive the interface
-if opt_mem_type == "HMC_2500_1x32":
-# The static latency of the vault controllers is  
estimated

-# to be smaller than a full DRAM channel controller
-mem_ctrl = m5.objects.MemCtrl(min_writes_per_switch =  
8,
- static_backend_latency  
= '4ns',
- static_frontend_latency  
= '4ns')

-elif opt_mem_type == "SimpleMemory":
-mem_ctrl = m5.objects.SimpleMemory()
-elif opt_mem_type == "QoSMemSinkInterface":
-mem_ctrl = m5.objects.QoSMemSinkCtrl()
-else:
-mem_ctrl = m5.objects.MemCtrl()
-
-# Hookup the controller to the interface and add to the  
list

-if opt_mem_type == "QoSMemSinkInterface":
-mem_ctrl.interface = dram_intf
-elif opt_mem_type != "SimpleMemory":
-mem_ctrl.dram = dram_intf
+mem_ctrl = dram_intf.controller()

 mem_ctrls.append(mem_ctrl)

diff --git a/src/mem/DRAMInterface.py b/src/mem/DRAMInterface.py
index 4f59498..16a4f8b 100644
--- a/src/mem/DRAMInterface.py
+++ b/src/mem/DRAMInterface.py
@@ -1,4 +1,4 @@
-# Copyright (c) 2012-2020 ARM Limited
+# Copyright (c) 2012-2021 Arm Limited
 # All rights reserved.
 #
 # The license below extends only to copyright in the software and shall
@@ -38,6 +38,7 @@
 # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

+from m5.objects.MemCtrl import MemCtrl
 from m5.objects.MemInterface import *

 # Enum for the page policy, either open, open_adaptive, close, or
@@ -254,6 +255,15 @@
 # Second voltage range defined by some DRAMs
 VDD2 = Param.Voltage("0V", "2nd Voltage Range")

+def controller(self):
+"""
+Instantiate the memory controller and bind it to
+the current interface.
+"""
+controller = MemCtrl()
+controller.dram = self
+return controller
+
 # A single DDR3-1600 x64 channel (one command and address bus), with
 # timings based on a DDR3-1600 4 Gbit datasheet (Micron MT41J512M8) in
 # an 8x8 configuration.
@@ -424,6 +434,15 @@
 write_buffer_size = 32
 read_buffer_size = 32

+def controller(self):
+"""
+Instantiate the memory controller and bind it to
+the current interface.
+"""
+return MemCtrl(min_writes_per_switch = 8,
+   static_backend_latency = '4ns',
+   static_frontend_latency = '4ns')
+
 # A single DDR3-2133 x64 channel refining a selected subset of the
 # options for the DDR-1600 configuration, based on the same DDR3-1600
 # 4 Gbit datasheet (Micron MT41J512M8). Most parameters are kept
diff --git a/src/mem/SimpleMemory.py b/src/mem/SimpleMemory.py
index e8eac69..0e059e8 100644
--- a/src/mem/SimpleMemory.py
+++ b/src/mem/SimpleMemory.py
@@ -1,4 +1,4 @@
-# Copyright (c) 2012-2013 ARM Limited
+# Copyright (c) 2012-2013, 2021 Arm Limited
 # All rights reserved.
 #
 # The license below extends only to copyright in the software and shall
@@ -49,3 +49,7 @@
 # representative of a x64 DDR3-1600 channel.
 bandwidth = Param.MemoryBandwidth('12.8GiB/s',

[gem5-dev] Change in gem5/gem5[develop]: configs: Remove unused argument from create_mem_intf

2021-03-04 Thread Giacomo Travaglini (Gerrit) via gem5-dev 
Giacomo Travaglini has uploaded this change for review. (  
https://gem5-review.googlesource.com/c/public/gem5/+/42075 )



Change subject: configs: Remove unused argument from create_mem_intf
..

configs: Remove unused argument from create_mem_intf

The number of memory controllers is not actually used by the
create_mem_intf function

Change-Id: I8663b38938de9b62b778679c1bc5c7c6e15a60da
Signed-off-by: Giacomo Travaglini 
---
M configs/common/MemConfig.py
1 file changed, 3 insertions(+), 3 deletions(-)



diff --git a/configs/common/MemConfig.py b/configs/common/MemConfig.py
index 7c32ea7..d71dc61 100644
--- a/configs/common/MemConfig.py
+++ b/configs/common/MemConfig.py
@@ -37,7 +37,7 @@
 from common import ObjectList
 from common import HMC

-def create_mem_intf(intf, r, i, nbr_mem_ctrls, intlv_bits, intlv_size,
+def create_mem_intf(intf, r, i, intlv_bits, intlv_size,
 xor_low_bit):
 """
 Helper function for creating a single memoy controller from the given
@@ -199,7 +199,7 @@
 for i in range(nbr_mem_ctrls):
 if opt_mem_type and (not opt_nvm_type or range_iter % 2 != 0):
 # Create the DRAM interface
-dram_intf = create_mem_intf(intf, r, i, nbr_mem_ctrls,
+dram_intf = create_mem_intf(intf, r, i,
 intlv_bits, intlv_size,  
opt_xor_low_bit)


 # Set the number of ranks based on the command-line
@@ -223,7 +223,7 @@
 mem_ctrls.append(mem_ctrl)

 elif opt_nvm_type and (not opt_mem_type or range_iter % 2 ==  
0):

-nvm_intf = create_mem_intf(n_intf, r, i, nbr_mem_ctrls,
+nvm_intf = create_mem_intf(n_intf, r, i,
intlv_bits, intlv_size)
 # Set the number of ranks based on the command-line
 # options if it was explicitly set

--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/42075
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: I8663b38938de9b62b778679c1bc5c7c6e15a60da
Gerrit-Change-Number: 42075
Gerrit-PatchSet: 1
Gerrit-Owner: Giacomo Travaglini 
Gerrit-MessageType: newchange
___
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: Simplify some code in the guest ABI mechanism.

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


Change subject: sim: Simplify some code in the guest ABI mechanism.
..

sim: Simplify some code in the guest ABI mechanism.

Instead of using recursively applied templates to accumulate a series of
wrapper lambdas which dispatch to a call, use pure parameter pack
expansion. This has two benefits. One, it makes the code simpler(ish) and
easier to understand. The parameter pack machinery is still intrinsically
fairly tricky, but there's less of it and it's a fairly straightforward
application of that mechanism.

Also, a nice side benefit is that the template for simcall dispatch will
expand to a small fixed number of functions which do all their work
locally, instead of having a new function for each layer of the onion,
one per parameter, and no calls through lambdas. That should hopefully
make debugging easier, and produce less bookkeeping overhead as far as
really long names, lots of functions, etc.

This code, specifically the code in dispatch.hh, can be simplified even
further in the future once we start using c++17 which is if constexpr,
and std::apply which explodes a tuple and uses its components as
arguments to a function, something I'm doing manually here.

Change-Id: If7c9234cc1014101211474c2ec20362702cf78c2
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/41600
Reviewed-by: Gabe Black 
Maintainer: Gabe Black 
Tested-by: kokoro 
---
M src/sim/guest_abi.hh
M src/sim/guest_abi/dispatch.hh
M src/sim/guest_abi/layout.hh
3 files changed, 48 insertions(+), 112 deletions(-)

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



diff --git a/src/sim/guest_abi.hh b/src/sim/guest_abi.hh
index ea3325f..75c4e00 100644
--- a/src/sim/guest_abi.hh
+++ b/src/sim/guest_abi.hh
@@ -51,7 +51,7 @@
 // types will be zero initialized.
 auto state = GuestABI::initializeState(tc);
 GuestABI::prepareForFunction(tc, state);
-return GuestABI::callFrom(tc, state,  
target);
+return GuestABI::callFrom(tc, state,  
target);

 }

 template 
@@ -86,7 +86,7 @@
 // types will be zero initialized.
 auto state = GuestABI::initializeState(tc);
 GuestABI::prepareForArguments(tc, state);
-GuestABI::callFrom(tc, state, target);
+GuestABI::callFrom(tc, state, target);
 }

 template 
@@ -113,7 +113,7 @@

 GuestABI::prepareForFunction(tc, state);
 ss << name;
-GuestABI::dumpArgsFrom(0, ss, tc, state);
+GuestABI::dumpArgsFrom(ss, tc, state);
 return ss.str();
 }

diff --git a/src/sim/guest_abi/dispatch.hh b/src/sim/guest_abi/dispatch.hh
index bc365b9..8f3a4ac 100644
--- a/src/sim/guest_abi/dispatch.hh
+++ b/src/sim/guest_abi/dispatch.hh
@@ -30,8 +30,11 @@

 #include 
 #include 
+#include 
 #include 
+#include 

+#include "base/compiler.hh"
 #include "sim/guest_abi/definition.hh"
 #include "sim/guest_abi/layout.hh"

@@ -50,114 +53,60 @@
  * still possible to support by redefining these functions..
  */

-// With no arguments to gather, call the target function and store the
-// result.
-template 
-static typename std::enable_if_t::value && store_ret,  
Ret>

-callFrom(ThreadContext *tc, typename ABI::State ,
-std::function target)
+template 
+static inline typename std::enable_if_t
+callFromHelper(Target , ThreadContext *tc, State , Args  
&,

+std::index_sequence)
 {
-Ret ret = target(tc);
+return target(tc, std::get(args)...);
+}
+
+template 
+static inline typename std::enable_if_t
+callFromHelper(Target , ThreadContext *tc, State , Args  
&,

+std::index_sequence)
+{
+Ret ret = target(tc, std::get(args)...);
 storeResult(tc, ret, state);
 return ret;
 }

-template 
-static typename std::enable_if_t::value && !store_ret,  
Ret>

+template 
+static inline Ret
 callFrom(ThreadContext *tc, typename ABI::State ,
-std::function target)
+std::function target)
 {
-return target(tc);
+// Extract all the arguments from the thread context. Braced  
initializers

+// are evaluated from left to right.
+auto args = std::tuple{getArgument(tc, state)...};
+
+// Call the wrapper which will call target.
+return callFromHelper(
+target, tc, state, std::move(args),
+std::make_index_sequence{});
 }

-// With no arguments to gather and nothing to return, call the target  
function.

-template 
-static void
-callFrom(ThreadContext *tc, typename ABI::State ,
-std::function target)
-{
-target(tc);
-}
-
-// Recursively gather arguments for target from tc until we get to the base
-// case above.
-template 
-static typename std::enable_if_t::value, Ret>
-callFrom(ThreadContext *tc, typename ABI::State ,
-std::function target)
-{
-// Extract the next argument from the thread context.
-NextArg next = getArgument(tc, state);
-
-//

[gem5-dev] Change in gem5/gem5[develop]: base: Add a macro to expand parameter pack expressions in order.

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


Change subject: base: Add a macro to expand parameter pack expressions in  
order.

..

base: Add a macro to expand parameter pack expressions in order.

This wraps up the strange compiler goop necessary to evaluate
expressions based on parameter pack expansions in order.

Change-Id: I16fbd53d22492a8c20524e3ef8bb8ff5e5d59b14
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/42033
Reviewed-by: Gabe Black 
Maintainer: Gabe Black 
Tested-by: kokoro 
---
M src/base/compiler.hh
1 file changed, 10 insertions(+), 0 deletions(-)

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



diff --git a/src/base/compiler.hh b/src/base/compiler.hh
index 643352c..c003bfa 100644
--- a/src/base/compiler.hh
+++ b/src/base/compiler.hh
@@ -112,6 +112,16 @@
 // we can't do that with direct substitution.
 #  define M5_LIKELY(cond) __builtin_expect(!!(cond), 1)
 #  define M5_UNLIKELY(cond) __builtin_expect(!!(cond), 0)
+
+// Evaluate an expanded parameter pack in order. Multiple arguments can be
+// passed in which be evaluated in order relative to each other as a group.
+// The argument(s) must include a parameter pack to expand. This works  
because
+// the elements of a brace inclosed initializer list are evaluated in  
order,

+// as are the arguments to the comma operator, which evaluates to the last
+// value. This is compiler specific because it uses variadic macros.
+#define M5_FOR_EACH_IN_PACK(...) \
+do { M5_VAR_USED int i[] = { 0, ((void)(__VA_ARGS__), 0)... }; } while  
(false)

+
 #else
 #  error "Don't know what to do for your compiler."
 #endif

--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/42033
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: I16fbd53d22492a8c20524e3ef8bb8ff5e5d59b14
Gerrit-Change-Number: 42033
Gerrit-PatchSet: 4
Gerrit-Owner: Gabe Black 
Gerrit-Reviewer: Bobby R. Bruce 
Gerrit-Reviewer: Daniel Carvalho 
Gerrit-Reviewer: Gabe Black 
Gerrit-Reviewer: Jason Lowe-Power 
Gerrit-Reviewer: Jason Lowe-Power 
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[release-staging-v21-0]: arch-arm: Fix atomics permission checks in TLB

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


Change subject: arch-arm: Fix atomics permission checks in TLB
..

arch-arm: Fix atomics permission checks in TLB

For stage 2 translations, atomic accesses were not checking the
access permission bits in the page table descriptors, and were
instead wrongly using the nature of the request itself
(r/w booleans).

Change-Id: I27fbc95f04ea659e77ad5a3afb551873c9c971f0
Signed-off-by: Giacomo Travaglini 
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/42073
Reviewed-by: Jason Lowe-Power 
Reviewed-by: Richard Cooper 
Reviewed-by: Bobby R. Bruce 
Maintainer: Jason Lowe-Power 
Maintainer: Bobby R. Bruce 
Tested-by: kokoro 
---
M src/arch/arm/tlb.cc
1 file changed, 1 insertion(+), 2 deletions(-)

Approvals:
  Jason Lowe-Power: Looks good to me, approved; Looks good to me, approved
  Bobby R. Bruce: Looks good to me, approved; Looks good to me, approved
  Richard Cooper: Looks good to me, approved
  kokoro: Regressions pass



diff --git a/src/arch/arm/tlb.cc b/src/arch/arm/tlb.cc
index fd72d25..8e5b3ca 100644
--- a/src/arch/arm/tlb.cc
+++ b/src/arch/arm/tlb.cc
@@ -872,8 +872,7 @@
 // sctlr.wxn overrides the xn bit
 grant = !wxn && !xn;
 } else if (is_atomic) {
-grant = r && w;
-grant_read = r;
+grant = hap;
 } else if (is_write) {
 grant = hap & 0x2;
 } else { // is_read

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


Gerrit-Project: public/gem5
Gerrit-Branch: release-staging-v21-0
Gerrit-Change-Id: I27fbc95f04ea659e77ad5a3afb551873c9c971f0
Gerrit-Change-Number: 42073
Gerrit-PatchSet: 2
Gerrit-Owner: Giacomo Travaglini 
Gerrit-Reviewer: Andreas Sandberg 
Gerrit-Reviewer: Bobby R. Bruce 
Gerrit-Reviewer: Giacomo Travaglini 
Gerrit-Reviewer: Jason Lowe-Power 
Gerrit-Reviewer: Richard Cooper 
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]: cpu: Style fixes in the base and O3 dynamic inst classes.

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


Change subject: cpu: Style fixes in the base and O3 dynamic inst classes.
..

cpu: Style fixes in the base and O3 dynamic inst classes.

Change-Id: Idfd8e71856931fa101e00c58a2aa4018d076
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/42093
Reviewed-by: Daniel Carvalho 
Maintainer: Gabe Black 
Tested-by: kokoro 
---
M src/cpu/base_dyn_inst.hh
M src/cpu/o3/dyn_inst.hh
2 files changed, 27 insertions(+), 20 deletions(-)

Approvals:
  Daniel Carvalho: Looks good to me, approved
  Gabe Black: Looks good to me, approved
  kokoro: Regressions pass



diff --git a/src/cpu/base_dyn_inst.hh b/src/cpu/base_dyn_inst.hh
index 68a6bb3..a5a842a 100644
--- a/src/cpu/base_dyn_inst.hh
+++ b/src/cpu/base_dyn_inst.hh
@@ -729,8 +729,11 @@
 OpClass opClass() const { return staticInst->opClass(); }

 /** Returns the branch target address. */
-TheISA::PCState branchTarget() const
-{ return staticInst->branchTarget(pc); }
+TheISA::PCState
+branchTarget() const
+{
+return staticInst->branchTarget(pc);
+}

 /** Returns the number of source registers. */
 size_t numSrcRegs() const { return regs.numSrcs(); }
@@ -1016,11 +1019,15 @@

 /** Return whether dest registers' pinning status updated after squash  
*/

 bool
-isPinnedRegsSquashDone() const { return status[PinnedRegsSquashDone]; }
+isPinnedRegsSquashDone() const
+{
+return status[PinnedRegsSquashDone];
+}

 /** Sets dest registers' status updated after squash */
 void
-setPinnedRegsSquashDone() {
+setPinnedRegsSquashDone()
+{
 assert(!status[PinnedRegsSquashDone]);
 status.set(PinnedRegsSquashDone);
 }
diff --git a/src/cpu/o3/dyn_inst.hh b/src/cpu/o3/dyn_inst.hh
index f084368..7a54c7f 100644
--- a/src/cpu/o3/dyn_inst.hh
+++ b/src/cpu/o3/dyn_inst.hh
@@ -184,37 +184,37 @@
 this->thread->noSquashFromTC = no_squash_from_TC;
 }

-void forwardOldRegs()
+void
+forwardOldRegs()
 {

 for (int idx = 0; idx < this->numDestRegs(); idx++) {
 PhysRegIdPtr prev_phys_reg = this->regs.prevDestIdx(idx);
-const RegId& original_dest_reg =
-this->staticInst->destRegIdx(idx);
+const RegId& original_dest_reg =  
this->staticInst->destRegIdx(idx);

 switch (original_dest_reg.classValue()) {
   case IntRegClass:
 this->setIntRegOperand(this->staticInst.get(), idx,
-   this->cpu->readIntReg(prev_phys_reg));
+this->cpu->readIntReg(prev_phys_reg));
 break;
   case FloatRegClass:
 this->setFloatRegOperandBits(this->staticInst.get(), idx,
-   this->cpu->readFloatReg(prev_phys_reg));
+this->cpu->readFloatReg(prev_phys_reg));
 break;
   case VecRegClass:
 this->setVecRegOperand(this->staticInst.get(), idx,
-   this->cpu->readVecReg(prev_phys_reg));
+this->cpu->readVecReg(prev_phys_reg));
 break;
   case VecElemClass:
 this->setVecElemOperand(this->staticInst.get(), idx,
-   this->cpu->readVecElem(prev_phys_reg));
+this->cpu->readVecElem(prev_phys_reg));
 break;
   case VecPredRegClass:
 this->setVecPredRegOperand(this->staticInst.get(), idx,
-   this->cpu->readVecPredReg(prev_phys_reg));
+this->cpu->readVecPredReg(prev_phys_reg));
 break;
   case CCRegClass:
 this->setCCRegOperand(this->staticInst.get(), idx,
-   this->cpu->readCCReg(prev_phys_reg));
+this->cpu->readCCReg(prev_phys_reg));
 break;
   case MiscRegClass:
 // no need to forward misc reg values
@@ -309,25 +309,25 @@
 {
 return cpu->template setVecLane(this->regs.renamedDestIdx(idx),  
val);

 }
-virtual void
+void
 setVecLaneOperand(const StaticInst *si, int idx,
 const LaneData& val) override
 {
 return setVecLaneOperandT(si, idx, val);
 }
-virtual void
+void
 setVecLaneOperand(const StaticInst *si, int idx,
 const LaneData& val) override
 {
 return setVecLaneOperandT(si, idx, val);
 }
-virtual void
+void
 setVecLaneOperand(const StaticInst *si, int idx,
 const LaneData& val) override
 {
 return setVecLaneOperandT(si, idx, val);
 }
-virtual void
+void
 setVecLaneOperand(const StaticInst *si, int idx,