Re: [gem5-dev] Review Request 3535: gpu-compute: Adding context serialization methods to Wavefront

[Andreas Hansson]

> On June 30, 2016, 7:17 a.m., Andreas Hansson wrote:
> > src/gpu-compute/wavefront.hh, line 389
> > 
> >
> > Surely one of these should be const
> 
> Andreas Hansson wrote:
> I do not see this being addressed. getContext should be const I would 
> imagine.
> 
> Alexandru Dutu wrote:
> I see, you meant the functions to be const. Well the functions can't 
> actually be const as both of them are calling non-const functions (i.e. remap 
> and emplace_back).

It does seem rather alarming that a "getter" is non const. It would be good to 
either fix this issue, or make it clear what it is really changing.


- Andreas


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


On July 8, 2016, 10:14 p.m., Tony Gutierrez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3535/
> ---
> 
> (Updated July 8, 2016, 10:14 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11556:9e4c3b188fc5
> ---
> gpu-compute: Adding context serialization methods to Wavefront
> 
> This patch adds methods to serialize the context of a particular wavefront
> to the simulated system memory. Context serialization is used when a wavefront
> is preempeted (i.e. context switch).
> 
> 
> Diffs
> -
> 
>   src/gpu-compute/wavefront.hh 91f58918a76abf1a1dedcaa70a9b95789da7b88c 
>   src/gpu-compute/wavefront.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c 
> 
> Diff: http://reviews.gem5.org/r/3535/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tony Gutierrez
> 
>

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

Re: [gem5-dev] Review Request 3535: gpu-compute: Adding context serialization methods to Wavefront

[Andreas Hansson]

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

Ship it!


Ship It!

- Andreas Hansson


On July 8, 2016, 10:14 p.m., Tony Gutierrez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3535/
> ---
> 
> (Updated July 8, 2016, 10:14 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11556:9e4c3b188fc5
> ---
> gpu-compute: Adding context serialization methods to Wavefront
> 
> This patch adds methods to serialize the context of a particular wavefront
> to the simulated system memory. Context serialization is used when a wavefront
> is preempeted (i.e. context switch).
> 
> 
> Diffs
> -
> 
>   src/gpu-compute/wavefront.hh 91f58918a76abf1a1dedcaa70a9b95789da7b88c 
>   src/gpu-compute/wavefront.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c 
> 
> Diff: http://reviews.gem5.org/r/3535/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tony Gutierrez
> 
>

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

Re: [gem5-dev] Review Request 3535: gpu-compute: Adding context serialization methods to Wavefront

[Alexandru Dutu]

> On June 30, 2016, 7:17 a.m., Andreas Hansson wrote:
> > src/gpu-compute/wavefront.hh, line 389
> > 
> >
> > Surely one of these should be const
> 
> Andreas Hansson wrote:
> I do not see this being addressed. getContext should be const I would 
> imagine.

I see, you meant the functions to be const. Well the functions can't actually 
be const as both of them are calling non-const functions (i.e. remap and 
emplace_back).


> On June 30, 2016, 7:17 a.m., Andreas Hansson wrote:
> > src/gpu-compute/wavefront.cc, line 946
> > 
> >
> > Not too many bonus points for comments here...that's for sure.
> 
> Alexandru Dutu wrote:
> I have added a comment in the header file which should describe the 
> behaviour of this method and added comments inside it where things are not 
> straight forward (i.e. recovergence stack and LDS). Please let me know what 
> is not clear and requires a comment, would be happy to add some more.
> 
> Andreas Hansson wrote:
> I find the code rather unfortunate. It would have been great to see this 
> implemented using e.g. protobuf or similar, but it's up to you if you really 
> think this is maintainable (and understandable).

Actually, I had 2 implementations for this: 1 using iostreams and the good old 
void * implementation you see currently. I've chosen this implementation for 
simplicity as there isn't much difference (in maintainability of readability) 
between "*(int *)iter = wfId; iter += sizeof(wfId);" and 
"ProtoMessage::PacketHeader out; out.set_wf_id(w->wfId); ... 
out->write(wavefront_context);". Meaning one still has to set all the 
attributes of the object to be serialized. I will be using protobufs more and I 
might change this code in the future if there is an advantage. Also, the 
reconvergence stack makes the use of protobufers less convenient as you still 
have to iterate over it and have a function call to set each entry.


- Alexandru


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


On July 8, 2016, 10:14 p.m., Tony Gutierrez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3535/
> ---
> 
> (Updated July 8, 2016, 10:14 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11556:9e4c3b188fc5
> ---
> gpu-compute: Adding context serialization methods to Wavefront
> 
> This patch adds methods to serialize the context of a particular wavefront
> to the simulated system memory. Context serialization is used when a wavefront
> is preempeted (i.e. context switch).
> 
> 
> Diffs
> -
> 
>   src/gpu-compute/wavefront.hh 91f58918a76abf1a1dedcaa70a9b95789da7b88c 
>   src/gpu-compute/wavefront.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c 
> 
> Diff: http://reviews.gem5.org/r/3535/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tony Gutierrez
> 
>

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

Re: [gem5-dev] Review Request 3535: gpu-compute: Adding context serialization methods to Wavefront

[Andreas Hansson]

> On June 30, 2016, 7:17 a.m., Andreas Hansson wrote:
> > src/gpu-compute/wavefront.hh, line 389
> > 
> >
> > Surely one of these should be const

I do not see this being addressed. getContext should be const I would imagine.


> On June 30, 2016, 7:17 a.m., Andreas Hansson wrote:
> > src/gpu-compute/wavefront.cc, line 946
> > 
> >
> > Not too many bonus points for comments here...that's for sure.
> 
> Alexandru Dutu wrote:
> I have added a comment in the header file which should describe the 
> behaviour of this method and added comments inside it where things are not 
> straight forward (i.e. recovergence stack and LDS). Please let me know what 
> is not clear and requires a comment, would be happy to add some more.

I find the code rather unfortunate. It would have been great to see this 
implemented using e.g. protobuf or similar, but it's up to you if you really 
think this is maintainable (and understandable).


- Andreas


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


On July 8, 2016, 10:14 p.m., Tony Gutierrez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/3535/
> ---
> 
> (Updated July 8, 2016, 10:14 p.m.)
> 
> 
> Review request for Default.
> 
> 
> Repository: gem5
> 
> 
> Description
> ---
> 
> Changeset 11556:9e4c3b188fc5
> ---
> gpu-compute: Adding context serialization methods to Wavefront
> 
> This patch adds methods to serialize the context of a particular wavefront
> to the simulated system memory. Context serialization is used when a wavefront
> is preempeted (i.e. context switch).
> 
> 
> Diffs
> -
> 
>   src/gpu-compute/wavefront.hh 91f58918a76abf1a1dedcaa70a9b95789da7b88c 
>   src/gpu-compute/wavefront.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c 
> 
> Diff: http://reviews.gem5.org/r/3535/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tony Gutierrez
> 
>

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

Re: [gem5-dev] Review Request 3535: gpu-compute: Adding context serialization methods to Wavefront

[Tony Gutierrez]

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

(Updated July 8, 2016, 3:14 p.m.)


Review request for Default.


Summary (updated)
-

gpu-compute: Adding context serialization methods to Wavefront


Repository: gem5


Description (updated)
---

Changeset 11556:9e4c3b188fc5
---
gpu-compute: Adding context serialization methods to Wavefront

This patch adds methods to serialize the context of a particular wavefront
to the simulated system memory. Context serialization is used when a wavefront
is preempeted (i.e. context switch).


Diffs (updated)
-

  src/gpu-compute/wavefront.hh 91f58918a76abf1a1dedcaa70a9b95789da7b88c 
  src/gpu-compute/wavefront.cc 91f58918a76abf1a1dedcaa70a9b95789da7b88c 

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


Testing
---


Thanks,

Tony Gutierrez

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