Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

William #267

Draft
wants to merge 21 commits into
base: develop
Choose a base branch
from
Draft

William #267

wants to merge 21 commits into from

Conversation

Will-Shaddix
Copy link

For Jason to review, MessageQueue changes are in message_queue.cc, .hh, Msg_Map_process_test.py is my test config, mapped_queue.cpp is what was used to create the binary I am running in the config

kaustav-goswami and others added 8 commits May 5, 2023 12:23
This change adds hardware support to the stdlib's RISCV board
to simulate NUMA memory ranges.

Change-Id: If3e371dcbd51b6eb3903d42809ae6579aef1b163
Signed-off-by: Kaustav Goswami <[email protected]>
Copy link
Member

@powerjg powerjg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks for putting this together. I have lots of small comments. Please take the time to learn gem5's style guide and take the time to write code that is up to our standards. Use stack overflow and github copilot!! to find the "right way" to write code.

For the actual problem that you're facing... the code is missing some pieces. I can try to find some time to sit down with you and work on this. Maybe tomorrow afternoon?

@@ -1806,3 +1806,101 @@ class LPDDR5_6400_1x16_8B_BL32(LPDDR5_6400_1x16_BG_BL32):
tCCD_L = "0ns"
tRRD_L = "0ns"
tWTR_L = "0ns"


class LLM(DRAMInterface):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you drop LLM changes from this PR?

type = 'MessageQueue'
cxx_header = "mem/message_queue.hh"
cxx_class = "gem5::MessageQueue"
# can either have a vector of ports connecting to the cores, or have a vector of write ports and 1 read port
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sure you install pre-commit and follow the gem5 style guide (in this case lines should be < 80 chars)


};

std::deque<std::tuple<uint32_t, uint32_t, uint32_t, Tick>> queue; //Could be address or vertexID, vertexID might be easier
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a very complex datastructure. Maybe it should be a map? A deque of a complex tuple doesn't seem like the right solution. In other words, this is a bad code smell :)

std::deque<std::tuple<uint32_t, uint32_t, uint32_t, Tick>> queue; //Could be address or vertexID, vertexID might be easier

uint32_t queueSize;
AddrRange my_range;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow the gem5 coding style. In this case, you need to use camelCase

Suggested change
AddrRange my_range;
AddrRange myRange;

uint32_t queueSize;
AddrRange my_range;
// std::vector<RespPort> corePorts;
RespPort corePorts;//= new RespPort();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can't be new since the RespPort is not a pointer.

Comment on lines +139 to +153
for chnl in range(num_chnls):
# size = addr_range.size()
interface = HBM_1000_4H_1x128()
interface.range = AddrRange(addr_range.start, size = addr_range.size(),
intlvHighBit = intlv_low_bit + intlv_bits - 1,
xorHighBit = 0,
intlvBits = intlv_bits,
intlvMatch = chnl)
ctrl = MemCtrl()
ctrl.dram = interface

#ctrl.dram.null = True
#ctrl.dram.addr_mapping = addr_map
#ctrl.dram.page_policy = page_policy
mem_ctrls.append(ctrl)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should simplify this for now. You should simply have one memory channel.

thispath = os.path.dirname(os.path.realpath(__file__))

binpath = os.path.join(thispath, '../../',
'tests/test-progs/hello/bin/x86/linux/mapped_queue_fixed_private')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't have to be in test-progs, and you should avoid checking in binaries.

process.cmd = [binpath]
# Set the cpu to use the process as its workload and create thread contexts
system.cpu.workload = process
# system.cpu.workload.map(vaddr=0x1000000, paddr=0x3CF9BDC0, size=4096)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This paddr (0x3CF9BDC0) is what you need to set your addr_range for the message queue.


# process.map(vaddr=0x10000, paddr=0x3CF9BDC0, size=4096, cacheable=False)

process.map(vaddr=0x1000000, paddr=0xFF00000, size=4096, cacheable=False)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe the above comment should be here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I set the Addr Range of Message Queue to be 255MB - 256MB which I believe 255 MB is 0xFF00000, unless I calculated that wrong. I know the size doesn't really match up to 1 MB but I figured the pointer should be pointing to 255 MB.


print("Beginning simulation!")
exit_event = m5.simulate()
print('Exiting @ tick %i because %s' % (m5.curTick(), exit_event.getCause()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kaustav-goswami pushed a commit that referenced this pull request Oct 31, 2023
Though in "ext" this directory is regularly modified. `pre-commit`
should run on these files.

This PR includes running `pre-commit run --files ext/testlib` to
reformat the files in "ext/testlib" using Python Black.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants