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

change isneighbour to compute async topo #2

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

yuxwang99
Copy link

Adapt the isneighbour function to compute the connection between blocks in a random MxN topology.
Fix the printassembly type

@yuxwang99
Copy link
Author

Hi Cristian, I am a PhD student from EPFL who would like to work based on your SAT-MapIt framework. I have raised the PR to make it runnable for our CGRA architecture without affecting the original functionality. Let me know if you don't hope to make this change please, I can withdraw them.
Another thing is there a typo in the instruction printassembly part? Hope this will help us.
Looking forward to discuss with you

@CristianTirelli
Copy link
Owner

Hi yuxwang,

thank you for the interest in this project.
Regarding your fixes:

  1. Adapt the isneighbour function to compute the connection between blocks in a random MxN topology.
    Can you give me more information about this. The original code behaves the same as the one you proposed. There is only one intrinsic change and is the PE position in the CGRA grid.
    Originally on a 2x2 CGRA you have this kind of assignment:
    | PE0 | PE1 |
    | PE2 | PE3 |
    with your change:
    | PE0 | PE2 |
    | PE1 | PE3 |
    Both are valid and CGRA independent for what I know, so I don't see any reason to prefer one to the other. Am I missing something?

  2. Fix the printassembly type
    I agree with you here, the type is not correct (apparently). In reality it was intended because in the early testing phase on the device the MV operation was not implemented and I just replaced it with the ADD, to mimic the same behavior.
    Let's confirm that the opcode is correctly handled with your group, before committing this change.

Looking forward to work with you, let me know if you have any question.

@yuxwang99
Copy link
Author

Hi Cristian,
I agree with you that let's wait for the opcode to work correctly to merge. Thanks for your information,
Regarding the first issue. I inferred the order of the PE index from the code which seems to be wrong. I original thought it is
| PE0 | PE1 |
| PE2 | PE3 |, the correct order is quite a large help for me to understand the behavior:)
I will change the behavior in the correct order. Thanks!

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.

2 participants