Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
This is really great Sanjoy. Thanks! I have a few comments.
mlir/docs/Rationale/SideEffectsAndSpeculation.md | ||
---|---|---|
19 | can we introduce an appropriate generalization of the notion of "heap" here? It seems overly specific to talk about "heap"/"stack" in a document about MLIR broadly. | |
22 | Is there an appropriate generalization of this that includes graph regions as well? cc @mehdi_amini |
mlir/docs/Rationale/SideEffectsAndSpeculation.md | ||
---|---|---|
20 | How about something like this:
|
Can you format the markdown to wrap on 80 lines? (ideally just run mdformat on it)
Also we should link to this doc from other places (like the SideEffect interface doc).
mlir/docs/Rationale/SideEffectsAndSpeculation.md | ||
---|---|---|
19 | We should likely add something like "or any other global state for a program, usually matching some HW (modifying memory or other resources)". Keeping "heap" isn't a big deal in a "rationale" document I think (it's not a spec). | |
20 | I like the suggestion here. In particular "can assume it does not happen" is important to mention.
That can be misunderstood because the condition is really dynamic in general, the "can assume it does not happen" should be enough though? | |
21 | More broadly speaking: any operation that may not terminate (infinite loop is a specific construct that won't terminate). | |
22 | I am not sure: without control-flow (a graph region...) it gets weird to have non-local control flow! |
Also we should link to this doc from other places (like the SideEffect interface doc).
I don't see a doc, do you mean the .td file?
mlir/docs/Rationale/SideEffectsAndSpeculation.md | ||
---|---|---|
19 | Generalized the language here a bit, PTAL. | |
20 | Agreed -- folks might read "unreachable" to mean "statically unreachable" which isn't correct. | |
22 | I don't think any of this makes sense generically in a graph region. For instance, an graph region could be modeling a "sea of nodes"-style IR where all memory effects are explicitly sequenced via an explicit MemorySSA representation. Or it could be represent something that's not even an IR in the traditional sense. So for now I've added a disclaimer that this doc only applies to CFG regions. LMK if I should remove this. |
mlir/docs/Rationale/SideEffectsAndSpeculation.md | ||
---|---|---|
35 | Micro nit: "E.g.," | |
37 | Ha good example for %s case :) | |
43 | Micro nit: Only one space after period. | |
45 | And here there is no differentiator between where the op produces value/has side effects or not? E.g., in the scf.while case, given it has recursive side effect, it can still be removed in some cases today even I believe. [But Mehdi has good point above, this is a rationale doc not a spec] | |
84 | So, for clarification, if your op expects rank-3 inputs but gets a rank-4 input then you have an invalid module/function. The op has undefined behavior there, where your program as a whole is wrong. (effectively all ops which allow dynamic shapes need this and mostly only speculated in static shape case). |
mlir/docs/Rationale/SideEffectsAndSpeculation.md | ||
---|---|---|
33 | I think so -- reading from immutable resources seems pure. Under the assumption that freeing memory counts as mutating a resource. LMK if I missed something. | |
43 | Looks like mdformat fixed all such instances. | |
45 | Not sure I totally understood your concern, but I added a note that operations may exhibit multiple kinds of side effects. Let me know if I misunderstood. | |
84 |
I'd rather frame this as the entire _execution_ is invalid. |
can we introduce an appropriate generalization of the notion of "heap" here? It seems overly specific to talk about "heap"/"stack" in a document about MLIR broadly.