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] add debug messages explaining failure reasons of isValidIntOrFloat #69476
Conversation
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-core Author: Jeremy Kun (j2kun) ChangesI have run into assertion failures quite often when calling this method via Full diff: https://github.com/llvm/llvm-project/pull/69476.diff 1 Files Affected:
diff --git a/mlir/lib/IR/BuiltinAttributes.cpp b/mlir/lib/IR/BuiltinAttributes.cpp
index 64949a17107297a..4b18c8c34ae6962 100644
--- a/mlir/lib/IR/BuiltinAttributes.cpp
+++ b/mlir/lib/IR/BuiltinAttributes.cpp
@@ -20,9 +20,12 @@
#include "llvm/ADT/APSInt.h"
#include "llvm/ADT/Sequence.h"
#include "llvm/ADT/TypeSwitch.h"
+#include "llvm/Support/Debug.h"
#include "llvm/Support/Endian.h"
#include <optional>
+#define DEBUG_TYPE "builtinattributes"
+
using namespace mlir;
using namespace mlir::detail;
@@ -1098,24 +1101,45 @@ bool DenseElementsAttr::isValidRawBuffer(ShapedType type,
static bool isValidIntOrFloat(Type type, int64_t dataEltSize, bool isInt,
bool isSigned) {
// Make sure that the data element size is the same as the type element width.
- if (getDenseElementBitWidth(type) !=
- static_cast<size_t>(dataEltSize * CHAR_BIT))
+ auto denseEltBitWidth = getDenseElementBitWidth(type);
+ auto dataSize = static_cast<size_t>(dataEltSize * CHAR_BIT);
+ if (denseEltBitWidth != dataSize) {
+ LLVM_DEBUG(llvm::dbgs() << "expected dense element bit width "
+ << denseEltBitWidth << " to match data size "
+ << dataSize << " for type " << type << "\n");
return false;
+ }
// Check that the element type is either float or integer or index.
- if (!isInt)
- return llvm::isa<FloatType>(type);
+ if (!isInt) {
+ bool valid = llvm::isa<FloatType>(type);
+ if (!valid)
+ LLVM_DEBUG(llvm::dbgs()
+ << "expected float type when isInt is false, but found "
+ << type << "\n");
+ return valid;
+ }
if (type.isIndex())
return true;
auto intType = llvm::dyn_cast<IntegerType>(type);
- if (!intType)
+ if (!intType) {
+ LLVM_DEBUG(llvm::dbgs()
+ << "expected integer type when isInt is true, but found "
+ << type << "\n");
return false;
+ }
// Make sure signedness semantics is consistent.
if (intType.isSignless())
return true;
- return intType.isSigned() ? isSigned : !isSigned;
+
+ bool valid = intType.isSigned() == isSigned;
+ if (!valid)
+ LLVM_DEBUG(llvm::dbgs()
+ << "expected signedness " << isSigned << " to match type "
+ << type << "\n");
+ return valid;
}
/// Defaults down the subclass implementation.
@@ -1247,12 +1271,14 @@ DenseElementsAttr DenseElementsAttr::bitcast(Type newElType) {
DenseElementsAttr
DenseElementsAttr::mapValues(Type newElementType,
function_ref<APInt(const APInt &)> mapping) const {
- return llvm::cast<DenseIntElementsAttr>(*this).mapValues(newElementType, mapping);
+ return llvm::cast<DenseIntElementsAttr>(*this).mapValues(newElementType,
+ mapping);
}
DenseElementsAttr DenseElementsAttr::mapValues(
Type newElementType, function_ref<APInt(const APFloat &)> mapping) const {
- return llvm::cast<DenseFPElementsAttr>(*this).mapValues(newElementType, mapping);
+ return llvm::cast<DenseFPElementsAttr>(*this).mapValues(newElementType,
+ mapping);
}
ShapedType DenseElementsAttr::getType() const {
|
1de2c48
to
ff09ac0
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
ff09ac0
to
b667268
Compare
Can we add to the assertion a message hinting the user to re-run with |
TIL this is a thing. Added! |
Thanks, will merge after CI is passing |
Actually your GitHub setting would lead to |
I have run into assertion failures quite often when calling this method via `DenseElementsAttr::get`, and I think this would help, at the very least, by printing out the bit width size mismatches, rather than a plain assertion failure. I included all the other cases in the method for completeness
b7d0e12
to
13632a6
Compare
Done. |
Pushed in 04d6308 |
Ah, I just amended the commit. I do prefer to keep you email private when
possible. Is there a reason to not to that for MLIR?
…On Thu, Oct 19, 2023, 8:45 AM Benjamin Maxwell ***@***.***> wrote:
I still see this:
[image: Screenshot 2023-10-18 at 4 11 22 PM]
<https://user-images.githubusercontent.com/3372300/276430453-2ae03f13-8c90-49f9-9876-fb99a47bb5e6.png>
I can however checkout the PR and push the commit manually.
FYI: You need to uncheck "Keep my email addresses private" (under Settings
> Emails)
—
Reply to this email directly, view it on GitHub
<#69476 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAS2PKTAP4CGLP7EY5SC4ADYAFDK7AVCNFSM6AAAAAA6FXHZNSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONZRGI2TQNBXHA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
It is a bit new for us to have to deal with "private emails", in the git log every commit has an author with name/email traditionally. If you feel that we should allow this GitHub feature, can you comment on it here? https://discourse.llvm.org/t/hidden-emails-on-github-should-we-do-something-about-it/74223 |
I have run into assertion failures quite often when calling this method via
DenseElementsAttr::get
, and I think this would help, at the very least, by printing out the bit width size mismatches, rather than a non-descript assertion failure. I included debug messages for all the other cases in the method for completeness.