[Formal][Property Annotation] Annotating Exit Token Invariants#922
[Formal][Property Annotation] Annotating Exit Token Invariants#922Basmet0 wants to merge 32 commits into
Conversation
1. Within an IOG, find all deciding branches (between looping or never coming back) 2. Find the decider operation making the condition 3. Search forward for all branches coming out of the decider. These are all exit branches.
…tation rather than recomputing
| struct BranchOpDecision { | ||
| // true -> this branch loops towards itself | ||
| // false -> this branch exits | ||
| bool trueLoop; | ||
| bool falseLoop; | ||
|
|
||
| inline bool isExitBranch() { return trueLoop != falseLoop; } | ||
| }; | ||
|
|
||
| BranchOpDecision findBranchLoops(const IOG &iog, ConditionalBranchOp branch) { | ||
| BranchOpDecision ret; | ||
| Operation *opTrue = *branch.getTrueResult().getUsers().begin(); | ||
| auto paths = iog.findAllPaths(opTrue, branch); | ||
| ret.trueLoop = !(paths.units.find(branch) == paths.units.end()); | ||
|
|
||
| Operation *opFalse = *branch.getFalseResult().getUsers().begin(); | ||
| paths = iog.findAllPaths(opFalse, branch); | ||
| ret.falseLoop = !(paths.units.find(branch) == paths.units.end()); | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
Suggestions:
- Maybe just call it isIOGLoopExitBranch(..) (returns a Boolean value instead)? We can omit the new custom type in that case
- Could be declared as a class method of IOG
| Operation *getDecider(ConditionalBranchOp branch) { | ||
| std::vector<mlir::Value> stack; | ||
| stack.push_back(branch.getConditionOperand()); | ||
| Operation *dec = branch; | ||
| while (!stack.empty()) { | ||
| mlir::Value cur = stack.back(); | ||
| stack.pop_back(); | ||
| Operation *op = cur.getDefiningOp(); | ||
|
|
||
| if (!op) { | ||
| continue; | ||
| } | ||
| dec = op; | ||
|
|
||
| if (auto forkOp = dyn_cast<ForkOp>(op)) { | ||
| stack.push_back(forkOp.getOperand()); | ||
| } | ||
| if (auto buffer = dyn_cast<BufferOp>(op)) { | ||
| stack.push_back(buffer.getOperand()); | ||
| } | ||
| } | ||
| return dec; | ||
| } |
There was a problem hiding this comment.
Suggestions:
// a decider is ....- we could return a std::optional<Operation *> (or
llvm::FailureOr<Operation*>) and encode "we didn't find a decider" as nullopt - The parent function returns failure() if no decider was discovered
- use else if
For instance, if op is somehow a nullptr we should return a nullopt
comments for decider/unstartedPath
| iogBranch = slots.end; | ||
| } | ||
| } | ||
| assert(iogBranch); |
| Operation *dec = *getDecider(branch); | ||
| deciders.insert({dec, exitValue}); | ||
| } | ||
| for (auto [dec, exitValue] : deciders) { |
There was a problem hiding this comment.
The loop body here is quite long, i think it makes sense to put some markers to visually separate the code for different invariants:
// [START Enumerating ExitTokenOrder invariants]
for (const auto &slots : slotPaths) {
...
propertyTable.push_back(p.toJSON());
}
// [END Enumerating ExitTokenOrder invariants]
// [START Enumerating ExitTokenNoAncestors invariants]
// [END Enumerating ExitTokenNoAncestors invariants]| inline static const StringLiteral PATH_CM_LIT = "cmerge_mux_path"; | ||
| }; | ||
|
|
||
| // In a path between a decider unit and a branch, only the first token along the |
There was a problem hiding this comment.
BTW the descriptions you added there are informative, it is just that the thing we want to describe is generally quite convoluted and we want to visualize this part for the reader
| } | ||
|
|
||
| Operation *op = cur.channel.getDefiningOp(); | ||
| if (!op || !iog.contains(op) || op == exit || |
There was a problem hiding this comment.
Suggestions
- Order the cases as
generating more returns paths (each input of a multi-input unit),populating one return path (Buffer), andterminination condition - Explain the termination condition in more detail?
- Use if .. else if
There was a problem hiding this comment.
-
In this function, the operations are not handled case-by-case, but instead using their interfaces as any operation is allowed here. The order cannot be changed without breaking the behaviour:
First, terminate in bad cases (otherwise potentially too many things are annotated).
Second, if applicable, an eager fork output is added as a copied sent to the next slot.
Third, slots are annotated.
Lastly, all inputs are followed backwards (regardless of the number of inputs, can be 1 for buffer or fork, or 2 for arithmetic operations or loads)
I added some comments to explain this in code as well -
Yes, comment added
-
Similar to 1., if .. else if does not make sense here as I am not handling different cases but instead following the same four steps for any operation.
| std::vector<EffectiveSlotNamer> backPath; | ||
| std::vector<EagerForkSentNamer> backSents; |
There was a problem hiding this comment.
forward vs. backward: conceptually which one do you call forward?
| stack.pop_back(); | ||
|
|
||
| Operation *next = *path.cur.getUsers().begin(); | ||
| if (auto branch = dyn_cast<ConditionalBranchOp>(next)) { |
There was a problem hiding this comment.
Same suggestions as the other similar loop:
Suggestions
- Order the cases as
generating more returns paths (each input of a multi-input unit),populating one return path (Buffer), andterminination condition- Explain the termination condition in more detail?
- Use if .. else if
There was a problem hiding this comment.
Here that does make sense yes
| } | ||
| } | ||
|
|
||
| // Paths from decider to the branch might contain a fork before they contain a |
There was a problem hiding this comment.
remember to backreference the diagram I added in the other comment

An exit token - meaning a token that decides whether a loop exits or continues - can only occur once. There will be other decider tokens before an exit token, and, given that there is an exit token, no ancestors of the decider will contain a token.