Skip to content

[mlir] Slightly optimize bytecode op numbering #88310

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

Merged
merged 1 commit into from
Apr 10, 2024
Merged

Conversation

Mogball
Copy link
Contributor

@Mogball Mogball commented Apr 10, 2024

If the bytecode encoding supports properties, then the dictionary attribute is always the raw dictionary attribute of the operation, regardless of what it contains. Otherwise, get the dictionary attribute from the op: if the op does not have properties, then it returns the raw dictionary, otherwise it returns the combined inherent and discardable attributes.

@Mogball Mogball requested review from jpienaar and joker-eph April 10, 2024 19:01
@llvmbot llvmbot added mlir:core MLIR Core Infrastructure mlir labels Apr 10, 2024
@llvmbot
Copy link
Member

llvmbot commented Apr 10, 2024

@llvm/pr-subscribers-mlir-core

@llvm/pr-subscribers-mlir

Author: Jeff Niu (Mogball)

Changes

If the bytecode encoding supports properties, then the dictionary attribute is always the raw dictionary attribute of the operation, regardless of what it contains. Otherwise, get the dictionary attribute from the op: if the op does not have properties, then it returns the raw dictionary, otherwise it returns the combined inherent and discardable attributes.


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

1 Files Affected:

  • (modified) mlir/lib/Bytecode/Writer/IRNumbering.cpp (+6-5)
diff --git a/mlir/lib/Bytecode/Writer/IRNumbering.cpp b/mlir/lib/Bytecode/Writer/IRNumbering.cpp
index f36c9ef060b6d7..dd90e6853ac32b 100644
--- a/mlir/lib/Bytecode/Writer/IRNumbering.cpp
+++ b/mlir/lib/Bytecode/Writer/IRNumbering.cpp
@@ -429,17 +429,18 @@ void IRNumberingState::number(Operation &op) {
   // Prior to a version with native property encoding, or when properties are
   // not used, we need to number also the merged dictionary containing both the
   // inherent and discardable attribute.
-  if (config.getDesiredBytecodeVersion() <
-          bytecode::kNativePropertiesEncoding ||
-      !op.getPropertiesStorage()) {
+  DictionaryAttr dictAttr;
+  if (config.getDesiredBytecodeVersion() >= bytecode::kNativePropertiesEncoding)
+    dictAttr = op.getRawDictionaryAttrs();
+  else
     dictAttr = op.getAttrDictionary();
-  }
   if (!dictAttr.empty())
     number(dictAttr);
 
   // Visit the operation properties (if any) to make sure referenced attributes
   // are numbered.
-  if (config.getDesiredBytecodeVersion() >= bytecode::kNativePropertiesEncoding &&
+  if (config.getDesiredBytecodeVersion() >=
+          bytecode::kNativePropertiesEncoding &&
       op.getPropertiesStorageSize()) {
     if (op.isRegistered()) {
       // Operation that have properties *must* implement this interface.

If the bytecode encoding supports properties, then the dictionary
attribute is always the raw dictionary attribute of the operation,
regardless of what it contains. Otherwise, get the dictionary attribute
from the op: if the op does not have properties, then it returns the raw
dictionary, otherwise it returns the combined inherent and discardable
attributes.
@Mogball Mogball merged commit fb771fe into llvm:main Apr 10, 2024
joker-eph pushed a commit to joker-eph/llvm-project that referenced this pull request Apr 17, 2024
If the bytecode encoding supports properties, then the dictionary
attribute is always the raw dictionary attribute of the operation,
regardless of what it contains. Otherwise, get the dictionary attribute
from the op: if the op does not have properties, then it returns the raw
dictionary, otherwise it returns the combined inherent and discardable
attributes.
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