Skip to content
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] [NFC] Use APFloat semantics to get floating type width #107372

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

sergey-kozub
Copy link
Contributor

As suggested in the comments of #105573

@sergey-kozub sergey-kozub self-assigned this Sep 5, 2024
@llvmbot llvmbot added mlir:core MLIR Core Infrastructure mlir labels Sep 5, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 5, 2024

@llvm/pr-subscribers-mlir-core

@llvm/pr-subscribers-mlir

Author: Sergey Kozub (sergey-kozub)

Changes

As suggested in the comments of #105573


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

1 Files Affected:

  • (modified) mlir/lib/IR/BuiltinTypes.cpp (+2-14)
diff --git a/mlir/lib/IR/BuiltinTypes.cpp b/mlir/lib/IR/BuiltinTypes.cpp
index 16b53efa55fb80..58f83333b00c20 100644
--- a/mlir/lib/IR/BuiltinTypes.cpp
+++ b/mlir/lib/IR/BuiltinTypes.cpp
@@ -91,21 +91,9 @@ IntegerType IntegerType::scaleElementBitwidth(unsigned scale) {
 //===----------------------------------------------------------------------===//
 
 unsigned FloatType::getWidth() {
-  if (llvm::isa<Float8E5M2Type, Float8E4M3Type, Float8E4M3FNType,
-                Float8E5M2FNUZType, Float8E4M3FNUZType, Float8E4M3B11FNUZType,
-                Float8E3M4Type>(*this))
-    return 8;
-  if (llvm::isa<Float16Type, BFloat16Type>(*this))
-    return 16;
-  if (llvm::isa<Float32Type, FloatTF32Type>(*this))
+  if (llvm::isa<FloatTF32Type>(*this))
     return 32;
-  if (llvm::isa<Float64Type>(*this))
-    return 64;
-  if (llvm::isa<Float80Type>(*this))
-    return 80;
-  if (llvm::isa<Float128Type>(*this))
-    return 128;
-  llvm_unreachable("unexpected float type");
+  return getFloatSemantics().sizeInBits;
 }
 
 /// Returns the floating semantics for the given type.

if (llvm::isa<Float16Type, BFloat16Type>(*this))
return 16;
if (llvm::isa<Float32Type, FloatTF32Type>(*this))
if (llvm::isa<FloatTF32Type>(*this))
return 32;
Copy link
Member

@apivovarov apivovarov Sep 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add a comment noting that the actual width of TF32 is 19 bits. However, since it is a truncated version of Float32, we treat it as 32 bits in MLIR FloatType::getWidth for compatibility.

@jfurtek, @jpienaar , @River707 What are your thoughts on this approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the comment, thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine I think, this is NFC right? (please edit the PR title)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't have a problem with this either

@sergey-kozub sergey-kozub changed the title [MLIR] Use APFloat semantics to get floating type width [MLIR] [NFC] Use APFloat semantics to get floating type width Sep 10, 2024
@sergey-kozub sergey-kozub merged commit 083e25c into llvm:main Sep 10, 2024
5 of 6 checks passed
VitaNuo pushed a commit to VitaNuo/llvm-project that referenced this pull request Sep 12, 2024
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.

6 participants