mlir/Presburger: contribute a free-standing parser #94916
Conversation
|
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-presburger Author: Ramkumar Ramachandra (artagnon) ChangesThe Presburger library is already quite independent of MLIR, with the exception of the MLIR Support library. There is, however, one major exception: the test suite for the library depends on the core MLIR parser. To free it of this dependency, extract the parts of the core MLIR parser that are applicable to the Presburger test suite, author custom parsing data structures, and adapt the new parser to parse into these structures. This patch is part of a project to move the Presburger library into LLVM. Patch is 118.80 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/94916.diff 29 Files Affected:
diff --git a/mlir/unittests/Analysis/Presburger/Parser.h b/mlir/include/mlir/Analysis/Presburger/Parser.h
similarity index 67%
rename from mlir/unittests/Analysis/Presburger/Parser.h
rename to mlir/include/mlir/Analysis/Presburger/Parser.h
index 75842fb054e2b..67f6766fe5bd2 100644
--- a/mlir/unittests/Analysis/Presburger/Parser.h
+++ b/mlir/include/mlir/Analysis/Presburger/Parser.h
@@ -11,32 +11,25 @@
//
//===----------------------------------------------------------------------===//
-#ifndef MLIR_UNITTESTS_ANALYSIS_PRESBURGER_PARSER_H
-#define MLIR_UNITTESTS_ANALYSIS_PRESBURGER_PARSER_H
+#ifndef MLIR_ANALYSIS_PRESBURGER_PARSER_H
+#define MLIR_ANALYSIS_PRESBURGER_PARSER_H
#include "mlir/Analysis/Presburger/IntegerRelation.h"
#include "mlir/Analysis/Presburger/PWMAFunction.h"
#include "mlir/Analysis/Presburger/PresburgerRelation.h"
-#include "mlir/AsmParser/AsmParser.h"
-#include "mlir/Dialect/Affine/Analysis/AffineStructures.h"
-#include "mlir/IR/AffineExpr.h"
-#include "mlir/IR/AffineMap.h"
-#include "mlir/IR/IntegerSet.h"
namespace mlir {
namespace presburger {
+/// Parses an IntegerPolyhedron from a StringRef.
+IntegerPolyhedron parseIntegerPolyhedron(StringRef str);
-/// Parses an IntegerPolyhedron from a StringRef. It is expected that the string
-/// represents a valid IntegerSet.
-inline IntegerPolyhedron parseIntegerPolyhedron(StringRef str) {
- MLIRContext context(MLIRContext::Threading::DISABLED);
- return affine::FlatAffineValueConstraints(parseIntegerSet(str, &context));
-}
+/// Parses a MultiAffineFunction from a StringRef.
+MultiAffineFunction parseMultiAffineFunction(StringRef str);
/// Parse a list of StringRefs to IntegerRelation and combine them into a
-/// PresburgerSet by using the union operation. It is expected that the strings
-/// are all valid IntegerSet representation and that all of them have compatible
-/// spaces.
+/// PresburgerSet by using the union operation. It is expected that the
+/// strings are all valid IntegerSet representation and that all of them have
+/// compatible spaces.
inline PresburgerSet parsePresburgerSet(ArrayRef<StringRef> strs) {
assert(!strs.empty() && "strs should not be empty");
@@ -47,25 +40,10 @@ inline PresburgerSet parsePresburgerSet(ArrayRef<StringRef> strs) {
return result;
}
-inline MultiAffineFunction parseMultiAffineFunction(StringRef str) {
- MLIRContext context(MLIRContext::Threading::DISABLED);
-
- // TODO: Add default constructor for MultiAffineFunction.
- MultiAffineFunction multiAff(PresburgerSpace::getRelationSpace(),
- IntMatrix(0, 1));
- if (getMultiAffineFunctionFromMap(parseAffineMap(str, &context), multiAff)
- .failed())
- llvm_unreachable(
- "Failed to parse MultiAffineFunction because of semi-affinity");
- return multiAff;
-}
-
inline PWMAFunction
parsePWMAF(ArrayRef<std::pair<StringRef, StringRef>> pieces) {
assert(!pieces.empty() && "At least one piece should be present.");
- MLIRContext context(MLIRContext::Threading::DISABLED);
-
IntegerPolyhedron initDomain = parseIntegerPolyhedron(pieces[0].first);
MultiAffineFunction initMultiAff = parseMultiAffineFunction(pieces[0].second);
@@ -100,8 +78,7 @@ parsePresburgerRelationFromPresburgerSet(ArrayRef<StringRef> strs,
result.convertVarKind(VarKind::SetDim, 0, numDomain, VarKind::Domain, 0);
return result;
}
-
} // namespace presburger
} // namespace mlir
-#endif // MLIR_UNITTESTS_ANALYSIS_PRESBURGER_PARSER_H
+#endif // MLIR_ANALYSIS_PRESBURGER_PARSER_H
diff --git a/mlir/lib/Analysis/Presburger/CMakeLists.txt b/mlir/lib/Analysis/Presburger/CMakeLists.txt
index 83d0514c9e7d1..4a89e3929a709 100644
--- a/mlir/lib/Analysis/Presburger/CMakeLists.txt
+++ b/mlir/lib/Analysis/Presburger/CMakeLists.txt
@@ -1,3 +1,5 @@
+add_subdirectory(Parser)
+
add_mlir_library(MLIRPresburger
Barvinok.cpp
IntegerRelation.cpp
diff --git a/mlir/lib/Analysis/Presburger/Parser/CMakeLists.txt b/mlir/lib/Analysis/Presburger/Parser/CMakeLists.txt
new file mode 100644
index 0000000000000..f708a5c8db949
--- /dev/null
+++ b/mlir/lib/Analysis/Presburger/Parser/CMakeLists.txt
@@ -0,0 +1,6 @@
+add_mlir_library(MLIRPresburgerParser
+ Flattener.cpp
+ Lexer.cpp
+ ParserImpl.cpp
+ ParseStructs.cpp
+ Token.cpp)
diff --git a/mlir/lib/Analysis/Presburger/Parser/Flattener.cpp b/mlir/lib/Analysis/Presburger/Parser/Flattener.cpp
new file mode 100644
index 0000000000000..97507b64b28e7
--- /dev/null
+++ b/mlir/lib/Analysis/Presburger/Parser/Flattener.cpp
@@ -0,0 +1,410 @@
+//===- Flattener.cpp - Presburger ParseStruct Flattener ---------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file implements the Flattener class for flattening the parse tree
+// produced by the parser for the Presburger library.
+//
+//===----------------------------------------------------------------------===//
+
+#include "Flattener.h"
+#include "llvm/ADT/SmallVector.h"
+
+namespace mlir {
+namespace presburger {
+namespace detail {
+using llvm::SmallVector;
+
+AffineExpr AffineExprFlattener::getAffineExprFromFlatForm(
+ ArrayRef<int64_t> flatExprs, unsigned numDims, unsigned numSymbols) {
+ assert(flatExprs.size() - numDims - numSymbols - 1 == localExprs.size() &&
+ "unexpected number of local expressions");
+
+ // Dimensions and symbols.
+ AffineExpr expr = std::make_unique<AffineConstantExpr>(0);
+ for (unsigned j = 0; j < getLocalVarStartIndex(); ++j) {
+ if (flatExprs[j] == 0)
+ continue;
+ if (j < numDims)
+ expr =
+ std::move(expr) + std::make_unique<AffineDimExpr>(j) * flatExprs[j];
+ else
+ expr = std::move(expr) +
+ std::make_unique<AffineSymbolExpr>(j - numDims) * flatExprs[j];
+ }
+
+ // Local identifiers.
+ for (unsigned j = getLocalVarStartIndex(); j < flatExprs.size() - 1; ++j) {
+ if (flatExprs[j] == 0)
+ continue;
+ // It is safe to move out of the localExprs vector, since no expr is used
+ // more than once.
+ AffineExpr term =
+ std::move(localExprs[j - getLocalVarStartIndex()]) * flatExprs[j];
+ expr = std::move(expr) + std::move(term);
+ }
+
+ // Constant term.
+ int64_t constTerm = flatExprs[flatExprs.size() - 1];
+ if (constTerm != 0)
+ return std::move(expr) + constTerm;
+ return expr;
+}
+
+// In pure affine t = expr * c, we multiply each coefficient of lhs with c.
+//
+// In case of semi affine multiplication expressions, t = expr * symbolic_expr,
+// introduce a local variable p (= expr * symbolic_expr), and the affine
+// expression expr * symbolic_expr is added to `localExprs`.
+LogicalResult AffineExprFlattener::visitMulExpr(const AffineBinOpExpr &expr) {
+ assert(operandExprStack.size() >= 2);
+ SmallVector<int64_t, 8> rhs = operandExprStack.back();
+ operandExprStack.pop_back();
+ SmallVector<int64_t, 8> &lhs = operandExprStack.back();
+
+ // Flatten semi-affine multiplication expressions by introducing a local
+ // variable in place of the product; the affine expression
+ // corresponding to the quantifier is added to `localExprs`.
+ if (!isa<AffineConstantExpr>(expr.getRHS())) {
+ AffineExpr a = getAffineExprFromFlatForm(lhs, numDims, numSymbols);
+ AffineExpr b = getAffineExprFromFlatForm(rhs, numDims, numSymbols);
+ addLocalVariableSemiAffine(std::move(a) * std::move(b), lhs, lhs.size());
+ return success();
+ }
+
+ // Get the RHS constant.
+ int64_t rhsConst = rhs[getConstantIndex()];
+ for (int64_t &lhsElt : lhs)
+ lhsElt *= rhsConst;
+
+ return success();
+}
+
+LogicalResult AffineExprFlattener::visitAddExpr(const AffineBinOpExpr &expr) {
+ assert(operandExprStack.size() >= 2);
+ const auto &rhs = operandExprStack.back();
+ auto &lhs = operandExprStack[operandExprStack.size() - 2];
+ assert(lhs.size() == rhs.size());
+ // Update the LHS in place.
+ for (unsigned i = 0; i < rhs.size(); ++i)
+ lhs[i] += rhs[i];
+ // Pop off the RHS.
+ operandExprStack.pop_back();
+ return success();
+}
+
+//
+// t = expr mod c <=> t = expr - c*q and c*q <= expr <= c*q + c - 1
+//
+// A mod expression "expr mod c" is thus flattened by introducing a new local
+// variable q (= expr floordiv c), such that expr mod c is replaced with
+// 'expr - c * q' and c * q <= expr <= c * q + c - 1 are added to localVarCst.
+//
+// In case of semi-affine modulo expressions, t = expr mod symbolic_expr,
+// introduce a local variable m (= expr mod symbolic_expr), and the affine
+// expression expr mod symbolic_expr is added to `localExprs`.
+LogicalResult AffineExprFlattener::visitModExpr(const AffineBinOpExpr &expr) {
+ assert(operandExprStack.size() >= 2);
+
+ SmallVector<int64_t, 8> rhs = operandExprStack.back();
+ operandExprStack.pop_back();
+ SmallVector<int64_t, 8> &lhs = operandExprStack.back();
+
+ // Flatten semi affine modulo expressions by introducing a local
+ // variable in place of the modulo value, and the affine expression
+ // corresponding to the quantifier is added to `localExprs`.
+ if (!isa<AffineConstantExpr>(expr.getRHS())) {
+ AffineExpr dividendExpr =
+ getAffineExprFromFlatForm(lhs, numDims, numSymbols);
+ AffineExpr divisorExpr =
+ getAffineExprFromFlatForm(rhs, numDims, numSymbols);
+ AffineExpr modExpr = std::move(dividendExpr) % std::move(divisorExpr);
+ addLocalVariableSemiAffine(std::move(modExpr), lhs, lhs.size());
+ return success();
+ }
+
+ int64_t rhsConst = rhs[getConstantIndex()];
+ if (rhsConst <= 0)
+ return failure();
+
+ // Check if the LHS expression is a multiple of modulo factor.
+ unsigned i;
+ for (i = 0; i < lhs.size(); ++i)
+ if (lhs[i] % rhsConst != 0)
+ break;
+ // If yes, modulo expression here simplifies to zero.
+ if (i == lhs.size()) {
+ std::fill(lhs.begin(), lhs.end(), 0);
+ return success();
+ }
+
+ // Add a local variable for the quotient, i.e., expr % c is replaced by
+ // (expr - q * c) where q = expr floordiv c. Do this while canceling out
+ // the GCD of expr and c.
+ SmallVector<int64_t, 8> floorDividend(lhs);
+ uint64_t gcd = rhsConst;
+ for (int64_t lhsElt : lhs)
+ gcd = std::gcd(gcd, (uint64_t)std::abs(lhsElt));
+ // Simplify the numerator and the denominator.
+ if (gcd != 1) {
+ for (int64_t &floorDividendElt : floorDividend)
+ floorDividendElt = floorDividendElt / static_cast<int64_t>(gcd);
+ }
+ int64_t floorDivisor = rhsConst / static_cast<int64_t>(gcd);
+
+ // Construct the AffineExpr form of the floordiv to store in localExprs.
+
+ AffineExpr dividendExpr =
+ getAffineExprFromFlatForm(floorDividend, numDims, numSymbols);
+ AffineExpr divisorExpr = std::make_unique<AffineConstantExpr>(floorDivisor);
+ AffineExpr floorDivExpr =
+ floorDiv(std::move(dividendExpr), std::move(divisorExpr));
+ int loc;
+ if ((loc = findLocalId(floorDivExpr)) == -1) {
+ addLocalFloorDivId(floorDividend, floorDivisor, std::move(floorDivExpr));
+ // Set result at top of stack to "lhs - rhsConst * q".
+ lhs[getLocalVarStartIndex() + numLocals - 1] = -rhsConst;
+ } else {
+ // Reuse the existing local id.
+ lhs[getLocalVarStartIndex() + loc] = -rhsConst;
+ }
+ return success();
+}
+
+LogicalResult
+AffineExprFlattener::visitCeilDivExpr(const AffineBinOpExpr &expr) {
+ return visitDivExpr(expr, /*isCeil=*/true);
+}
+LogicalResult
+AffineExprFlattener::visitFloorDivExpr(const AffineBinOpExpr &expr) {
+ return visitDivExpr(expr, /*isCeil=*/false);
+}
+
+LogicalResult AffineExprFlattener::visitDimExpr(const AffineDimExpr &expr) {
+ operandExprStack.emplace_back(SmallVector<int64_t, 32>(getNumCols(), 0));
+ auto &eq = operandExprStack.back();
+ assert(expr.getPosition() < numDims && "Inconsistent number of dims");
+ eq[getDimStartIndex() + expr.getPosition()] = 1;
+ return success();
+}
+
+LogicalResult
+AffineExprFlattener::visitSymbolExpr(const AffineSymbolExpr &expr) {
+ operandExprStack.emplace_back(SmallVector<int64_t, 32>(getNumCols(), 0));
+ auto &eq = operandExprStack.back();
+ assert(expr.getPosition() < numSymbols && "inconsistent number of symbols");
+ eq[getSymbolStartIndex() + expr.getPosition()] = 1;
+ return success();
+}
+
+LogicalResult
+AffineExprFlattener::visitConstantExpr(const AffineConstantExpr &expr) {
+ operandExprStack.emplace_back(SmallVector<int64_t, 32>(getNumCols(), 0));
+ auto &eq = operandExprStack.back();
+ eq[getConstantIndex()] = expr.getValue();
+ return success();
+}
+
+void AffineExprFlattener::addLocalVariableSemiAffine(
+ AffineExpr &&expr, SmallVectorImpl<int64_t> &result,
+ unsigned long resultSize) {
+ assert(result.size() == resultSize && "result vector size mismatch");
+ int loc;
+ if ((loc = findLocalId(expr)) == -1)
+ addLocalIdSemiAffine(std::move(expr));
+ std::fill(result.begin(), result.end(), 0);
+ if (loc == -1)
+ result[getLocalVarStartIndex() + numLocals - 1] = 1;
+ else
+ result[getLocalVarStartIndex() + loc] = 1;
+}
+
+// t = expr floordiv c <=> t = q, c * q <= expr <= c * q + c - 1
+// A floordiv is thus flattened by introducing a new local variable q, and
+// replacing that expression with 'q' while adding the constraints
+// c * q <= expr <= c * q + c - 1 to localVarCst (done by
+// IntegerRelation::addLocalFloorDiv).
+//
+// A ceildiv is similarly flattened:
+// t = expr ceildiv c <=> t = (expr + c - 1) floordiv c
+//
+// In case of semi affine division expressions, t = expr floordiv symbolic_expr
+// or t = expr ceildiv symbolic_expr, introduce a local variable q (= expr
+// floordiv/ceildiv symbolic_expr), and the affine floordiv/ceildiv is added to
+// `localExprs`.
+LogicalResult AffineExprFlattener::visitDivExpr(const AffineBinOpExpr &expr,
+ bool isCeil) {
+ assert(operandExprStack.size() >= 2);
+
+ SmallVector<int64_t, 8> rhs = operandExprStack.back();
+ operandExprStack.pop_back();
+ SmallVector<int64_t, 8> &lhs = operandExprStack.back();
+
+ // Flatten semi affine division expressions by introducing a local
+ // variable in place of the quotient, and the affine expression corresponding
+ // to the quantifier is added to `localExprs`.
+ if (!isa<AffineConstantExpr>(expr.getRHS())) {
+ AffineExpr a = getAffineExprFromFlatForm(lhs, numDims, numSymbols);
+ AffineExpr b = getAffineExprFromFlatForm(rhs, numDims, numSymbols);
+ AffineExpr divExpr = isCeil ? ceilDiv(std::move(a), std::move(b))
+ : floorDiv(std::move(a), std::move(b));
+ addLocalVariableSemiAffine(std::move(divExpr), lhs, lhs.size());
+ return success();
+ }
+
+ // This is a pure affine expr; the RHS is a positive constant.
+ int64_t rhsConst = rhs[getConstantIndex()];
+ if (rhsConst <= 0)
+ return failure();
+
+ // Simplify the floordiv, ceildiv if possible by canceling out the greatest
+ // common divisors of the numerator and denominator.
+ uint64_t gcd = std::abs(rhsConst);
+ for (int64_t lhsElt : lhs)
+ gcd = std::gcd(gcd, (uint64_t)std::abs(lhsElt));
+ // Simplify the numerator and the denominator.
+ if (gcd != 1) {
+ for (int64_t &lhsElt : lhs)
+ lhsElt = lhsElt / static_cast<int64_t>(gcd);
+ }
+ int64_t divisor = rhsConst / static_cast<int64_t>(gcd);
+ // If the divisor becomes 1, the updated LHS is the result. (The
+ // divisor can't be negative since rhsConst is positive).
+ if (divisor == 1)
+ return success();
+
+ // If the divisor cannot be simplified to one, we will have to retain
+ // the ceil/floor expr (simplified up until here). Add an existential
+ // quantifier to express its result, i.e., expr1 div expr2 is replaced
+ // by a new identifier, q.
+ AffineExpr a = getAffineExprFromFlatForm(lhs, numDims, numSymbols);
+ AffineExpr b = std::make_unique<AffineConstantExpr>(divisor);
+
+ int loc;
+ AffineExpr divExpr = isCeil ? ceilDiv(std::move(a), std::move(b))
+ : floorDiv(std::move(a), std::move(b));
+ if ((loc = findLocalId(divExpr)) == -1) {
+ if (!isCeil) {
+ SmallVector<int64_t, 8> dividend(lhs);
+ addLocalFloorDivId(dividend, divisor, std::move(divExpr));
+ } else {
+ // lhs ceildiv c <=> (lhs + c - 1) floordiv c
+ SmallVector<int64_t, 8> dividend(lhs);
+ dividend.back() += divisor - 1;
+ addLocalFloorDivId(dividend, divisor, std::move(divExpr));
+ }
+ }
+ // Set the expression on stack to the local var introduced to capture the
+ // result of the division (floor or ceil).
+ std::fill(lhs.begin(), lhs.end(), 0);
+ if (loc == -1)
+ lhs[getLocalVarStartIndex() + numLocals - 1] = 1;
+ else
+ lhs[getLocalVarStartIndex() + loc] = 1;
+ return success();
+}
+
+void AffineExprFlattener::addLocalFloorDivId(ArrayRef<int64_t> dividend,
+ int64_t divisor,
+ AffineExpr &&localExpr) {
+ assert(divisor > 0 && "positive constant divisor expected");
+ for (SmallVector<int64_t, 8> &subExpr : operandExprStack)
+ subExpr.insert(subExpr.begin() + getLocalVarStartIndex() + numLocals, 0);
+ localExprs.emplace_back(std::move(localExpr));
+ ++numLocals;
+ // Update localVarCst.
+ localVarCst.addLocalFloorDiv(dividend, divisor);
+}
+
+void AffineExprFlattener::addLocalIdSemiAffine(AffineExpr &&localExpr) {
+ for (SmallVector<int64_t, 8> &subExpr : operandExprStack)
+ subExpr.insert(subExpr.begin() + getLocalVarStartIndex() + numLocals, 0);
+ localExprs.emplace_back(std::move(localExpr));
+ ++numLocals;
+}
+
+int AffineExprFlattener::findLocalId(const AffineExpr &localExpr) {
+ auto *it = llvm::find(localExprs, localExpr);
+ if (it == localExprs.end())
+ return -1;
+ return it - localExprs.begin();
+}
+
+AffineExprFlattener::AffineExprFlattener(unsigned numDims, unsigned numSymbols)
+ : numDims(numDims), numSymbols(numSymbols), numLocals(0),
+ localVarCst(PresburgerSpace::getSetSpace(numDims, numSymbols)) {
+ operandExprStack.reserve(8);
+}
+
+// Flattens the expressions in map. Returns failure if 'expr' was unable to be
+// flattened. For example two specific cases:
+// 1. semi-affine expressions not handled yet.
+// 2. has poison expression (i.e., division by zero).
+static LogicalResult
+getFlattenedAffineExprs(ArrayRef<AffineExpr> exprs, unsigned numDims,
+ unsigned numSymbols,
+ std::vector<SmallVector<int64_t, 8>> &flattenedExprs,
+ IntegerPolyhedron &localVarCst) {
+ if (exprs.empty()) {
+ localVarCst = IntegerPolyhedron(
+ 0, 0, numDims + numSymbols + 1,
+ presburger::PresburgerSpace::getSetSpace(numDims, numSymbols, 0));
+ return success();
+ }
+
+ AffineExprFlattener flattener(numDims, numSymbols);
+ // Use the same flattener to simplify each expression successively. This way
+ // local variables / expressions are shared.
+ for (const AffineExpr &expr : exprs) {
+ if (!expr->isPureAffine())
+ return failure();
+ // has poison expression
+ LogicalResult flattenResult = flattener.walkPostOrder(*expr);
+ if (failed(flattenResult))
+ return failure();
+ }
+
+ assert(flattener.operandExprStack.size() == exprs.size());
+ flattenedExprs.clear();
+ flattenedExprs.assign(flattener.operandExprStack.begin(),
+ flattener.operandExprStack.end());
+
+ localVarCst.clearAndCopyFrom(flattener.localVarCst);
+
+ return success();
+}
+
+LogicalResult
+getFlattenedAffineExprs(const AffineMap &map,
+ std::vector<SmallVector<int64_t, 8>> &flattenedExprs,
+ IntegerPolyhedron &cst) {
+ if (map.getNumExprs() == 0) {
+ cst = IntegerPolyhedron(0, 0, map.getNumDims() + map.getNumSymbols() + 1,
+ presburger::PresburgerSpace::getSetSpace(
+ map.getNumDims(), map.getNumSymbols(), 0));
+ return success();
+ }
+ return getFlattenedAffineExprs(map.getExprs(), map.getNumDims(),
+ map.getNumSymbols(), flattenedExprs, cst);
+}
+
+LogicalResult
+getFlattenedAffineExprs(const IntegerSet &set,
+ std::vector<SmallVector<int64_t, 8>> &flattenedExprs,
+ ...
[truncated]
|
There was a problem hiding this comment.
Thank you for the PR. On an initial look, I see that you are duplicating AffineExpr inside Presburger. I don't think this is the way to go. I would instead prefer a parser that directly builds a MultiAffineFunction/AffineFunction (might need some work inside Presburger) (https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/Analysis/Presburger/PWMAFunction.h#L41) and uses that.
artagnon
force-pushed
the
presburger-parser
branch
from
ec491a0
to
581fad7
Compare
|
I've now written a fresh parser for just pure affine expressions. |
|
I wasn't able to parse directly into a MultiAffineFunction, since I couldn't compute the relation space, which requires knowing the total number of div expressions in advance, but the new parse structures are very thin, and the flattener trivially flattens them directly into an IntMatrix. |
The Presburger library is already quite independent of MLIR, with the exception of the MLIR Support library. There is, however, one major exception: the test suite for the library depends on the core MLIR parser. To free it of this dependency, extract the parts of the core MLIR parser that are applicable to the Presburger test suite, author custom parsing data structures, and adapt the new parser to parse into these structures. This patch is part of a project to move the Presburger library into LLVM.
artagnon
force-pushed
the
presburger-parser
branch
from
52d54ac
to
3545795
Compare
|
Rebase and ping. This is the last bit of work before I can move Presburger into llvm, and demonstrate its usage in ConstraintSystem. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Could I kindly request @nikic or someone else to review this patch in @Groverkss' absence? (He's on vacation for some time) |
|
Not that it matters, but this patch gives a 15% speedup on the unit test binary.
|
|
I can review this over the weekend |
| constexpr unsigned getNumDomainVars() const { return numDomain; } | ||
| constexpr unsigned &numDomainVars() { return numDomain; } | ||
| constexpr unsigned getNumRangeVars() const { return numRange; } | ||
| constexpr unsigned &numRangeVars() { return numRange; } | ||
| constexpr unsigned getNumSetDimVars() const { return numRange; } | ||
| constexpr unsigned &numSetDimVars() { return numRange; } | ||
| constexpr unsigned getNumSymbolVars() const { return numSymbols; } | ||
| constexpr unsigned &numSymbolVars() { return numSymbols; } | ||
| constexpr unsigned getNumLocalVars() const { return numLocals; } | ||
| constexpr unsigned &numLocalVars() { return numLocals; } | ||
| constexpr unsigned getNumDimVars() const { return numDomain + numRange; } | ||
| constexpr unsigned getNumDimAndSymbolVars() const { | ||
| return getNumDimVars() + getNumSymbolVars(); |
There was a problem hiding this comment.
These methods should not return a reference. This allows a user to change a field and get into an unspecified place. Please use insertVar/removeVar methods instead.
There was a problem hiding this comment.
Those methods are insufficient, as I directly set the variable to the value in some places.
| unsigned getNumDimVars() const { return numDomain + numRange; } | ||
| unsigned getNumDimAndSymbolVars() const { | ||
| return numDomain + numRange + numSymbols; | ||
| constexpr bool isSetDimIdx(unsigned i) const { | ||
| return i < getSymbolStartIdx(); | ||
| } | ||
| constexpr bool isSymbolIdx(unsigned i) const { | ||
| return i >= getSymbolStartIdx() && i < getLocalVarStartIdx(); | ||
| } | ||
| constexpr bool isLocalVarIdx(unsigned i) const { | ||
| return i >= getLocalVarStartIdx() && i < getConstantIdx(); | ||
| } | ||
| unsigned getNumVars() const { | ||
| return numDomain + numRange + numSymbols + numLocals; | ||
| constexpr bool isConstantIdx(unsigned i) const { | ||
| return i == getConstantIdx(); | ||
| } |
There was a problem hiding this comment.
A user should not assume any ordering of different variable types. This is an internal detail and should not be exposed. I know there are some methods that exist, but they are supposed to be depreciated in future. You shouldn't have to make any ordering assumptions. If you absolutely have to, then please reuse the existing methods below.
There was a problem hiding this comment.
Well, this "implementation detail" is used by CoefficientVector. Either I've got to have my own ParseInfo, or expose these methods in PresburgerSpace.
| @@ -173,6 +173,7 @@ class PresburgerSpace { | |||
| return PresburgerSpace(/*numDomain=*/0, /*numRange=*/numDims, numSymbols, | |||
| numLocals); | |||
| } | |||
| PresburgerSpace() = default; | |||
There was a problem hiding this comment.
There shouldn't be a default constructor for PresburgerSpace. PresburgerSpace has members which need to be initialized to maintain invariants (Identifiers). Please remove the default constructor.
There was a problem hiding this comment.
Well, I need the default constructor when I initialize ParserImpl. Again, either the existing ParseInfo was fine, or you let me uglify PresburgerSpace for the purposes of the parser.
| /// Parse an IntegerPolyhedron from a StringRef. | ||
| /// | ||
| /// integer-set ::= dim-and-symbol-id-lists `:` | ||
| /// '(' affine-constraint-conjunction? ')' | ||
| /// affine-constraint-conjunction ::= affine-constraint (`,` | ||
| /// affine-constraint)* | ||
| /// | ||
| IntegerPolyhedron parseIntegerPolyhedron(StringRef str); | ||
|
|
||
| /// Parse a MultiAffineFunction from a StringRef. | ||
| /// | ||
| /// affine-map ::= dim-and-symbol-id-lists `->` multi-dim-affine-expr | ||
| /// | ||
| /// multi-dim-affine-expr ::= `(` `)` | ||
| /// multi-dim-affine-expr ::= `(` affine-expr (`,` affine-expr)* `)` | ||
| MultiAffineFunction parseMultiAffineFunction(StringRef str); |
There was a problem hiding this comment.
I noticed a number of documentation comments in this patch that are directly copy pasted from various places in MLIR. Could you please check them and update them based on where they are used?
For example, here, affine-map and integer-set are a MLIR thing. If Presburger ends up moving in LLVM, a user has no clue what an affine-map is. Please instead use the relevant naming here.
|
@Groverkss Thank you for your time, but I don't think this constant back-and-forth is leading us anywhere. Can I kindly get a qualified reviewer to review this code? |
|
I'm sorry you felt this was a back-and-forth conversation leading nowhere. I would be very happy to see this patch land, and I thank you for all your work on this patch. We seem to have different ideas on how the parser should look, which is hard to understand based on review comments. I think the best way to move forward would be to have a call and strategise this better. That way, we can spend less time on reviews and make it easier for you to land things. I'll reach out to you offline to set up a call. |
Sounds good. I'll explain to you how the parser works. |
As far as I am aware, @Groverkss is the qualified reviewer for FPL code. I recommend you
Regarding a wholly new parser just for tests, my 2 cents is if you're really so inclined, then it should be confined/scoped to the tests rather than exposed as a redundant utility. |
|
I had some time to think about the parser, and here are my notes. The parser is a garden-variety recursive-descent parser with no lookahead. This means that all it can do when it encounters an operation token is to call the overload for the operation on lhs and rhs subexpressions. To take a simple parsing problem, consider: The parser sees x. What is x? Is it an IntegerRelation? struct IntegerRelation {
PresburgerSpace space;
IntMatrix equalities;
IntMatrix inequalities;
}If so, is it an equality and inequality? Is it a MultiAffineFunction? struct MultiAffineFunction {
PresburgerSpace space;
IntMatrix output;
DivisonRepr divs;
}If so, is x the output, or is it a div? Let's assume that it's an output, and let's continue parsing with this MultiAffineFunction that has an IntMatrix with one row and one column. We encounter 2, which we handle the same way as x, and x + 2 is a MultiAffineFunction with output having two columns and one row. Now, we encounter the floordiv. We now need to construct a fresh MultiAffineFunction with a fresh PresburgerSpace that has numDivs = 0 + 1. In the current parser, this is represented as: Much more efficient and less wasteful, complete with move semantics, and std::unique_ptr, so that there is only ever one root of the parse tree. Let us set aside the fact that building a MultiAffineFunction piece-wise is wasteful, and proceed to a more complicated example: x floordiv 2 and y floordiv 3 are MultiAffineFunctions with single divs in them. When you add them, let's assume that you extract the two divs, and create a fresh MultiAffineFunction with two divs (this is a vector with a bunch of numbers). Now, when we encounter floordiv 4, how do we disambiguate between this expression and: In both cases, all we have is a MultiAffineFunction with two divs, which are just a bunch of numbers. Digging this information out of the numbers is quite non-trivial, and we're working with matrices of numbers instead of first-class types. Moreover, consider the output itself in an exapanded example: When we parse x + y, the output is straightforward: {1, 1}. However, when we get to the div with a multiplication factor, it becomes more complicated, because the following numbers need to be appended: {0, 0, 2}. Basically, as we parse each nested div, we need to add 0 to the output (but how do we know that it's nested in a recursive descent parser?), and when we parse the outermost div, we need to add the multiplication factor 2 (again, how do we know that it's the outermost div?). It's not impossible though: when we parse the subexpression: We get a MultiAffineFunction with three divs, but it's non-trivial to figure out the nesting structure: all we have are a bunch of numbers, as opposed to first-class types in the current parser. This is why the current parser necessitates the use of a simple flattener that visits nested expressions, and fills in the correct output. The conclusion is that it's very difficult to parse directly into a MultiAffineFunction (although not impossible), and we'd be paining ourselves for no gain. Apart from the fact that it's wasteful, taking this approach would create more supporting code that inherits from MultiAffineFunction and extends the API. |
|
No engineer should be asked to make their code objectively worse. |
The Presburger library is already quite independent of MLIR, with the exception of the MLIR Support library. There is, however, one major exception: the test suite for the library depends on the core MLIR parser. To free it of this dependency, write a brand new parser for just pure affine expressions.
This patch is part of a project to move the Presburger library into LLVM.