Any comments on my comments :) ?
On Tue, Jun 25, 2013 at 9:53 AM, Amin Farmahini <[email protected]> wrote: > Now I understand your other question "Also, do you not think it is wise to > use the Param.MemorySize here and specify it in bytes of kb?" > I think memory size is a function rowbuffer size and number of rows per > DRAM array and not the other way around. > > Amin > > On Tue, Jun 25, 2013 at 9:36 AM, Amin Farmahini <[email protected]> wrote: > >> This is an automatically generated e-mail. To reply, visit: >> http://reviews.gem5.org/r/1927/ >> >> On June 25th, 2013, 10:12 a.m. UTC, *Andreas Hansson* wrote: >> >> Overall this looks like a great step in the right direction. One thing that >> struck me is the complexity of the iteration to split the packets. Why make >> it so complicated? Would we not simply change decode packet to take a >> container as an input parameter and push back as many packets as needed? >> This should also help remove any assumptions on alignment. What do you think? >> >> I read your comment multiple times to grasp it, but still not sure I fully >> understand it. >> >> "change decode packet to take a container as an input parameter and push >> back as many packets as needed". before pushing each split packet, I make >> sure this split packet is not in the write queue. If it is in the write >> queue, we don't push it back. If we push all split packets at once, there is >> no way to check the write queue. Please, note that a packet could be split >> into multiple split burst-size packets in which some burst-size packets >> could be in the write queue and some not. >> >> Overall, I am open to discuss it further and willing to make it more simple, >> if possible. >> >> >> On June 25th, 2013, 10:12 a.m. UTC, *Andreas Hansson* wrote: >> >> >> src/mem/SimpleDRAM.py<http://reviews.gem5.org/r/1927/diff/2/?file=36306#file36306line121> >> (Diff >> revision 2) >> >> def makeMultiChannel(cls, nbr_mem_ctrls, mem_start_addr, mem_size, >> >> 121 >> >> lines_per_rowbuffer = Param.Unsigned("Row buffer size in cache lines") >> >> 121 >> >> device_bus_width = Param.Unsigned("data bus width in bits for each DRAM >> "\ >> >> Do we need it per device? Why not combined? >> >> I believe this way is more clear. we use devicesPerRank to calculate the >> combined bus width. (devicesPerRank * deviceBusWidth) >> The devicesPerRank parameter helps us find out how many devices we have. As >> in our current configurations, some DRAMs have 8 devices, and some only 1. >> It is better to distinguish between them and make it more clear how many >> devices we have. >> >> >> On June 25th, 2013, 10:12 a.m. UTC, *Andreas Hansson* wrote: >> >> >> src/mem/SimpleDRAM.py<http://reviews.gem5.org/r/1927/diff/2/?file=36306#file36306line124> >> (Diff >> revision 2) >> >> def makeMultiChannel(cls, nbr_mem_ctrls, mem_start_addr, mem_size, >> >> 124 >> >> device_rowbuffer_size = Param.Unsigned("Page (row buffer) size per "\ >> >> Same as above, why per device and not per channel? >> >> Also, do you not think it is wise to use the Param.MemorySize here and >> specify it in bytes of kb? >> >> similarly, we use devicesPerRank to calculate the total row buffer size => >> (simple_dram.cc:108) rowBufferSize = devicesPerRank * deviceRowBufferSize; >> The reason is clarity again. >> >> For your other question, so do you mean we want to have a fixed memory size? >> >> >> On June 25th, 2013, 10:12 a.m. UTC, *Andreas Hansson* wrote: >> >> >> src/mem/simple_dram.hh<http://reviews.gem5.org/r/1927/diff/2/?file=36307#file36307line435> >> (Diff >> revision 2) >> >> class SimpleDRAM : public AbstractMemory >> >> 435 >> >> uint32_t burstSize; >> >> I'd suggest to make all these const >> >> Please note that burst_length and burst_size are different. burst_length is >> defined const, but burst_size is not const because burst_size is initialized >> in the body just like rowsPerBank. >> >> >> - Amin >> >> On June 24th, 2013, 6:01 a.m. UTC, Amin Farmahini wrote: >> Review request for Default. >> By Amin Farmahini. >> >> *Updated June 24, 2013, 6:01 a.m.* >> *Repository: * gem5 >> Description >> >> This patch gets rid of bytesPerCacheLine parameter and makes the DRAM >> configuration separate from cache line size. >> Instead of bytesPerCacheLine, I define a parameter for DRAM called >> burst_length. The burst_length parameter shows the length of a DRAM device >> burst in bits. >> Also, I replace lines_per_rowbuffer with device_rowbuffer_size to improve >> code portablity. >> >> Updates: >> - a burst length in beats for each memory type. >> - an interface width for each memory type. >> - the memory controller model is extended to reason about "system" packets >> vs "dram" packets and assemble the responses properly. It means that system >> packets larger than a full burst are split into multiple dram packets. >> >> Testing >> >> None >> >> Diffs >> >> - src/mem/simple_dram.cc (UNKNOWN) >> - src/mem/simple_dram.hh (UNKNOWN) >> - src/mem/SimpleDRAM.py (UNKNOWN) >> >> View Diff <http://reviews.gem5.org/r/1927/diff/> >> > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
