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] Add DenseUI32ArrayAttr #68230

Closed
wants to merge 0 commits into from
Closed

[mlir] Add DenseUI32ArrayAttr #68230

wants to merge 0 commits into from

Conversation

amrami
Copy link
Contributor

@amrami amrami commented Oct 4, 2023

No description provided.

@llvmbot llvmbot added mlir:core MLIR Core Infrastructure mlir mlir:ods labels Oct 4, 2023
@amrami amrami self-assigned this Oct 9, 2023
@amrami amrami closed this Oct 9, 2023
@amrami amrami reopened this Oct 9, 2023
@amrami amrami requested a review from joker-eph October 9, 2023 10:33
@joker-eph
Copy link
Collaborator

Can you add a motivation to this PR?

This was not included very intentionally, so we should really carefully look into whether we want to add it now.

@amrami
Copy link
Contributor Author

amrami commented Oct 10, 2023

@joker-eph - I have a op with permutation attribute. Since this is a permutation of the tensor dims, I know that values are >= 0.
So I find it more accurate to declare the attribute as DenseUI32ArrayAttr.

I looked now at the existing code, and saw that linalg TransposeOp uses DenseI64ArrayAttr:$permutation.
Then the verifier validates that no negative numbers are there:

LogicalResult TransposeOp::verify() {
  ArrayRef<int64_t> permutationRef = getPermutation();

  if (!isPermutationVector(permutationRef))
    return emitOpError("permutation is not valid");
...

Where

bool mlir::isPermutationVector(ArrayRef<int64_t> interchange) {
  assert(llvm::all_of(interchange, [](int64_t s) { return s >= 0; }) &&
         "permutation must be non-negative");
  llvm::SmallDenseSet<int64_t, 4> seenVals;
  for (auto val : interchange) {
    if (seenVals.count(val))
      return false;
    seenVals.insert(val);
  }
  return seenVals.size() == interchange.size();
}

I believe the permutation could be DenseUI64ArrayAttr (which can be added too)

@joker-eph
Copy link
Collaborator

Using "unsigned" to model "positive quantities" is a common C++ mistake, to the point where multiple coding guidelines are actively discouraging to use unsigned integer.

A lot has been written about this, good references are [unsigned: A Guideline for Better Code] https://www.youtube.com/watch?v=wvtFGa6XJDU) and Garbage In, Garbage Out: Arguing about Undefined Behavior..., as well as this panel discussion:

Other coding guidelines already embrace this:

It is rare that overflowing (and wrapping) an unsigned integer won't trigger a program bug when the overflow was not intentionally handled. Using signed arithmetic means that you can actually trap on over/underflow and catch these bugs (when using fuzzing for instance).

Chandler explained this nicely in his CPPCon 2016 talk "Garbage In, Garbage Out: Arguing about Undefined Behavior...". I encourage to watch the full talk but here is one relevant example: https://youtu.be/yG1OZ69H_-o?t=2006 , and here he mentions how Google experimented with this internally: https://youtu.be/yG1OZ69H_-o?t=2249

Unsigned integer also have a discontinuity right to the left of zero. Suppose A, B and C are small positive integers close to zero, say all less than a hundred or so. Then given: A + B > C
and knowing elementary school algebra, one can rewrite that as: A > B - C
But C might be greater than B, and the subtraction would produce some huge number. This happens even when working with seemingly harmless numbers like A=2, B=2, and C=3.

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:ods mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants