-
Notifications
You must be signed in to change notification settings - Fork 11.3k
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
mlir/Presburger: contribute a free-standing parser #94916
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -173,6 +173,7 @@ class PresburgerSpace { | |
return PresburgerSpace(/*numDomain=*/0, /*numRange=*/numDims, numSymbols, | ||
numLocals); | ||
} | ||
PresburgerSpace() = default; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
/// Get the domain/range space of this space. The returned space is a set | ||
/// space. | ||
|
@@ -182,18 +183,43 @@ class PresburgerSpace { | |
/// Get the space without local variables. | ||
PresburgerSpace getSpaceWithoutLocals() const; | ||
|
||
unsigned getNumDomainVars() const { return numDomain; } | ||
unsigned getNumRangeVars() const { return numRange; } | ||
unsigned getNumSetDimVars() const { return numRange; } | ||
unsigned getNumSymbolVars() const { return numSymbols; } | ||
unsigned getNumLocalVars() const { return numLocals; } | ||
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(); | ||
Comment on lines
+186
to
+198
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Those methods are insufficient, as I directly set the variable to the value in some places. |
||
} | ||
constexpr unsigned getNumVars() const { | ||
return getNumDimAndSymbolVars() + getNumLocalVars(); | ||
} | ||
constexpr unsigned getNumCols() const { return getNumVars() + 1; } | ||
|
||
constexpr unsigned getSetDimStartIdx() const { return 0; } | ||
constexpr unsigned getSymbolStartIdx() const { return getNumDimVars(); } | ||
constexpr unsigned getLocalVarStartIdx() const { | ||
return getNumDimAndSymbolVars(); | ||
} | ||
constexpr unsigned getConstantIdx() const { return getNumCols() - 1; } | ||
|
||
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(); | ||
} | ||
Comment on lines
-191
to
223
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, this "implementation detail" is used by CoefficientVector. Either I've got to have my own ParseInfo, or expose these methods in PresburgerSpace. |
||
|
||
/// Get the number of vars of the specified kind. | ||
|
@@ -321,18 +347,18 @@ class PresburgerSpace { | |
|
||
private: | ||
// Number of variables corresponding to domain variables. | ||
unsigned numDomain; | ||
unsigned numDomain = 0; | ||
|
||
// Number of variables corresponding to range variables. | ||
unsigned numRange; | ||
unsigned numRange = 0; | ||
|
||
/// Number of variables corresponding to symbols (unknown but constant for | ||
/// analysis). | ||
unsigned numSymbols; | ||
unsigned numSymbols = 0; | ||
|
||
/// Number of variables corresponding to locals (variables corresponding | ||
/// to existentially quantified variables). | ||
unsigned numLocals; | ||
unsigned numLocals = 0; | ||
|
||
/// Stores whether or not identifiers are being used in this space. | ||
bool usingIds = false; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
add_subdirectory(Parser) | ||
|
||
add_mlir_library(MLIRPresburger | ||
Barvinok.cpp | ||
IntegerRelation.cpp | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
add_mlir_library(MLIRPresburgerParser | ||
Flattener.cpp | ||
Lexer.cpp | ||
ParserImpl.cpp | ||
ParseStructs.cpp | ||
Token.cpp) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
//===- 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 "ParseStructs.h" | ||
|
||
using namespace mlir; | ||
using namespace presburger; | ||
|
||
void Flattener::visitDiv(const PureAffineExprImpl &div) { | ||
int64_t divisor = div.getDivisor(); | ||
|
||
// First construct the linear part of the divisor. | ||
auto dividend = div.collectLinearTerms().getPadded( | ||
space.getLocalVarStartIdx() + localExprs.size() + 1); | ||
|
||
// Next, insert the non-linear coefficients. | ||
for (const auto &[hash, adjustedMulFactor, adjustedLinearTerm] : | ||
div.getNonLinearCoeffs()) { | ||
|
||
// adjustedMulFactor will be mulFactor * -divisor in case of mod, and | ||
// mulFactor in case of floordiv. | ||
dividend[lookupLocal(hash)] = adjustedMulFactor; | ||
|
||
// The expansion is either a modExpansion which we previously stored, or the | ||
// adjustedLinearTerm, which is correct in the case when we're encountering | ||
// the innermost mod for the first time. | ||
CoefficientVector expansion = | ||
lookupModExpansion(hash) | ||
.value_or(adjustedLinearTerm) | ||
.getPadded(space.getLocalVarStartIdx() + localExprs.size() + 1); | ||
dividend += expansion; | ||
|
||
// If this is a mod, insert the new computed expansion, which is the | ||
// dividend * mulFactor. | ||
if (div.isMod()) | ||
localModExpansion.insert({div.hash(), dividend * div.getMulFactor()}); | ||
} | ||
|
||
cst.addLocalFloorDiv(dividend, divisor); | ||
localExprs.insert(div.hash()); | ||
} | ||
|
||
void Flattener::flatten(unsigned row, PureAffineExprImpl &div) { | ||
// Visit divs inner to outer. | ||
for (auto &nestedDiv : div.getNestedDivTerms()) | ||
flatten(row, *nestedDiv); | ||
|
||
if (div.hasDivisor()) { | ||
visitDiv(div); | ||
return; | ||
} | ||
|
||
// Hit multiple times every time we have a linear sub-expression, but the | ||
// row is overwritten to consider only the outermost div, which is hit | ||
// last. | ||
|
||
// Set the linear part of the row. | ||
setRow(row, div.getLinearDividend()); | ||
|
||
// Set the non-linear coefficients. | ||
for (const auto &[hash, adjustedMulFactor, adjustedLinearTerm] : | ||
div.getNonLinearCoeffs()) { | ||
flatMatrix(row, lookupLocal(hash)) = adjustedMulFactor; | ||
CoefficientVector expansion = | ||
lookupModExpansion(hash).value_or(adjustedLinearTerm); | ||
addToRow(row, expansion); | ||
} | ||
} | ||
|
||
std::pair<IntMatrix, IntegerPolyhedron> Flattener::flatten() { | ||
// Use the same flattener to simplify each expression successively. This way | ||
// local variables / expressions are shared. | ||
for (const auto &[row, expr] : enumerate(exprs)) | ||
flatten(row, *expr); | ||
|
||
return {flatMatrix, cst}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
//===- Flattener.h - 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 | ||
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
#ifndef MLIR_ANALYSIS_PRESBURGER_PARSER_FLATTENER_H | ||
#define MLIR_ANALYSIS_PRESBURGER_PARSER_FLATTENER_H | ||
|
||
#include "ParseStructs.h" | ||
#include "mlir/Analysis/Presburger/IntegerRelation.h" | ||
#include "llvm/ADT/DenseMap.h" | ||
#include "llvm/ADT/SetVector.h" | ||
|
||
namespace mlir::presburger { | ||
using llvm::SmallDenseMap; | ||
using llvm::SmallSetVector; | ||
|
||
class Flattener : public FinalParseResult { | ||
public: | ||
// The final flattened result is stored here. | ||
IntMatrix flatMatrix; | ||
|
||
// We maintain a set of divs that we have seen while flattening. The size of | ||
// this set is at most info.numDivs, hitting info.numDivs at the end of the | ||
// flattening, if that expression contains all the possible divs. | ||
SmallSetVector<size_t, 4> localExprs; | ||
|
||
// We maintain a mapping between local mods and their expansions. The vector | ||
// is the dividend. | ||
SmallDenseMap<size_t, CoefficientVector, 2> localModExpansion; | ||
|
||
Flattener(FinalParseResult &&parseResult) | ||
: FinalParseResult(std::move(parseResult)), | ||
flatMatrix(exprs.size(), space.getNumCols()) {} | ||
|
||
std::pair<IntMatrix, IntegerPolyhedron> flatten(); | ||
|
||
private: | ||
void flatten(unsigned row, PureAffineExprImpl &div); | ||
void visitDiv(const PureAffineExprImpl &div); | ||
|
||
void addToRow(unsigned row, const CoefficientVector &l) { | ||
flatMatrix.addToRow(row, | ||
getDynamicAPIntVec(l.getPadded(space.getNumCols()))); | ||
} | ||
void setRow(unsigned row, const CoefficientVector &l) { | ||
flatMatrix.setRow(row, getDynamicAPIntVec(l.getPadded(space.getNumCols()))); | ||
} | ||
|
||
unsigned lookupLocal(size_t hash) { | ||
const auto *it = find(localExprs, hash); | ||
assert(it != localExprs.end() && | ||
"Local expression not found; walking from inner to outer?"); | ||
return space.getLocalVarStartIdx() + it - localExprs.begin(); | ||
} | ||
std::optional<CoefficientVector> lookupModExpansion(size_t hash) { | ||
return localModExpansion.contains(hash) | ||
? std::make_optional(localModExpansion.at(hash)) | ||
artagnon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
: std::nullopt; | ||
artagnon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
}; | ||
} // namespace mlir::presburger | ||
|
||
#endif // MLIR_ANALYSIS_PRESBURGER_PARSER_FLATTENER_H |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
andinteger-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.