[jum-multisys] PlatformDescription refactoring

[Giuseppe Massari]

Dear all (especially Federico Reghenzani and Michele Zanella),

I would perform a refactoring of the PlatformDescription class as follows:

All the sublcasses ProcessingElement, CPU, MulticoreProcessor, Systems... will derive from a new subclass Resource, defining the member functions SetId() and GetId().

The result is two-fold: 1) avoid code replication; 2) immediate retrieving of the local system id.

Let me know what do you think. I can introduce this change by myself, if you agree.

Cheers

[Federico Reghenzani]

I agree.

I think it would be also useful a method to retrive also the full path, e.g. given a ProcessingElement instance, get the "sys0.cpu1.pe3" string.

Maybe it can be directly inserted in PlatformDescription class, something like:

    std::string GetString(const Resource &) const;

Cheers,
Federico Reghenzani

--
You received this message because you are subscribed to the Google Groups "BOSP Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to bosp-devel+...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Michele Zanella]

Ok, it would be a good idea to have the upper class Resource.

For the path string I think we have 2 possibilities: directly adding a method in the PlatformDescription class that works at element level giving a full path (for a pe: "sys0.cpu1.pe1"), as Reghe said, or giving only the string of the single element name+id ("pe1").

I think that the first solution is better because directly the PlatformDescription object will return its own information without the need of cycling each element in an external method. 

In any case will I introduce the new AddResources in the ResourceAccounter as agreed today?

Cheers, M

--

Michele Zanella

[Giuseppe Massari]

On Fri, 29 Jul 2016 at 19:42 Michele Zanella <zanella...@gmail.com> wrote:

Ok, it would be a good idea to have the upper class Resource.

For the path string I think we have 2 possibilities: directly adding a method in the PlatformDescription class that works at element level giving a full path (for a pe: "sys0.cpu1.pe1"), as Reghe said, or giving only the string of the single element name+id ("pe1").

I think that the first solution is better because directly the PlatformDescription object will return its own information without the need of cycling each element in an external method. 

I can work on it today.

 

In any case will I introduce the new AddResources in the ResourceAccounter as agreed today?

The member function will be "RegisterResources(const PlatformDescription  & plat_descr)". 

As soon as the patches above are ready, Michele can implement the RegisterResources.

[Giuseppe Massari]

I need your opinion about this. To get the system id number we can decide between two approaches:

A) To include an attribute "id=..." in the systems.xml file.
In this case we need to introduce changes also in the generator_xml.in script to include such attribute in each <include> occurrence.

B) To assign a progressive number directly in the PlatformLoader plugin, by increasing the id for each ParseSystemDocument(..) call.

I prefer option B, which is the less invasive change but also the more robust and effective option (think about the fact that the Systems objects are stored into a vector...).

Tell me what you think before I proceed.

 

[Federico Reghenzani]

I would also prefer option B, since if one disables a system, the SysX number still remain progressive.

[Michele Zanella]

I think that option B is better, yes.

[Giuseppe Massari]

Hello folks,

I pushed an updated version of the branch. 

Please fetch the branch and rebase yours or simply cherry-pick the latest commits, as you prefer.

I introduced a deep refactoring of the PlatformLoader and added the discussed feature. I leave further details to the description messages included in the commits.

Cheers

[Michele Zanella]

Hi!

I noticed that in the PlatformDescription there are 2 methods that have the names different although one is the "const" version of the other:

inline const std::vector<MemoryPtr_t> & GetMemories() 

inline std::vector<MemoryPtr_t> & GetMemoryAll().

To keep the naming convention (cache: P) coherence I think it is better to change both names in GetMemoriesAll(). Can I perform the change?

Cheers

[Michele Zanella]

Another question...

In the RegisterAccounter::RegisterResource(...) there is the "units" parameter, from the single Resource of the PlatformDescription I cannot retrieve this information directly...

Is it better to overload the RegisterResource method without this parameter or what else?

PS: Giuseppe enjoy your meal (considering that you cannot send me something to taste :( )

--

Michele Zanella

[Federico Reghenzani]

2016-08-02 19:54 GMT+02:00 Michele Zanella <zanella...@gmail.com>:

Another question...

In the RegisterAccounter::RegisterResource(...) there is the "units" parameter, from the single Resource of the PlatformDescription I cannot retrieve this information directly...

Is it better to overload the RegisterResource method without this parameter or what else?

The "units" indicated in the XML is already converted in the parser, so every data in PlatformDescription is in the "standard" unit. I checked the current RegisterResource and I found that it calls ConvertValue(...) that performs the same conversion of the parser. Thus, I think that unit should not be considered in RegisterResources(PlatformDescription) because PlatformDescription contains already the converted values. Moreover, maintaining units in PlatformDescription members would be useless.

We can also simply PlatformDescription removing the distinction for int64_t and double int32_t, because I noticed that (u)int64_t is extensively used in BBQ, so compile BBQ in an architecture that does not support native 64-bit integer seems not feasible.

 

PS: Giuseppe enjoy your meal (considering that you cannot send me something to taste :( )

On Tue, Aug 2, 2016 at 7:40 PM, Michele Zanella <zanella...@gmail.com> wrote:

Hi!

I noticed that in the PlatformDescription there are 2 methods that have the names different although one is the "const" version of the other:

inline const std::vector<MemoryPtr_t> & GetMemories() 

inline std::vector<MemoryPtr_t> & GetMemoryAll().

To keep the naming convention (cache: P) coherence I think it is better to change both names in GetMemoriesAll(). Can I perform the change?

I agree. I also noticed that Memory objects are stored with shared_ptr inside the vector, instead other resources not.