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

[Builder] Add index from/to Float/Fixed/UFixed type casting #242

Merged
merged 2 commits into from
Nov 3, 2024

Conversation

zzzDavid
Copy link
Contributor

@zzzDavid zzzDavid commented Nov 1, 2024

Description

This PR introduces expanding support for casting between Index, Fixed, and UFixed types in the Allo IR. This enhancement allows for seamless conversions between these types, which broadens the flexibility of type handling in the IR layer.

Problems

Previously, the Allo IR lacked direct support for conversions between Index and Fixed/UFixed types, limiting the flexibility in scenarios where such conversions are needed for operations. Attempting these conversions required workarounds or was unsupported, creating friction in code requiring dynamic type handling.

Proposed Solutions

  • Added Index <-> Fixed/UFixed single-step conversions in the cast_map within ASTTransformer.
  • Implemented new casting operations IntToFixedOp and FixedToIntOp as intermediary conversions to facilitate direct type transformations.
  • Updated the IndexCastOp to handle Index -> Fixed/UFixed conversions.

Examples

Here’s a snippet of the updated behavior:

  1. Index to Fixed/UFixed Casting:

    def kernel(a: index) -> Fixed:
        a_fixed: Fixed = a
        return a_fixed
  2. Fixed/UFixed to Index Casting:

    def kernel(a: Fixed) -> index:
        a_idx: index = a
        return a_idx

Both conversions now produce correct IR output without errors, validating the new casting support.

Checklist

  • PR's title starts with a category (e.g., [IR], [Builder])
  • Changes are complete (i.e., finished coding on this PR)
  • All changes have test coverage, with cases for both Index -> Fixed/UFixed and Fixed/UFixed -> Index conversions
  • Code is well-documented

@zzzDavid zzzDavid requested a review from chhzh123 November 1, 2024 17:24
Copy link
Member

@chhzh123 chhzh123 left a comment

Choose a reason for hiding this comment

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

Mostly LGTM

@@ -42,6 +43,44 @@ def kernel(a: int32) -> float32:
assert mod(1) == kernel(1)


def test_index_fixed_casting():
def test_one_cast(fixed):
def kernel(a: fixed) -> float32:
Copy link
Member

Choose a reason for hiding this comment

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

where a is used here?

Comment on lines -370 to -372
# FP to Index is not supported in MLIR
# (Float, Index): RuntimeError,
# (Index, Float): RuntimeError,
Copy link
Member

Choose a reason for hiding this comment

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

Can you also add float<->index testing?

@zzzDavid
Copy link
Contributor Author

zzzDavid commented Nov 3, 2024

Thanks, I fixed the init value issue in the test case, and added two test cases for float <-> index casting

Copy link
Member

@chhzh123 chhzh123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@chhzh123 chhzh123 changed the title [Builder] Add index from/to Fixed/UFixed type casting (Fix #241) [Builder] Add index from/to Float/Fixed/UFixed type casting Nov 3, 2024
@chhzh123 chhzh123 merged commit d61322e into cornell-zhang:main Nov 3, 2024
1 check passed
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.

2 participants