[mlir] Ensure fmaximum/fminimum is defined in mlir/Dialect/LLVMIR/LLVMEnums.td

[jthackray]

Fix a compile error (with LLVM_ENABLE_WERROR=On) when building bin/flang:

   enumeration values 'FMaximum' and 'FMinimum' not handled in switch

caused by adding new LLVM IR instructions in #137701.

This wasn't picked up by the auto CI builders on GitHub, so I hadn't
realised until @kazutakahirata notified me about it.

[llvmbot]

@llvm/pr-subscribers-mlir-llvm

@llvm/pr-subscribers-mlir

Author: Jonathan Thackray (jthackray)

Changes

Fix a compile error (with LLVM_ENABLE_WERROR=On) when building bin/flang:

   enumeration values 'FMaximum' and 'FMinimum' not handled in switch

caused by adding new LLVM IR instructions in #137701.

This wasn't picked up by the auto CI builders on GitHub, so I hadn't
realised until @kazutakahirata notified me about it.

---

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

1 Files Affected:

• (modified) mlir/include/mlir/Dialect/LLVMIR/LLVMEnums.td (+4-1)

diff --git a/mlir/include/mlir/Dialect/LLVMIR/LLVMEnums.td b/mlir/include/mlir/Dialect/LLVMIR/LLVMEnums.td
index 6c0fe363d5551..596f562911f8f 100644
--- a/mlir/include/mlir/Dialect/LLVMIR/LLVMEnums.td
+++ b/mlir/include/mlir/Dialect/LLVMIR/LLVMEnums.td
@@ -110,6 +110,8 @@ def AtomicBinOpUSubCond : LLVM_EnumAttrCase<"usub_cond",
                                             "usub_cond", "USubCond", 17>;
 def AtomicBinOpUSubSat : LLVM_EnumAttrCase<"usub_sat",
                                            "usub_sat", "USubSat", 18>;
+def AtomicBinOpFMaximum : LLVM_EnumAttrCase<"fmaximum", "fmaximum", "FMaximum", 19>;
+def AtomicBinOpFMinimum : LLVM_EnumAttrCase<"fminimum", "fminimum", "FMinimum", 20>;
 
 // A sentinel value that has no MLIR counterpart.
 def AtomicBadBinOp : LLVM_EnumAttrCase<"", "", "BAD_BINOP", 0>;
@@ -122,7 +124,8 @@ def AtomicBinOp : LLVM_EnumAttr<
      AtomicBinOpNand, AtomicBinOpOr, AtomicBinOpXor, AtomicBinOpMax,
      AtomicBinOpMin, AtomicBinOpUMax, AtomicBinOpUMin, AtomicBinOpFAdd,
      AtomicBinOpFSub, AtomicBinOpFMax, AtomicBinOpFMin, AtomicBinOpUIncWrap,
-     AtomicBinOpUDecWrap, AtomicBinOpUSubCond, AtomicBinOpUSubSat],
+     AtomicBinOpUDecWrap, AtomicBinOpUSubCond, AtomicBinOpUSubSat,
+     AtomicBinOpFMaximum, AtomicBinOpFMinimum],
     [AtomicBadBinOp]> {
   let cppNamespace = "::mlir::LLVM";
 }

[arsenm]

Probably should have tests showing this translates correctly?

[krzysz00]

Approved

Can you do me a favor and have a followup PR where you hook these up to memref.atomicrmw?

[krzysz00]

(Huh, yeah, should have a test)

[jthackray]

Probably should have tests showing this translates correctly?

Thanks @arsenm. I'm not familiar with where existing tests for other atomics are tested in this way. Can you point them out to me? Thanks.

[jthackray]

Can you do me a favor and have a followup PR where you hook these up to memref.atomicrmw?

@krzysz00 - sure, not sure exactly how to do that, but I'll give it a try tomorrow.

[arsenm]

Probably should have tests showing this translates correctly?

Thanks @arsenm. I'm not familiar with where existing tests for other atomics are tested in this way. Can you point them out to me? Thanks.

I don't know how to mlir, but I would hope there is an existing test to adjust. the closest match grep finds is mlir/test/Target/LLVMIR/llvmir.mlir? seems to cover too much, but has most atomicrmw ops covered

[krzysz00]

The tests are in mlir/test/Target/LLVMIR/ - llvmir.mlir has the export tests and import.ll has the import tests. There's a bunch of other llvm.atomicrmw in there

[kazutakahirata]

Thank you for reproducing and fixing the problem quickly!

[jthackray]

Thank you for reproducing and fixing the problem quickly!

No problem - sorry I didn't see your original comment 17 hours ago. This won't get merged until tomorrow and I've added tests - hope you can wait until then. You can manually apply my patch to your local build if you need it sooner.

[jthackray]

Thanks for the pointers, @arsenm @krzysz00 - I've added some extra tests now.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.