search

EPFL-LAP / dynamatic

DHLS (Dynamic High-Level Synthesis) compiler based on MLIR
Other
65 stars 19 forks source link

[SpeculationPass] Fixed SCCommitCtrl connection #170

Closed shundroid closed 3 weeks ago

shundroid commented 1 month ago

The connection of SCCommitCtrl from the speculator to save-commit units was slightly different from Haoran's thesis, which caused a deadlock. Thus I updated the implementation to match that of Haoran's thesis.

Also, I fixed the names of branch ops to make them more understandable ( #169 ). The rename of SCBranchCtrl to SCIsMisspec is not done here because it involves the change in the backend.

Now the connection would be like this: Screenshot from 2024-10-18 15-14-41

shundroid commented 1 month ago

Thanks I fixed both!

Jiahui17 commented 1 month ago

@shundroid Thanks for the fix!

This is the first time I am trying to understand (at least partially) what is happening in the speculation codebase.

  1. What is this condition at the input?
  2. Are you adding the branch that delivers the condition to save commits according to the control flow?
murphe67 commented 1 month ago

@Jiahui17 yes, its the condition that routed the speculative token to the save-commit, there's 3 branches required for routing according to Haoran's solution. Aleix's code places them in the wrong order, causing a mismatched number of inputs at the 2nd branch and ultimately deadlock.

Jiahui17 commented 1 month ago

@Jiahui17 yes, its the condition that routed the speculative token to the save-commit, there's 3 branches required for routing according to Haoran's solution. Aleix's code places them in the wrong order, causing a mismatched number of inputs at the 2nd branch and ultimately deadlock.

@murphe67 I understand now, thanks!

lucas-rami commented 1 month ago

@shundroid you should have rights to squash and merge this now that Jiahui approved the PR, feel free to do it. If you cannot merge for some reason let me know, there may be a problem in the repository's config.

shundroid commented 1 month ago

@lucas-rami Thank you for your suggestion. I'm ready to merge this PR but it seems I don't have the permission. image

lucas-rami commented 3 weeks ago

Sorry I just found time to go dig into the repo's settings. I changed something which I think should allow you to push since changes were approved, can you confirm that you can squash and merge? If so please feel free to do it as well.

shundroid commented 3 weeks ago

Thank you I could do that!