This is an archive of the discontinued LLVM Phabricator instance.

mlir: Fix attribute parser errors for ui64

Authored by frej on Mar 20 2020, 6:53 AM.



The attribute parser fails to correctly parse unsigned 64 bit
attributes as the check `isNegative ? (int64_t)-val.getValue() >= 0
: (int64_t)val.getValue() < 0` will falsely detect an overflow for
unsigned values larger than 2^63-1.

This patch reworks the overflow logic to instead of doing arithmetic
on int64_t use APInt::isSignBitSet() and knowledge of the attribute

Test-cases which verify the de-facto behavior of the parser and
triggered the previous faulty handing of unsigned 64 bit attrbutes are
also added.

Diff Detail

Event Timeline

frej created this revision.Mar 20 2020, 6:53 AM
Herald added a reviewer: rriddle. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
rriddle requested changes to this revision.Mar 20 2020, 1:56 PM

Thanks for the fix!


Can you reflow this to be:

if (isNegative) {

  if (!apInt.isSignbitSet())
} else if (type.isSignedInteger() || type.isIndex()) && apInt.isSignBitSet()) {



Does the same logic apply here as well?


Please fix the now misaligned comments.


nit: Can you move all of these to one attribute dictionary to reduce the amount of boilerplate here?

214 ↗(On Diff #251622)

None of these are necessary for checking the parser.

This revision now requires changes to proceed.Mar 20 2020, 1:56 PM
frej added a comment.Mar 23 2020, 12:07 AM

Thanks for the review.


Will do.


As far as I can tell, this code appears to have the same problem. I'll move the range checking to its own method and call it from both places.


Will do


Do you mean that you want to see a single operator with multiple attributes?

My first (never sent upstream) version of this patch started out with a single operator looking more or less like the sketch below :

def AttrValueValidRangesOp : TEST_Op<"attr_valid_ranges"> {
  let arguments = (ins
    // for each <width> in 8,16,32,64
    AnyI<width>Attr:$i<width>_min, // check that  -(2^(<width>-1)) is accepted
    AnyI<width>Attr:$i<width>_max, // check that 2^(<width>-1)-1 is accepted
    AnyI<width>Attr:$i<width>_wrap_around, // check that 2^<width>-1 is accepted and interpreted as -1 (the correctness of this can be debated, but it is what the unmodified parser does)
    SI<width>Attr:$si<width>_min, // check that  -(2^(<width>-1)) is accepted
    SI<width>Attr:$si<width>_max, // check that 2^(<width>-1)-1 is accepted
    UI<width>Attr:$ui<width>_max // check that 2^(<width>)-1 is accepted

But then I started thinking about writing the test cases for detecting out of range attributes and discovered that for checking just a single attribute I would have to specify the other 23 attributes. This would greatly inflate the size of these tests as I can only check one attribute at a time. In my rush to get this submitted before the weekend I completely forgot about those kinds of tests. As far as I can tell the current test suite does not systematically test that overflows are detected, so I think that is something that I should add to be sure that I'm not failing to catch errors the existing parser does. I could of course use two operators, one with many attributes for testing valid values, and one with a single attribute for testing invalid values. Would that be palatable?

214 ↗(On Diff #251622)

Please explain. How can I check that values are valid for an attribute of a given width without having an operator with such an attribute? The best solution I can come up with is a single operator using an APInt attribute:

def APIntAttrValueOp : TEST_Op<"apint_attr"> {
  let arguments = (ins APIntAttr:$attr);

Would this be acceptable or did you have another solution in mind?

rriddle added inline comments.Mar 23 2020, 10:54 AM
214 ↗(On Diff #251622)

What I mean is; when you are testing error handling inside of the Parser/AsmPrinter you don't need to register any operations. MLIR doesn't require that you register operations. You could have foo.op and still trigger the necessary code paths inside of the parser/printer.

frej updated this revision to Diff 252306.Mar 24 2020, 7:06 AM

Incorporated River Riddle's review comments and improved the testing of out-of-range values.

frej marked 8 inline comments as done.Mar 24 2020, 7:09 AM
rriddle accepted this revision.Mar 24 2020, 11:55 AM

Looks great, I really appreciate the fix!

This revision is now accepted and ready to land.Mar 24 2020, 11:55 AM
frej added a comment.Mar 24 2020, 10:40 PM

@rriddle : Thanks, are you willing to do the commit as I don't have commit rights? I'd like the change to be attributed to "Frej Drejhammar <>" if possible.

This revision was automatically updated to reflect the committed changes.