Skip to content

[mlir][openacc][NFC] Remove useless OptionalAttr with UnitAttr #68337

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
Oct 5, 2023

Conversation

clementval
Copy link
Contributor

UnitAttr exits or not so adding OptionalAttr around it is not necessary. This patch cleanup this for the RoutineOp

@llvmbot llvmbot added mlir flang Flang issues not falling into any other category mlir:openacc flang:fir-hlfir openacc labels Oct 5, 2023
@llvmbot
Copy link
Member

llvmbot commented Oct 5, 2023

@llvm/pr-subscribers-mlir
@llvm/pr-subscribers-mlir-openacc
@llvm/pr-subscribers-flang-fir-hlfir

@llvm/pr-subscribers-openacc

Changes

UnitAttr exits or not so adding OptionalAttr around it is not necessary. This patch cleanup this for the RoutineOp


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

2 Files Affected:

  • (modified) flang/lib/Lower/OpenACC.cpp (+2-3)
  • (modified) mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td (+6-6)
diff --git a/flang/lib/Lower/OpenACC.cpp b/flang/lib/Lower/OpenACC.cpp
index 84ce4cde94200f9..9670cc01b593b7e 100644
--- a/flang/lib/Lower/OpenACC.cpp
+++ b/flang/lib/Lower/OpenACC.cpp
@@ -2981,9 +2981,8 @@ genACC(Fortran::lower::AbstractConverter &converter,
   std::stringstream routineOpName;
   routineOpName << accRoutinePrefix.str() << routineCounter++;
   auto routineOp = modBuilder.create<mlir::acc::RoutineOp>(
-      loc, routineOpName.str(), funcName, mlir::StringAttr{}, mlir::UnitAttr{},
-      mlir::UnitAttr{}, mlir::UnitAttr{}, mlir::UnitAttr{}, mlir::UnitAttr{},
-      mlir::UnitAttr{}, mlir::IntegerAttr{});
+      loc, routineOpName.str(), funcName, mlir::StringAttr{}, false, false,
+      false, false, false, false, mlir::IntegerAttr{});
 
   for (const Fortran::parser::AccClause &clause : clauses.v) {
     if (std::get_if<Fortran::parser::AccClause::Seq>(&clause.u)) {
diff --git a/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td b/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
index ce1b4e29cd51b9b..60156cc334c72ec 100644
--- a/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
+++ b/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
@@ -1563,12 +1563,12 @@ def OpenACC_RoutineOp : OpenACC_Op<"routine", [IsolatedFromAbove]> {
   let arguments = (ins SymbolNameAttr:$sym_name,
                        SymbolNameAttr:$func_name,
                        OptionalAttr<StrAttr>:$bind_name,
-                       OptionalAttr<UnitAttr>:$gang,
-                       OptionalAttr<UnitAttr>:$worker,
-                       OptionalAttr<UnitAttr>:$vector,
-                       OptionalAttr<UnitAttr>:$seq,
-                       OptionalAttr<UnitAttr>:$nohost,
-                       OptionalAttr<UnitAttr>:$implicit,
+                       UnitAttr:$gang,
+                       UnitAttr:$worker,
+                       UnitAttr:$vector,
+                       UnitAttr:$seq,
+                       UnitAttr:$nohost,
+                       UnitAttr:$implicit,
                        OptionalAttr<APIntAttr>:$gangDim);
 
   let extraClassDeclaration = [{

Copy link
Contributor

@vzakhari vzakhari left a comment

Choose a reason for hiding this comment

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

LGTM

@clementval clementval merged commit 4f98fb2 into llvm:main Oct 5, 2023
@clementval clementval deleted the acc_routine_op branch October 5, 2023 17:45
Comment on lines +2984 to +2985
loc, routineOpName.str(), funcName, mlir::StringAttr{}, false, false,
false, false, false, false, mlir::IntegerAttr{});
Copy link
Collaborator

@joker-eph joker-eph Oct 5, 2023

Choose a reason for hiding this comment

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

Nit as I'm driving by.

We should "name" non-variable arguments, such as:

    loc, routineOpName.str(), funcName, /*bind_name=*/mlir::StringAttr{}, /*gang=*/false,
       /*worker=*/false, /*vector=*/false, /*seq=*/false, /*nohost=*/false,
      /*implicit=*/false, /*gangDim*/mlir::IntegerAttr{});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I have a follow up patch that will update the operation creation.

Guzhu-AMD pushed a commit to GPUOpen-Drivers/llvm-project that referenced this pull request Oct 12, 2023
Local branch amd-gfx 5c22358 Merged main:918829959f96 into amd-gfx:7ce30e5343ec
Remote branch main 4f98fb2 [mlir][openacc][NFC] Remove useless OptionalAttr with UnitAttr (llvm#68337)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang:fir-hlfir flang Flang issues not falling into any other category mlir:openacc mlir openacc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants