[mlir] add debug messages explaining failure reasons of isValidIntOrFloat

[j2kun]

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.

[llvmbot]

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-core

Author: Jeremy Kun (j2kun)

Changes

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.

---

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

1 Files Affected:

• (modified) mlir/lib/IR/BuiltinAttributes.cpp (+34-8)

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 { 

[github-actions]

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

[joker-eph]

Can we add to the assertion a message hinting the user to re-run with -debug-only=builtinattributes ?

[j2kun]

Can we add to the assertion a message hinting the user to re-run with -debug-only=builtinattributes ?

TIL this is a thing. Added!

[joker-eph]

Thanks, will merge after CI is passing

[joker-eph]

Actually your GitHub setting would lead to j2kun@users.noreply.github.com used for the commit, can you fix this?

[j2kun]

Done.

[joker-eph]

I still see this:

I can however checkout the PR and push the commit manually.

[joker-eph]

Pushed in 04d6308

[MacDue]

I still see this:

I can however checkout the PR and push the commit manually.

FYI: You need to uncheck "Keep my email addresses private" (under Settings > Emails)

[j2kun]

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: ***@***.***>

[joker-eph]

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.
When committing a patch for someone on Phabricator I would ask them which name/email they want to associate to the commit.

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
It'll help us figuring out what we should do about this...