Skip to content

[MLIR] Fix generic assembly syntax for ArrayAttr containing hex float #94583

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
merged 1 commit into from
Jun 6, 2024

Conversation

joker-eph
Copy link
Collaborator

When a float attribute is printed with Hex, we should not elide the type because it is parsed back as i64 otherwise.

@joker-eph joker-eph requested review from jpienaar and Mogball June 6, 2024 07:11
@llvmbot llvmbot added mlir:core MLIR Core Infrastructure mlir labels Jun 6, 2024
@llvmbot
Copy link
Member

llvmbot commented Jun 6, 2024

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-core

Author: Mehdi Amini (joker-eph)

Changes

When a float attribute is printed with Hex, we should not elide the type because it is parsed back as i64 otherwise.


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

2 Files Affected:

  • (modified) mlir/lib/IR/AsmPrinter.cpp (+6-3)
  • (modified) mlir/test/IR/array-of-attr.mlir (+3)
diff --git a/mlir/lib/IR/AsmPrinter.cpp b/mlir/lib/IR/AsmPrinter.cpp
index 29e36210f1270..ba47283e37bee 100644
--- a/mlir/lib/IR/AsmPrinter.cpp
+++ b/mlir/lib/IR/AsmPrinter.cpp
@@ -2061,7 +2061,7 @@ void AsmPrinter::Impl::printLocationInternal(LocationAttr loc, bool pretty,
 
 /// Print a floating point value in a way that the parser will be able to
 /// round-trip losslessly.
-static void printFloatValue(const APFloat &apValue, raw_ostream &os) {
+static void printFloatValue(const APFloat &apValue, raw_ostream &os, bool *printedHex = nullptr) {
   // We would like to output the FP constant value in exponential notation,
   // but we cannot do this if doing so will lose precision.  Check here to
   // make sure that we only output it in exponential format if we can parse
@@ -2102,6 +2102,8 @@ static void printFloatValue(const APFloat &apValue, raw_ostream &os) {
 
   // Print special values in hexadecimal format. The sign bit should be included
   // in the literal.
+  if (printedHex)
+    *printedHex = true;
   SmallVector<char, 16> str;
   APInt apInt = apValue.bitcastToAPInt();
   apInt.toString(str, /*Radix=*/16, /*Signed=*/false,
@@ -2275,10 +2277,11 @@ void AsmPrinter::Impl::printAttributeImpl(Attribute attr,
       return;
 
   } else if (auto floatAttr = llvm::dyn_cast<FloatAttr>(attr)) {
-    printFloatValue(floatAttr.getValue(), os);
+    bool printedHex = false;
+    printFloatValue(floatAttr.getValue(), os, &printedHex);
 
     // FloatAttr elides the type if F64.
-    if (typeElision == AttrTypeElision::May && floatAttr.getType().isF64())
+    if (typeElision == AttrTypeElision::May && floatAttr.getType().isF64() && !printedHex)
       return;
 
   } else if (auto strAttr = llvm::dyn_cast<StringAttr>(attr)) {
diff --git a/mlir/test/IR/array-of-attr.mlir b/mlir/test/IR/array-of-attr.mlir
index 1b6fe55205959..c64a02f230372 100644
--- a/mlir/test/IR/array-of-attr.mlir
+++ b/mlir/test/IR/array-of-attr.mlir
@@ -12,3 +12,6 @@ test.array_of_attr_op
 // CHECK: test.array_of_attr_op
 // CHECK-SAME: a = [], b = [], c = []
 test.array_of_attr_op a = [], b = [], c = []
+
+// CHECK-SAME: 1.000000e+00 : f32, 1.000000e+00, 0x7FF0000000000000 : f64
+"test.test"() {test.float_arr = [1.0 : f32, 1.0 : f64, 0x7FF0000000000000 : f64]} : () -> ()

Copy link

github-actions bot commented Jun 6, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

When a float attribute is printed with Hex, we should not elide the type
because it is parsed back as i64 otherwise.
@joker-eph joker-eph merged commit 2df68e0 into llvm:main Jun 6, 2024
7 checks passed
@joker-eph joker-eph deleted the fix_printer branch June 6, 2024 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mlir:core MLIR Core Infrastructure mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants