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

[FXML-4448] Add EmitC lowering for arith.{trunci,extsi,extui} #177

Merged
merged 9 commits into from
May 8, 2024

Conversation

cferry-AMD
Copy link
Collaborator

This PR introduces EmitC lowering for various integer casts in the Arith dialect.

Per C++ Reference:

  • If the destination type is unsigned, the resulting value is the smallest unsigned value equal to the source value modulo 2^n where n is the number of bits used to represent the destination type. That is, depending on whether the destination type is wider or narrower, signed integers are sign-extended or truncated and unsigned integers are zero-extended or truncated respectively.

  • If the destination type is signed, the value does not change if the source integer can be represented in the destination type. Otherwise the result is

    • implementation-defined (until C++20)
    • the unique value of the destination type equal to the source value modulo 2^n where n is the number of bits used to represent the destination type(since C++20)
      (note that this is different from signed integer arithmetic overflow, which is undefined).

So, truncation is only guaranteed if unsigned casts are performed, or C++20 is used. This implementation sticks to unsigned where we know it is a truncation, like trunci, and index_cast with greater source width than destination. The only issue is when using index types (that don't have a bitwidth) and C++20 isn't used -- the resulting behavior is then implementation-defined.

@cferry-AMD cferry-AMD requested review from mgehre-amd and josel-amd May 6, 2024 15:16
Copy link
Collaborator

@josel-amd josel-amd left a comment

Choose a reason for hiding this comment

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

Small comments here and there

mlir/lib/Conversion/ArithToEmitC/ArithToEmitC.cpp Outdated Show resolved Hide resolved
mlir/lib/Conversion/ArithToEmitC/ArithToEmitC.cpp Outdated Show resolved Hide resolved
@mgehre-amd
Copy link
Collaborator

How about removing the index casts from here and open a separate PR for them once the size_t/ssize_t types are available?

@cferry-AMD
Copy link
Collaborator Author

@mgehre-amd, that's a good way forward.

Copy link
Collaborator

@mgehre-amd mgehre-amd left a comment

Choose a reason for hiding this comment

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

Looks good!

@josel-amd josel-amd self-requested a review May 8, 2024 11:03
@cferry-AMD cferry-AMD changed the title [FXML-4448] Add EmitC lowering for arith.{trunci,extsi,extui,index_cast,index_castui} [FXML-4448] Add EmitC lowering for arith.{trunci,extsi,extui} May 8, 2024
@cferry-AMD cferry-AMD merged commit 22a9433 into feature/fused-ops May 8, 2024
3 checks passed
@cferry-AMD cferry-AMD deleted the corentin.arith_integer_ops branch May 8, 2024 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants