Re: [gem5-dev] Review Request 2497: mem: Make the requests carried by packets const

[Steve Reinhardt via gem5-dev]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/2497/#review5597
---

Ship it!


Ship It!

- Steve Reinhardt


On Nov. 30, 2014, 1:19 a.m., Andreas Hansson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/2497/
> ---
> 
> (Updated Nov. 30, 2014, 1:19 a.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 10570:c19d57302758
> ---
> mem: Make the requests carried by packets const
> 
> This adds a basic level of sanity checking to the packet by ensuring
> that a request is not modified once the packet is created. The only
> issue that had to be worked around is the relaying of
> software-prefetches in the cache. The specific situation is now solved
> by first copying the request, and then creating a new packet
> accordingly.
> 
> 
> Diffs
> -
> 
>   src/mem/cache/cache_impl.hh dd04eb06ad42 
>   src/mem/packet.hh dd04eb06ad42 
> 
> Diff: http://reviews.gem5.org/r/2497/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andreas Hansson
> 
>

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

Re: [gem5-dev] Review Request 2497: mem: Make the requests carried by packets const

[Andreas Hansson via gem5-dev]

> On Nov. 29, 2014, 9:12 p.m., Steve Reinhardt wrote:
> > src/mem/cache/cache_impl.hh, line 627
> > 
> >
> > This part seems kind of ad hoc... why is it that src/dest need to be 
> > copied but nothing else?  I believe it's true, it just seems arbitrary with 
> > nothing else to go on here.
> > 
> > Also seems like we should have confidence that the packet is created 
> > cleanly (the last 4 asserts seem like overkill).

Remnants of my debugging. I've removed it and bumped the patch.


- Andreas


---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/2497/#review5576
---


On Nov. 30, 2014, 9:19 a.m., Andreas Hansson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/2497/
> ---
> 
> (Updated Nov. 30, 2014, 9:19 a.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 10570:c19d57302758
> ---
> mem: Make the requests carried by packets const
> 
> This adds a basic level of sanity checking to the packet by ensuring
> that a request is not modified once the packet is created. The only
> issue that had to be worked around is the relaying of
> software-prefetches in the cache. The specific situation is now solved
> by first copying the request, and then creating a new packet
> accordingly.
> 
> 
> Diffs
> -
> 
>   src/mem/cache/cache_impl.hh dd04eb06ad42 
>   src/mem/packet.hh dd04eb06ad42 
> 
> Diff: http://reviews.gem5.org/r/2497/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andreas Hansson
> 
>

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

Re: [gem5-dev] Review Request 2497: mem: Make the requests carried by packets const

[Andreas Hansson via gem5-dev]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/2497/
---

(Updated Nov. 30, 2014, 9:19 a.m.)


Review request for Default.


Repository: gem5


Description (updated)
---

Changeset 10570:c19d57302758
---
mem: Make the requests carried by packets const

This adds a basic level of sanity checking to the packet by ensuring
that a request is not modified once the packet is created. The only
issue that had to be worked around is the relaying of
software-prefetches in the cache. The specific situation is now solved
by first copying the request, and then creating a new packet
accordingly.


Diffs (updated)
-

  src/mem/cache/cache_impl.hh dd04eb06ad42 
  src/mem/packet.hh dd04eb06ad42 

Diff: http://reviews.gem5.org/r/2497/diff/


Testing
---


Thanks,

Andreas Hansson

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

Re: [gem5-dev] Review Request 2497: mem: Make the requests carried by packets const

[Steve Reinhardt via gem5-dev]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/2497/#review5576
---



src/mem/cache/cache_impl.hh


Is one vs. two spaces after periods in the gem5 style guide?  :)



src/mem/cache/cache_impl.hh


This part seems kind of ad hoc... why is it that src/dest need to be copied 
but nothing else?  I believe it's true, it just seems arbitrary with nothing 
else to go on here.

Also seems like we should have confidence that the packet is created 
cleanly (the last 4 asserts seem like overkill).


- Steve Reinhardt


On Nov. 16, 2014, 10:15 p.m., Andreas Hansson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/2497/
> ---
> 
> (Updated Nov. 16, 2014, 10:15 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 10547:e8cae196bce7
> ---
> mem: Make the requests carried by packets const
> 
> This adds a basic level of sanity checking to the packet by ensuring
> that a request is not modified once the packet is created. The only
> issue that had to be worked around is the relaying of
> software-prefetches in the cache. The specific situation is now solved
> by first copying the request, and then creating a new packet
> accordingly.
> 
> 
> Diffs
> -
> 
>   src/mem/cache/cache_impl.hh 1a9e235cab09 
>   src/mem/packet.hh 1a9e235cab09 
> 
> Diff: http://reviews.gem5.org/r/2497/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andreas Hansson
> 
>

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

Re: [gem5-dev] Review Request 2497: mem: Make the requests carried by packets const

[Andreas Hansson via gem5-dev]

> On Nov. 20, 2014, 5:42 p.m., Nilay Vaish wrote:
> > src/mem/cache/cache_impl.hh, line 626
> > 
> >
> > Why do we need the allocate() call? Since we know what the request is, 
> > can we not perform the allocation in the constructor itself?

Have a look at: http://reviews.gem5.org/r/2499/

The whole ownership issue is rather confusing, and these patches try to make it 
a bit less so.


- Andreas


---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/2497/#review5502
---


On Nov. 17, 2014, 6:15 a.m., Andreas Hansson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/2497/
> ---
> 
> (Updated Nov. 17, 2014, 6:15 a.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 10547:e8cae196bce7
> ---
> mem: Make the requests carried by packets const
> 
> This adds a basic level of sanity checking to the packet by ensuring
> that a request is not modified once the packet is created. The only
> issue that had to be worked around is the relaying of
> software-prefetches in the cache. The specific situation is now solved
> by first copying the request, and then creating a new packet
> accordingly.
> 
> 
> Diffs
> -
> 
>   src/mem/cache/cache_impl.hh 1a9e235cab09 
>   src/mem/packet.hh 1a9e235cab09 
> 
> Diff: http://reviews.gem5.org/r/2497/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andreas Hansson
> 
>

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

Re: [gem5-dev] Review Request 2497: mem: Make the requests carried by packets const

[Andreas Hansson via gem5-dev]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/2497/#review5503
---



src/mem/cache/cache_impl.hh


Someone has to allocate storage for the packet, and we either do that using 
"static" pointers where the packet carries a pointer around, but is not 
responsible for the ownership, or "dynamic" where the ownership lies with the 
packet itself.

We cannot allocate the space in the constructor as the "static" data does 
not rely on it.


- Andreas Hansson


On Nov. 17, 2014, 6:15 a.m., Andreas Hansson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/2497/
> ---
> 
> (Updated Nov. 17, 2014, 6:15 a.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 10547:e8cae196bce7
> ---
> mem: Make the requests carried by packets const
> 
> This adds a basic level of sanity checking to the packet by ensuring
> that a request is not modified once the packet is created. The only
> issue that had to be worked around is the relaying of
> software-prefetches in the cache. The specific situation is now solved
> by first copying the request, and then creating a new packet
> accordingly.
> 
> 
> Diffs
> -
> 
>   src/mem/cache/cache_impl.hh 1a9e235cab09 
>   src/mem/packet.hh 1a9e235cab09 
> 
> Diff: http://reviews.gem5.org/r/2497/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andreas Hansson
> 
>

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

Re: [gem5-dev] Review Request 2497: mem: Make the requests carried by packets const

[Nilay Vaish via gem5-dev]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/2497/#review5502
---



src/mem/cache/cache_impl.hh


Why do we need the allocate() call? Since we know what the request is, can 
we not perform the allocation in the constructor itself?


- Nilay Vaish


On Nov. 17, 2014, 6:15 a.m., Andreas Hansson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/2497/
> ---
> 
> (Updated Nov. 17, 2014, 6:15 a.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 10547:e8cae196bce7
> ---
> mem: Make the requests carried by packets const
> 
> This adds a basic level of sanity checking to the packet by ensuring
> that a request is not modified once the packet is created. The only
> issue that had to be worked around is the relaying of
> software-prefetches in the cache. The specific situation is now solved
> by first copying the request, and then creating a new packet
> accordingly.
> 
> 
> Diffs
> -
> 
>   src/mem/cache/cache_impl.hh 1a9e235cab09 
>   src/mem/packet.hh 1a9e235cab09 
> 
> Diff: http://reviews.gem5.org/r/2497/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andreas Hansson
> 
>

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

[gem5-dev] Review Request 2497: mem: Make the requests carried by packets const

[Andreas Hansson via gem5-dev]

---
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/2497/
---

Review request for Default.


Repository: gem5


Description
---

Changeset 10547:e8cae196bce7
---
mem: Make the requests carried by packets const

This adds a basic level of sanity checking to the packet by ensuring
that a request is not modified once the packet is created. The only
issue that had to be worked around is the relaying of
software-prefetches in the cache. The specific situation is now solved
by first copying the request, and then creating a new packet
accordingly.


Diffs
-

  src/mem/cache/cache_impl.hh 1a9e235cab09 
  src/mem/packet.hh 1a9e235cab09 

Diff: http://reviews.gem5.org/r/2497/diff/


Testing
---


Thanks,

Andreas Hansson

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