Skip to content

[NFC][mlir][gpu] Make sym_name an inherent attr in GPUModuleOp #94918

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 2 commits into from
Jun 9, 2024

Conversation

fabianmcg
Copy link
Contributor

@fabianmcg fabianmcg commented Jun 9, 2024

Make sym_name an inherent attr in GPUModuleOp so that it doesn't show in the discardable attributes.
The change is safe as the attribute is always expected to be present.

@fabianmcg fabianmcg changed the title [NFC][mlir][gpu] Make syn_name an intrinsic attr in GPUModuleOp [NFC][mlir][gpu] Make sym_name an intrinsic attr in GPUModuleOp Jun 9, 2024
@fabianmcg fabianmcg marked this pull request as ready for review June 9, 2024 21:50
@fabianmcg fabianmcg requested a review from joker-eph June 9, 2024 21:50
@llvmbot
Copy link
Member

llvmbot commented Jun 9, 2024

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-gpu

Author: Fabian Mora (fabianmcg)

Changes

Make sym_name an intrinsic attr in GPUModuleOp so that it doesn't show in the discardable attributes.
The change is safe as the attribute is always expected to be present.


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

2 Files Affected:

  • (modified) mlir/include/mlir/Dialect/GPU/IR/GPUOps.td (+1-1)
  • (modified) mlir/lib/Dialect/GPU/IR/GPUDialect.cpp (+1-2)
diff --git a/mlir/include/mlir/Dialect/GPU/IR/GPUOps.td b/mlir/include/mlir/Dialect/GPU/IR/GPUOps.td
index 10719aae5c8b4..eb81b6469746f 100644
--- a/mlir/include/mlir/Dialect/GPU/IR/GPUOps.td
+++ b/mlir/include/mlir/Dialect/GPU/IR/GPUOps.td
@@ -1241,7 +1241,7 @@ def GPU_BarrierOp : GPU_Op<"barrier"> {
 def GPU_GPUModuleOp : GPU_Op<"module", [
       DataLayoutOpInterface, HasDefaultDLTIDataLayout, IsolatedFromAbove,
       SymbolTable, Symbol, SingleBlockImplicitTerminator<"ModuleEndOp">
-    ]>, Arguments<(ins
+    ]>, Arguments<(ins SymbolNameAttr:$sym_name,
           OptionalAttr<GPUNonEmptyTargetArrayAttr>:$targets,
           OptionalAttr<OffloadingTranslationAttr>:$offloadingHandler)> {
   let summary = "A top level compilation unit containing code to be run on a GPU.";
diff --git a/mlir/lib/Dialect/GPU/IR/GPUDialect.cpp b/mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
index 0c2590d711301..d40c586a313b0 100644
--- a/mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
+++ b/mlir/lib/Dialect/GPU/IR/GPUDialect.cpp
@@ -1730,12 +1730,11 @@ void GPUModuleOp::build(OpBuilder &builder, OperationState &result,
                         StringRef name, ArrayAttr targets,
                         Attribute offloadingHandler) {
   ensureTerminator(*result.addRegion(), builder, result.location);
-  result.attributes.push_back(builder.getNamedAttr(
-      ::mlir::SymbolTable::getSymbolAttrName(), builder.getStringAttr(name)));
 
   Properties &props = result.getOrAddProperties<Properties>();
   if (targets)
     props.targets = targets;
+  props.setSymName(builder.getStringAttr(name));
   props.offloadingHandler = offloadingHandler;
 }
 

@joker-eph joker-eph changed the title [NFC][mlir][gpu] Make sym_name an intrinsic attr in GPUModuleOp [NFC][mlir][gpu] Make sym_name an inherent attr in GPUModuleOp Jun 9, 2024
@joker-eph
Copy link
Collaborator

The change is safe as the attribute is always expected to be present.

How was this expectation materialized until now?

@fabianmcg
Copy link
Contributor Author

How was this expectation materialized until now?

It was always being added by the builder as a discardable attribute.

@fabianmcg fabianmcg merged commit 54373e0 into llvm:main Jun 9, 2024
7 checks passed
@fabianmcg fabianmcg deleted the pr-gpu-module-sym-name branch June 9, 2024 22:36
@HerrCai0907 HerrCai0907 mentioned this pull request Jun 13, 2024
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.

3 participants