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

Update documentation for MutableOperandRange #65865

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

roytu
Copy link

@roytu roytu commented Sep 9, 2023

See #65829

Caching MutableOperandRange's within an op with AttrSizedOperandSegments, then accidentally invalidating them by editing an operand range, seems like a common issue. Added some documentation to perhaps help with this.

Note: I'm not sure if this is the best place to document something like this, or if we even need this at all -- please feel free to reject if you think it's too much of an edge case. Thanks :)

See llvm#65829

Caching `MutableOperandRange`'s within an op with `AttrSizedOperandSegments`, then accidentally invalidating them by editing an operand range, seems like a common issue. Added some documentation to perhaps help with this.
@roytu roytu requested a review from a team as a code owner September 9, 2023 22:15
@llvmbot llvmbot added mlir:core MLIR Core Infrastructure mlir labels Sep 9, 2023
@llvmbot
Copy link
Member

llvmbot commented Sep 9, 2023

@llvm/pr-subscribers-mlir

Changes

See #65829

Caching MutableOperandRange's within an op with AttrSizedOperandSegments, then accidentally invalidating them by editing an operand range, seems like a common issue. Added some documentation to perhaps help with this.

Note: I'm not sure if this is the best place to document something like this, or if we even need this at all -- please feel free to reject if you think it's too much of an edge case. Thanks :)

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

1 Files Affected:

  • (modified) mlir/include/mlir/IR/ValueRange.h (+4)
diff --git a/mlir/include/mlir/IR/ValueRange.h b/mlir/include/mlir/IR/ValueRange.h
index 187185b47b66695..e57ccc1924685fd 100644
--- a/mlir/include/mlir/IR/ValueRange.h
+++ b/mlir/include/mlir/IR/ValueRange.h
@@ -112,6 +112,10 @@ class OperandRangeRange final
 
 /// This class provides a mutable adaptor for a range of operands. It allows for
 /// setting, inserting, and erasing operands from the given range.
+///
+/// (Note that when using AttrSizedOperandSegments, making changes to a
+/// MutableOperandRange invalidates other MutableOperandRange's in memory; you
+/// must get a new mutable adaptor for the other operand ranges).
 class MutableOperandRange {
 public:
   /// A pair of a named attribute corresponding to an operand segment attribute,

///
/// (Note that when using AttrSizedOperandSegments, making changes to a
/// MutableOperandRange invalidates other MutableOperandRange's in memory; you
/// must get a new mutable adaptor for the other operand ranges).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is true even without using AttrSizedOperandSegments I believe.

Look at the members:

  /// The owning operation of this range.
  Operation *owner;
  /// The start index of the operand range within the owner operand list, and
  /// the length starting from `start`.
  unsigned start, length;

So any addition or removal of an operand for the owner Operation would invalidate the range.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mlir:core MLIR Core Infrastructure mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants