[eagerly-elastic] Rewrites A and D with FTD - #1006
Conversation
|
Just a quick comment: I think in general we could rename |
murphe67
left a comment
murphe67
left a comment
There was a problem hiding this comment.
Hey Elena! Thank you very much for this very high-quality pull request. I didn't review quite all of it, because I think I have given you enough to think about that you can extrapolate from 😁
This code is presented beautifully on the communication level and was very enjoyable to read, so thank you very much for that 😁
I think for all of this rewriting we need to hammer out some philosophical details about exactly what the input IR looks like and how things get divided up, so we get a super stable logical foundation to build on- I think you probably have some super useful info for me based on your experience with this, so we can discuss nicely in our meeting later.
Thanks again!! 😁 👍
| bool currentInverted = false, siblingInverted = false; | ||
| Value currentRoot = getForkTop(currentCond, currentInverted); | ||
| Value siblingRoot = | ||
| getForkTop(siblingBranch.getConditionOperand(), siblingInverted); |
There was a problem hiding this comment.
handling two nots by passing through both of them and then returning "not inverted" seems a little bit messy- when does this happen?
it feels like this code might be course-correcting for the nots being handled weirdly rather than this being necessary in an elegant way? but I am not 100% sure.
| /// Move the suppressors past the following operation, targetOp. Erase all the | ||
| /// suppressors going into targetOp and create new suppressors on every output | ||
| /// of targetOp. | ||
| void EagerlyElasticADPass::performSuppressorMotion( |
There was a problem hiding this comment.
this taking a branch op seems strange to me on the logical level?
There was a problem hiding this comment.
i think it's cleaner to get the condition if we also have one of the suppressors that will be moved
| // create inverted condition | ||
| Value condition = branchOp.getConditionOperand(); | ||
| Location loc = branchOp.getLoc(); | ||
| auto invertedCondition = builder.create<handshake::NotIOp>(loc, condition); |
There was a problem hiding this comment.
doesn't this bring up a complaint about depreciation when you build? should be NotIOp::build or something i think?
There was a problem hiding this comment.
never had any issues with that, i use builder.create<> for all my operations...
3 participants
This pass implements the two rewrites A and D from the eagerly elastic paper. Rewrite A pushes suppressors past operations and Rewrite D enables eager execution of loops with the help of a new "repeatingInitOp".
Before applying this pass, it is first necessary to run some form of FTD.