[prediction] pipeline duplication#956
Conversation
murphe67
left a comment
murphe67
left a comment
There was a problem hiding this comment.
Hey Elena!
Thanks for the really nice work on this PR, it is in super great shape 😁
I wrote a lot of comments, but they are kind of all in the same vein of:
I think you know what this does, and I think it works the way you think it works, but I think you could spend more time reading and writing it so that the file exists as an "act of communication"- the code is not only about doing what it is supposed to do, but also about taking a reader on a journey of what decisions you made and why.
Secondly, I think this PR currently assumes no branches in the duplicated region? and I think it would be nice to support that before merging 😁 Also duplicated regions which have additional input beyond the one we will eventually do data prediction on, so theyre not replaced with constants, just branched to their consumers in the different bbs based on which bb executes from the comparisons?
Let me know about any questions or concerns you have, I will maybe write more later but I think this is plently for you to get started with in terms of building a narrative in the code files 😁
murphe67
left a comment
murphe67
left a comment
There was a problem hiding this comment.
Hey Elena!
Thanks for the really nice work on this PR, it is in super great shape 😁
I wrote a lot of comments, but they are kind of all in the same vein of:
I think you know what this does, and I think it works the way you think it works, but I think you could spend more time reading and writing it so that the file exists as an "act of communication"- the code is not only about doing what it is supposed to do, but also about taking a reader on a journey of what decisions you made and why.
Secondly, I think this PR currently assumes no branches in the duplicated region? and I think it would be nice to support that before merging 😁 Also duplicated regions which have additional input beyond the one we will eventually do data prediction on, so theyre not replaced with constants, just branched to their consumers in the different bbs based on which bb executes from the comparisons?
Let me know about any questions or concerns you have, I will maybe write more later but I think this is plently for you to get started with in terms of building a narrative in the code files 😁
murphe67
left a comment
murphe67
left a comment
There was a problem hiding this comment.
Nice, the integration tests and documentation file are very nice now 😁 We discussed in the meeting but for the general thread to follow is this idea of "will my provided evidence convince someone skeptical I have thought about this thoroughly and that my decisions make sense" 😁
|
|
||
| ### Helper Functions | ||
|
|
||
| `readPredictMarker` |
There was a problem hiding this comment.
is this bit needed with the func signature below it?
There was a problem hiding this comment.
would you remove it completely or just remove the function signature?
| : terminator->getSuccessor(1); | ||
|
|
||
| // sweep the operations of the side-tracked block | ||
| for (mlir::Operation &op : otherSuccessor->getOperations()) { |
There was a problem hiding this comment.
when we are sweeping a block, we do not look at its branch operation. This means that if there are no values that go into the next block(s), we might not collect all of the operations that would be in these following blocks.
2 participants
The Pipeline Duplication Pass duplicates specific program paths with hardcoded constants. By using pragmas, you can specify a variable that frequently holds a specific value. The pass then generates an additional parallel path in the pipeline where that variable is treated as a constant.