feat(dataflow): sparse forward dataflow analysis api - #854
Conversation
7d86346 to
84799d7
Compare
axelcool1234
changed the title
f50cb06 to
e6412e3
Compare
9529f81 to
f072c4f
Compare
38d6512 to
b8429f1
Compare
b8429f1 to
fe25c77
Compare
559572d to
7c5e2b0
Compare
| private def isBranchOp | ||
| (op : OperationPtr) | ||
| (irCtx : IRContext OpCode) : Bool := | ||
| -- TODO: Replace this `.test .test` check once VeIR has proper branch ops. |
There was a problem hiding this comment.
there are plenty of branch ops in Veir right now!
but you do not want to be making these decisions here, take a look at Veir/GlobalOpInfo.lean which has some helpers for answering questions like this, that might be the place to put this information
There was a problem hiding this comment.
branch ops include cf.br, cf.cond_br, llvm.br, llvm cond_br, and also some RISC-V branches
There was a problem hiding this comment.
Here, I implemented an interface: #1237
| panic! s!"SparseForwardDataFlowAnalysis.getSuccessorOperand?: successor index {successorIndex} out of range" | ||
| else | ||
| match (op.get! irCtx).opType with | ||
| -- TODO: Replace this `.test .test` check once VeIR has proper branch ops. |
There was a problem hiding this comment.
also here-- I'm not exactly sure what you're doing, but don't hard-code information about dialects here if you can help it
| -- TODO: Replace this `.test .test` check once VeIR has proper branch ops. | ||
| -- `successorIndex` will become relevant then. | ||
| | .test .test => | ||
| if argumentIndex < op.getNumOperands! irCtx then |
There was a problem hiding this comment.
do you need this dynamic check? if I understand correctly, the called is making sure this is in-bounds, you should be able to attach a little proof of that
|
@math-fehr should this be using WfIRContext instead of IRContext? |
|
if I understand correctly, an unrecognized branch causes the analysis to start putting tops into successor blocks. but maybe it would be better to error out with a message that the analysis is unable to continue? I feel like we don't want to paper over something that's a serious problem |
|
I also had claude review this PR and it came up with a list of stuff to look at-- I don't want to paste it here, please do this on your own |
That would be better indeed! I'm not sure it's strictly necessary, but it might definitely bite us later if we don't do this. Otherwise, I'll just wait that axel fixes your changes and take a look at claude comments, so then I can review it when it's more ready! |
|
@math-fehr @regehr Sounds good guys, thanks for the review. I might only be able to get back to this next week. I'm wrapping up my internship (I have two presentations to prepare for this weekend and do this coming week). I really appreciate your patience, I know I have taken forever on this stuff. I look forward to finally getting this into VeIR :) |
7c5e2b0 to
89c9bff
Compare
89c9bff to
39da0d9
Compare
3 participants
Any transfer function that works on
OperationPtrwill use this API. Simply implementing a (transfer) function of the formOperationPtr -> DataFlowContext -> IRContext OpCode -> DataFlowContextgets you a sparse forward analysis plugged into the dataflow framework for free. This API handles most of the stuff the implementer of a transfer function shouldn't concern themselves with.