Skip to content
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

Merged
merged 2 commits into from
Oct 30, 2024

Conversation

ashermancinelli
Copy link
Contributor

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!

Fix some small nitpicks of the grammar, capitalization and formatting of
these rationale documents.
@llvmbot
Copy link
Member

llvmbot commented Oct 30, 2024

@llvm/pr-subscribers-mlir

Author: Asher Mancinelli (ashermancinelli)

Changes

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!


Full diff: https://github.com/llvm/llvm-project/pull/114157.diff

1 Files Affected:

  • (modified) mlir/docs/Rationale/SideEffectsAndSpeculation.md (+9-9)
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]`.

Copy link
Contributor

@cxy-1993 cxy-1993 left a 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!

@ashermancinelli ashermancinelli merged commit 6af275b into llvm:main Oct 30, 2024
11 checks passed
NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
A couple of these are probably up to preference, but the
grammar/capitalization changes are probably more critical for
readability.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants