Skip to content

[mlir][SPRIV][NFC] Avoid rollback in TypeCastingOpPattern #136284

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

Conversation

matthias-springer
Copy link
Member

This pattern used to create an op and then attached the converted rounding mode attribute. When the latter failed, the pattern aborted and a rollback was triggered.

This commit inverses the logic: the converted rounding mode is computed first, so that no changes have to be rolled back.

Note: This is in preparation of the One-Shot Dialect Conversion refactoring.

This pattern used to create an op and then attached the converted rounding mode attribute. When the latter failed, a rollback was triggered.

This commit inverses the logic: the converted rounding mode is computed first, so that no changes have to be rolled back.

Note: This is in preparation of the One-Shot Dialect Conversion refactoring.
@llvmbot
Copy link
Member

llvmbot commented Apr 18, 2025

@llvm/pr-subscribers-mlir-spirv

@llvm/pr-subscribers-mlir

Author: Matthias Springer (matthias-springer)

Changes

This pattern used to create an op and then attached the converted rounding mode attribute. When the latter failed, the pattern aborted and a rollback was triggered.

This commit inverses the logic: the converted rounding mode is computed first, so that no changes have to be rolled back.

Note: This is in preparation of the One-Shot Dialect Conversion refactoring.


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

1 Files Affected:

  • (modified) mlir/lib/Conversion/ArithToSPIRV/ArithToSPIRV.cpp (+12-8)
diff --git a/mlir/lib/Conversion/ArithToSPIRV/ArithToSPIRV.cpp b/mlir/lib/Conversion/ArithToSPIRV/ArithToSPIRV.cpp
index 9c4dfa27b1447..434d7df853a5e 100644
--- a/mlir/lib/Conversion/ArithToSPIRV/ArithToSPIRV.cpp
+++ b/mlir/lib/Conversion/ArithToSPIRV/ArithToSPIRV.cpp
@@ -847,24 +847,28 @@ struct TypeCastingOpPattern final : public OpConversionPattern<Op> {
       // Then we can just erase this operation by forwarding its operand.
       rewriter.replaceOp(op, adaptor.getOperands().front());
     } else {
-      auto newOp = rewriter.template replaceOpWithNewOp<SPIRVOp>(
-          op, dstType, adaptor.getOperands());
+      // Compute new rounding mode (if any).
+      std::optional<spirv::FPRoundingMode> rm = std::nullopt;
       if (auto roundingModeOp =
               dyn_cast<arith::ArithRoundingModeInterface>(*op)) {
         if (arith::RoundingModeAttr roundingMode =
                 roundingModeOp.getRoundingModeAttr()) {
-          if (auto rm =
-                  convertArithRoundingModeToSPIRV(roundingMode.getValue())) {
-            newOp->setAttr(
-                getDecorationString(spirv::Decoration::FPRoundingMode),
-                spirv::FPRoundingModeAttr::get(rewriter.getContext(), *rm));
-          } else {
+          if (!(rm =
+                    convertArithRoundingModeToSPIRV(roundingMode.getValue()))) {
             return rewriter.notifyMatchFailure(
                 op->getLoc(),
                 llvm::formatv("unsupported rounding mode '{0}'", roundingMode));
           }
         }
       }
+      // Create replacement op and attach rounding mode attribute (if any).
+      auto newOp = rewriter.template replaceOpWithNewOp<SPIRVOp>(
+          op, dstType, adaptor.getOperands());
+      if (rm) {
+        newOp->setAttr(
+            getDecorationString(spirv::Decoration::FPRoundingMode),
+            spirv::FPRoundingModeAttr::get(rewriter.getContext(), *rm));
+      }
     }
     return success();
   }

@matthias-springer matthias-springer merged commit c47042c into main Apr 19, 2025
14 checks passed
@matthias-springer matthias-springer deleted the users/matthias-springer/no_rollback_spirv_cast branch April 19, 2025 08:01
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