[Mlir-commits] [mlir] [mlir][Arith] Specify evaluation order of `getExpr` (PR #87859)

[Matthias Springer]

https://github.com/matthias-springer created https://github.com/llvm/llvm-project/pull/87859

The C++ standard does not specify an evaluation order for addition/... operands. E.g., in `a() + b()`, the compiler is free to evaluate `a` or `b` first.

This lead to different `mlir-opt` outputs in #85895. (FileCheck passed when compiled with LLVM but failed when compiled with gcc.)

>From b0f3633afd30793b8169c8625707ed697a78b3f9 Mon Sep 17 00:00:00 2001
From: Matthias Springer <springerm at google.com>
Date: Sat, 6 Apr 2024 03:21:07 +0000
Subject: [PATCH] [mlir][Arith] Specify evaluation order of `getExpr`

The C++ standard does not specify an evaluation order for addition/...
operands. E.g., in `a() + b()`, the compiler is free to evaluate `a` or
`b` first.

This lead to different `mlir-opt` outputs in #85895. (FileCheck passed
when compiled with LLVM but failed when compiled with gcc.)
---
 .../Arith/IR/ValueBoundsOpInterfaceImpl.cpp   | 21 +++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/mlir/lib/Dialect/Arith/IR/ValueBoundsOpInterfaceImpl.cpp b/mlir/lib/Dialect/Arith/IR/ValueBoundsOpInterfaceImpl.cpp
index 9c6b50e767ea26..90895e381c74b5 100644
--- a/mlir/lib/Dialect/Arith/IR/ValueBoundsOpInterfaceImpl.cpp
+++ b/mlir/lib/Dialect/Arith/IR/ValueBoundsOpInterfaceImpl.cpp
@@ -24,8 +24,15 @@ struct AddIOpInterface
     auto addIOp = cast<AddIOp>(op);
     assert(value == addIOp.getResult() && "invalid value");
 
-    cstr.bound(value) ==
-        cstr.getExpr(addIOp.getLhs()) + cstr.getExpr(addIOp.getRhs());
+    // Note: `getExpr` has a side effect: it may add a new column to the
+    // constraint system. The evaluation order of addition operands is
+    // unspecified in C++. To make sure that all compilers produce the exact
+    // same results (that can be FileCheck'd), it is important that `getExpr`
+    // is called first and assigned to temporary variables, and the addition
+    // is performed afterwards.
+    AffineExpr lhs = cstr.getExpr(addIOp.getLhs());
+    AffineExpr rhs = cstr.getExpr(addIOp.getRhs());
+    cstr.bound(value) == lhs + rhs;
   }
 };
 
@@ -49,8 +56,9 @@ struct SubIOpInterface
     auto subIOp = cast<SubIOp>(op);
     assert(value == subIOp.getResult() && "invalid value");
 
-    cstr.bound(value) ==
-        cstr.getExpr(subIOp.getLhs()) - cstr.getExpr(subIOp.getRhs());
+    AffineExpr lhs = cstr.getExpr(subIOp.getLhs());
+    AffineExpr rhs = cstr.getExpr(subIOp.getRhs());
+    cstr.bound(value) == lhs - rhs;
   }
 };
 
@@ -61,8 +69,9 @@ struct MulIOpInterface
     auto mulIOp = cast<MulIOp>(op);
     assert(value == mulIOp.getResult() && "invalid value");
 
-    cstr.bound(value) ==
-        cstr.getExpr(mulIOp.getLhs()) * cstr.getExpr(mulIOp.getRhs());
+    AffineExpr lhs = cstr.getExpr(mulIOp.getLhs());
+    AffineExpr rhs = cstr.getExpr(mulIOp.getRhs());
+    cstr.bound(value) == lhs *rhs;
   }
 };