[mlir][acc] Use consistent name for device_num operand

[razvanlupusoru]

acc.set, acc.init, and acc.shutdown take a device_num operand. However, this was named inconsistently. Give it the same consistent name for all aforementioned operations.

[llvmbot]

@llvm/pr-subscribers-clangir
@llvm/pr-subscribers-openacc

@llvm/pr-subscribers-mlir-openacc

Author: Razvan Lupusoru (razvanlupusoru)

Changes

acc.set, acc.init, and acc.shutdown take a device_num operand. However, this was named inconsistently. Give it the same consistent name for all aforementioned operations.

---

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

1 Files Affected:

• (modified) mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td (+4-4)

diff --git a/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td b/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
index 275472bc5edd9..5e249e639d837 100644
--- a/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
+++ b/mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
@@ -2611,11 +2611,11 @@ def OpenACC_InitOp : OpenACC_Op<"init", [AttrSizedOperandSegments]> {
   }];
 
   let arguments = (ins OptionalAttr<TypedArrayAttrBase<OpenACC_DeviceTypeAttr, "Device type attributes">>:$device_types,
-                       Optional<IntOrIndex>:$deviceNumOperand,
+                       Optional<IntOrIndex>:$deviceNum,
                        Optional<I1>:$ifCond);
 
   let assemblyFormat = [{
-    oilist(`device_num` `(` $deviceNumOperand `:` type($deviceNumOperand) `)`
+    oilist(`device_num` `(` $deviceNum `:` type($deviceNum) `)`
       | `if` `(` $ifCond `)`
     ) attr-dict-with-keyword
   }];
@@ -2642,11 +2642,11 @@ def OpenACC_ShutdownOp : OpenACC_Op<"shutdown", [AttrSizedOperandSegments]> {
   }];
 
   let arguments = (ins OptionalAttr<TypedArrayAttrBase<OpenACC_DeviceTypeAttr, "Device type attributes">>:$device_types,
-                       Optional<IntOrIndex>:$deviceNumOperand,
+                       Optional<IntOrIndex>:$deviceNum,
                        Optional<I1>:$ifCond);
 
   let assemblyFormat = [{
-    oilist(`device_num` `(` $deviceNumOperand `:` type($deviceNumOperand) `)`
+    oilist(`device_num` `(` $deviceNum `:` type($deviceNum) `)`
     |`if` `(` $ifCond `)`
     ) attr-dict-with-keyword
   }];

[clementval]

LGTM. Thanks Razvan!

[erichkeane]

Thanks! This might end up failing to compile the clang-lowering as a result, so I'll see if I can just pop a patch onto this to make it 'just work' for us.

[erichkeane]

Also, we have getIfCondMutable in a number of places as well, which is now inconsistent with skipping the 'operand'.

[razvanlupusoru]

Also, we have getIfCondMutable in a number of places as well, which is now inconsistent with skipping the 'operand'.

I am not sure I follow - are you saying that for consistency, you think that the word 'operand' should be included in the name of ifCond (or excluded for other cases)? We do have multiple cases where we do not use the word 'operand' - for example in acc.parallel we have numGangs, numWorkers, vectorLength, selfCond, ifCond. It is used in cases where it looks awkward to not have it. For example asyncOperands and waitOperands. This is subjective though :)

[razvanlupusoru]

I plan on merging this as soon as the 2 pending checks complete :)

[erichkeane]

Also, we have getIfCondMutable in a number of places as well, which is now inconsistent with skipping the 'operand'.

I am not sure I follow - are you saying that for consistency, you think that the word 'operand' should be included in the name of ifCond (or excluded for other cases)? We do have multiple cases where we do not use the word 'operand' - for example in acc.parallel we have numGangs, numWorkers, vectorLength, selfCond, ifCond. It is used in cases where it looks awkward to not have it. For example asyncOperands and waitOperands. This is subjective though :)

I guess I would like us to be consistent between all of them? I consider ifCond and deviceNumOperand to be equivalent, and if and deviceNum to be equivalent.

I admit it is quite subjective, and I'm happy as long as the name of the 'same thing' is the same between constructs, but I would expect them to either consistently name that it is the argument to the clause, or the clause itself.

[erichkeane]

I plan on merging this as soon as the 2 pending checks complete :)

Despite the above suggestion, I still believe this is a positive change, so still support merging this.