-
Notifications
You must be signed in to change notification settings - Fork 12.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[mlir][doc] Fix nitpicks in documentation #114157
[mlir][doc] Fix nitpicks in documentation #114157
Conversation
Fix some small nitpicks of the grammar, capitalization and formatting of these rationale documents.
@llvm/pr-subscribers-mlir Author: Asher Mancinelli (ashermancinelli) ChangesA couple of these are probably up to preference, but the grammar/capitalization changes are probably more critical for readability. Please let me know if you see any other touchups that should go in the same commit or if you'd like any of these removed. Cheers! Full diff: https://github.com/llvm/llvm-project/pull/114157.diff 1 Files Affected:
diff --git a/mlir/docs/Rationale/SideEffectsAndSpeculation.md b/mlir/docs/Rationale/SideEffectsAndSpeculation.md
index 8b08b757531bef..4d9021a356dfea 100644
--- a/mlir/docs/Rationale/SideEffectsAndSpeculation.md
+++ b/mlir/docs/Rationale/SideEffectsAndSpeculation.md
@@ -79,9 +79,9 @@ When adding a new op, ask:
1. Does it read from or write to the heap or stack? It should probably implement
`MemoryEffectsOpInterface`.
-1. Does these side effects ordered? It should probably set the stage of
- side effects to make analysis more accurate.
-1. Does These side effects act on every single value of resource? It probably
+1. Are these side effects ordered? The op should probably set the stage of
+ side effects to make analyses more accurate.
+1. Do these side effects act on every single value of a resource? It probably
should set the FullEffect on effect.
1. Does it have side effects that must be preserved, like a volatile store or a
syscall? It should probably implement `MemoryEffectsOpInterface` and model
@@ -106,9 +106,9 @@ add side effect correctly.
### SIMD compute operation
-If we have a SIMD backend dialect with a "simd.abs" operation, which reads all
+Consider a SIMD backend dialect with a "simd.abs" operation which reads all
values from the source memref, calculates their absolute values, and writes them
-to the target memref.
+to the target memref:
```mlir
func.func @abs(%source : memref<10xf32>, %target : memref<10xf32>) {
@@ -139,10 +139,10 @@ A typical approach is as follows:
}
```
-In the above example, we attach the side effect [MemReadAt<0, FullEffect>] to
+In the above example, we attach the side effect `[MemReadAt<0, FullEffect>]` to
the source, indicating that the abs operation reads each individual value from
the source during stage 0. Likewise, we attach the side effect
-[MemWriteAt<1, FullEffect>] to the target, indicating that the abs operation
+`[MemWriteAt<1, FullEffect>]` to the target, indicating that the abs operation
writes to each individual value within the target during stage 1 (after reading
from the source).
@@ -174,7 +174,7 @@ A typical approach is as follows:
}
```
-In the above example, we attach the side effect [MemReadAt<0, PartialEffect>] to
+In the above example, we attach the side effect `[MemReadAt<0, PartialEffect>]` to
the source, indicating that the load operation reads parts of values from the
memref during stage 0. Since side effects typically occur at stage 0 and are
-partial by default, we can abbreviate it as "[MemRead]".
+partial by default, we can abbreviate it as `[MemRead]`.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes!
A couple of these are probably up to preference, but the grammar/capitalization changes are probably more critical for readability.
A couple of these are probably up to preference, but the grammar/capitalization changes are probably more critical for readability. Please let me know if you see any other touchups that should go in the same commit or if you'd like any of these removed. Cheers!