Add AsmParser::parseDecimalInteger. #96255
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-mlir-core @llvm/pr-subscribers-mlir Author: None (quartersdg) ChangesAn attribute parser needs to parse lists of possibly negative integers separated by x in a way which is foiled by parseInteger handling hex formats and parseIntegerInDimensionList does not allow negatives. Full diff: https://github.com/llvm/llvm-project/pull/96255.diff 4 Files Affected:
diff --git a/mlir/include/mlir/IR/OpImplementation.h b/mlir/include/mlir/IR/OpImplementation.h
index fa435cb3155ed..ae412c7227f8e 100644
--- a/mlir/include/mlir/IR/OpImplementation.h
+++ b/mlir/include/mlir/IR/OpImplementation.h
@@ -714,16 +714,27 @@ class AsmParser {
return *parseResult;
}
+ /// Parse a decimal integer value from the stream.
+ template <typename IntT>
+ ParseResult parseDecimalInteger(IntT &result) {
+ auto loc = getCurrentLocation();
+ OptionalParseResult parseResult = parseOptionalDecimalInteger(result);
+ if (!parseResult.has_value())
+ return emitError(loc, "expected decimal integer value");
+ return *parseResult;
+ }
+
/// Parse an optional integer value from the stream.
virtual OptionalParseResult parseOptionalInteger(APInt &result) = 0;
+ virtual OptionalParseResult parseOptionalDecimalInteger(APInt &result) = 0;
- template <typename IntT>
- OptionalParseResult parseOptionalInteger(IntT &result) {
+ private:
+ template <typename IntT, typename ParseFn>
+ OptionalParseResult parseOptionalIntegerAndCheck(IntT &result,
+ ParseFn &&parseFn) {
auto loc = getCurrentLocation();
-
- // Parse the unsigned variant.
APInt uintResult;
- OptionalParseResult parseResult = parseOptionalInteger(uintResult);
+ OptionalParseResult parseResult = parseFn(uintResult);
if (!parseResult.has_value() || failed(*parseResult))
return parseResult;
@@ -737,6 +748,20 @@ class AsmParser {
return success();
}
+ public:
+ template <typename IntT>
+ OptionalParseResult parseOptionalInteger(IntT &result) {
+ return parseOptionalIntegerAndCheck(
+ result, [&](APInt &result) { return parseOptionalInteger(result); });
+ }
+
+ template <typename IntT>
+ OptionalParseResult parseOptionalDecimalInteger(IntT &result) {
+ return parseOptionalIntegerAndCheck(result, [&](APInt &result) {
+ return parseOptionalDecimalInteger(result);
+ });
+ }
+
/// These are the supported delimiters around operand lists and region
/// argument lists, used by parseOperandList.
enum class Delimiter {
diff --git a/mlir/lib/AsmParser/AsmParserImpl.h b/mlir/lib/AsmParser/AsmParserImpl.h
index 8f22be80865bf..b12687833e3fd 100644
--- a/mlir/lib/AsmParser/AsmParserImpl.h
+++ b/mlir/lib/AsmParser/AsmParserImpl.h
@@ -322,6 +322,11 @@ class AsmParserImpl : public BaseT {
return parser.parseOptionalInteger(result);
}
+ /// Parse an optional integer value from the stream.
+ OptionalParseResult parseOptionalDecimalInteger(APInt &result) override {
+ return parser.parseOptionalDecimalInteger(result);
+ }
+
/// Parse a list of comma-separated items with an optional delimiter. If a
/// delimiter is provided, then an empty list is allowed. If not, then at
/// least one element will be parsed.
diff --git a/mlir/lib/AsmParser/Parser.cpp b/mlir/lib/AsmParser/Parser.cpp
index 1b8b4bac1821e..48aabb0163464 100644
--- a/mlir/lib/AsmParser/Parser.cpp
+++ b/mlir/lib/AsmParser/Parser.cpp
@@ -42,6 +42,7 @@
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/ScopeExit.h"
#include "llvm/ADT/Sequence.h"
+#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringSet.h"
#include "llvm/Support/Alignment.h"
@@ -308,6 +309,51 @@ OptionalParseResult Parser::parseOptionalInteger(APInt &result) {
return success();
}
+namespace {
+bool isBinOrHexOrOctIndicator(char c) {
+ return (llvm::toLower(c) == 'x' || llvm::toLower(c) == 'b' ||
+ llvm::isDigit(c));
+}
+} // namespace
+
+/// Parse an optional integer value only in decimal format from the stream.
+OptionalParseResult Parser::parseOptionalDecimalInteger(APInt &result) {
+ Token curToken = getToken();
+ if (curToken.isNot(Token::integer, Token::minus)) {
+ return std::nullopt;
+ }
+
+ bool negative = consumeIf(Token::minus);
+ Token curTok = getToken();
+ if (parseToken(Token::integer, "expected integer value")) {
+ return failure();
+ }
+
+ StringRef spelling = curTok.getSpelling();
+ // If the integer is in bin, hex, or oct format, return only the 0 and reset
+ // the lex pointer.
+ if (spelling[0] == '0' && spelling.size() > 1 &&
+ isBinOrHexOrOctIndicator(spelling[1])) {
+ result = 0;
+ state.lex.resetPointer(spelling.data() + 1);
+ consumeToken();
+ return success();
+ }
+
+ if (spelling.getAsInteger(10, result))
+ return emitError(curTok.getLoc(), "integer value too large");
+
+ // Make sure we have a zero at the top so we return the right signedness.
+ if (result.isNegative())
+ result = result.zext(result.getBitWidth() + 1);
+
+ // Process the negative sign if present.
+ if (negative)
+ result.negate();
+
+ return success();
+}
+
/// Parse a floating point value from an integer literal token.
ParseResult Parser::parseFloatFromIntegerLiteral(
std::optional<APFloat> &result, const Token &tok, bool isNegative,
diff --git a/mlir/lib/AsmParser/Parser.h b/mlir/lib/AsmParser/Parser.h
index b959e67b8e258..4caab499e1a0e 100644
--- a/mlir/lib/AsmParser/Parser.h
+++ b/mlir/lib/AsmParser/Parser.h
@@ -144,6 +144,9 @@ class Parser {
/// Parse an optional integer value from the stream.
OptionalParseResult parseOptionalInteger(APInt &result);
+ /// Parse an optional integer value only in decimal format from the stream.
+ OptionalParseResult parseOptionalDecimalInteger(APInt &result);
+
/// Parse a floating point value from an integer literal token.
ParseResult parseFloatFromIntegerLiteral(std::optional<APFloat> &result,
const Token &tok, bool isNegative,
|
mlir/lib/AsmParser/Parser.cpp
Outdated
| @@ -308,6 +309,51 @@ OptionalParseResult Parser::parseOptionalInteger(APInt &result) { | |||
| return success(); | |||
| } | |||
|
|
|||
| namespace { | |||
| bool isBinOrHexOrOctIndicator(char c) { | |||
| return (llvm::toLower(c) == 'x' || llvm::toLower(c) == 'b' || | |||
There was a problem hiding this comment.
Looking at the lexer, it seems like only hex is supported there. I think if you did a binary one, it would not be returned as a single integer.
mlir/lib/AsmParser/Parser.cpp
Outdated
|
|
||
| StringRef spelling = curTok.getSpelling(); | ||
| // If the integer is in bin, hex, or oct format, return only the 0 and reset | ||
| // the lex pointer. |
There was a problem hiding this comment.
Could you expand this to explain the resetPointer below? (it follows if one looks at the implementation of the lexer, but in isolation here not so much)
| if (failed(*parser.parseOptionalDecimalInteger(intVal))) { | ||
| return Attribute(); | ||
| } | ||
| if (parser.parseGreater()) { |
There was a problem hiding this comment.
Lets parse x and a (say) here too, that way you have a valid hex number and you verify that the reset worked as expected.
There was a problem hiding this comment.
Actually 5 instead of a means you can reuse test below but as a single positive one.
There was a problem hiding this comment.
I reworked it to look more like a shape like the dialect this is going to fix later
An attribute parser needs to parse lists of possibly negative integers separated by x in a way which is foiled by parseInteger handling hex format and parseIntegerInDimensionList does not allow negatives.
jpienaar
force-pushed
the
parse-decimal-integer
branch
from
4571152 to
318c013
Compare
You can test this locally with the following command:git-clang-format --diff 56535a090d91ff10a60c884bacbd314dcf9659db 9731617570dc47eadffeedc2e8f2a6cfa5c0d673 --extensions cpp,h -- mlir/include/mlir/IR/OpImplementation.h mlir/lib/AsmParser/AsmParserImpl.h mlir/lib/AsmParser/Parser.cpp mlir/lib/AsmParser/Parser.h mlir/test/lib/Dialect/Test/TestAttributes.cppView the diff from clang-format here.diff --git a/mlir/include/mlir/IR/OpImplementation.h b/mlir/include/mlir/IR/OpImplementation.h
index ae412c7227..3d0064f703 100644
--- a/mlir/include/mlir/IR/OpImplementation.h
+++ b/mlir/include/mlir/IR/OpImplementation.h
@@ -728,7 +728,7 @@ public:
virtual OptionalParseResult parseOptionalInteger(APInt &result) = 0;
virtual OptionalParseResult parseOptionalDecimalInteger(APInt &result) = 0;
- private:
+private:
template <typename IntT, typename ParseFn>
OptionalParseResult parseOptionalIntegerAndCheck(IntT &result,
ParseFn &&parseFn) {
@@ -748,7 +748,7 @@ public:
return success();
}
- public:
+public:
template <typename IntT>
OptionalParseResult parseOptionalInteger(IntT &result) {
return parseOptionalIntegerAndCheck(
diff --git a/mlir/test/lib/Dialect/Test/TestAttributes.cpp b/mlir/test/lib/Dialect/Test/TestAttributes.cpp
index e09ea10906..e29fb5befe 100644
--- a/mlir/test/lib/Dialect/Test/TestAttributes.cpp
+++ b/mlir/test/lib/Dialect/Test/TestAttributes.cpp
@@ -67,7 +67,7 @@ void CompoundAAttr::print(AsmPrinter &printer) const {
//===----------------------------------------------------------------------===//
Attribute TestDecimalShapeAttr::parse(AsmParser &parser, Type type) {
- if (parser.parseLess()){
+ if (parser.parseLess()) {
return Attribute();
}
SmallVector<int64_t> shape;
|
|
@quartersdg Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested Please check whether problems have been caused by your change specifically, as How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
Summary: An attribute parser needs to parse lists of possibly negative integers separated by x in a way which is foiled by parseInteger handling hex formats and parseIntegerInDimensionList does not allow negatives. --------- Co-authored-by: Jacques Pienaar <[email protected]> Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250672
An attribute parser needs to parse lists of possibly negative integers separated by x in a way which is foiled by parseInteger handling hex formats and parseIntegerInDimensionList does not allow negatives.