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

Delete MLIR-AIE submodule #639

Merged
merged 12 commits into from
Aug 8, 2024
Merged

Delete MLIR-AIE submodule #639

merged 12 commits into from
Aug 8, 2024

Conversation

makslevental
Copy link
Collaborator

@makslevental makslevental commented Aug 3, 2024

This PR delete/removes the MLIR-AIE submodule. The load-bearing change is forking all of the AIEOps.td type things. The main reason for doing this now is Xilinx/mlir-aie#1660, which does not fix the parser issue that broke us but makes it permanent.

In performing this fork we take the most minimal set of auxiliary/support impls/function/code. Specifically, we take none of their verifiers. The reason is, as usual, 1) we don't intend to continue to have most/many of these ops 2) they're now not an egress boundary i.e., they're purely "compiler state" ops/IR. Naturally, whichever ops we keep or iterate on, we should have verifiers for but I prefer 1) to add them incrementally rather than eagerly 2) add them in a separate PR (or PRs).

A couple things to call out:

  1. The tests do not change (modulo i32 -> i8). This is an indication that the fork/port/vendoring is successful;
    • This change, which brings in closer alignment with aie-rt, incurs that cost that uint8_ts need to printed with std::to_string1;
  2. The prior vendored passes incur some minor changes because in doing the fork I removed all superfluous/redundant/confusing getters2;
  3. Rather than adding patches to https://github.com/nod-ai/mlir-air I add a few local seds. I did it this way because the patches would break an isolated build there3. Not the prettiest thing but I'll keep up with it.

Note, there's a still bit to do (I want to rename the xilinx::AIE namespace and then use typedefs) but this is ready to review.

On second thought let's merge this and then move all the ops/stuff to our mlir::iree_compiler::AMDAIE namespace in a follow-up.

Fixes #430

Footnotes

  1. Otherwise you print the ASCII character at the position rather the number itself.

  2. Stuff like getIDInt when the existing attr getter is getID.

  3. Which never happens sure but just seems like it sprawls the hackiness rather than containing it to one place.

@makslevental makslevental marked this pull request as draft August 3, 2024 00:44
@makslevental makslevental force-pushed the makslevental/fork-aie-ops branch 27 times, most recently from cc5c31d to 1185ec7 Compare August 7, 2024 06:15
@makslevental makslevental changed the title [wip] fork mlir-aie ops Delete MLIR-AIE submodule Aug 7, 2024
@makslevental makslevental force-pushed the makslevental/fork-aie-ops branch from 1185ec7 to b7cfe18 Compare August 7, 2024 06:37
@makslevental makslevental marked this pull request as ready for review August 7, 2024 19:01
@makslevental makslevental force-pushed the makslevental/fork-aie-ops branch 2 times, most recently from 501bd33 to ba87aef Compare August 7, 2024 21:12
@makslevental makslevental force-pushed the makslevental/fork-aie-ops branch from ba87aef to 9aa42ab Compare August 7, 2024 21:15
Copy link
Collaborator

@jtuyls jtuyls left a comment

Choose a reason for hiding this comment

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

LGTM

@makslevental
Copy link
Collaborator Author

@nirvedhmeshram @newling @Abhishek-Varma you guys want to take a look or you're good with this?

Copy link
Contributor

@nirvedhmeshram nirvedhmeshram left a comment

Choose a reason for hiding this comment

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

LGTM, just a minor question but you can address however and land.

@makslevental makslevental force-pushed the makslevental/fork-aie-ops branch from b591cd4 to 550ceda Compare August 8, 2024 03:06
@makslevental makslevental enabled auto-merge (squash) August 8, 2024 03:07
@makslevental makslevental merged commit 91a13bb into main Aug 8, 2024
2 checks passed
@makslevental makslevental deleted the makslevental/fork-aie-ops branch August 8, 2024 03:15
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.

[Tracking] Refactor MLIR-AIE dependency
3 participants