Skip to content

[SYCL-MLIR][NFC] Restructure Polygeist dialect #8266

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Feb 15, 2023
Merged

[SYCL-MLIR][NFC] Restructure Polygeist dialect #8266

merged 6 commits into from
Feb 15, 2023

Conversation

victor-eds
Copy link
Contributor

Apply following changes to the dialect to follow MLIR dialects conventions:

  • Change headers locations:
    • Dialect definition: "mlir/Dialect/Polygeist/IR/"
    • Transform passes: "mlir/Dialect/Polygeist/Transforms/"
    • Conversion passes: "mlir/Conversion/PolygeistTo/"
    • Dialect utils: "mlir/Dialect/Polygeist/Utils/.h"
  • Create a library for each conversion pass.
  • Conversion passes are defined under the mlir namespace.

This commit does not intend to be a complete refactoring of the dialect. This is a work in progress, not intending to refactor any code.

Signed-off-by: Victor Perez [email protected]

@victor-eds victor-eds added the sycl-mlir Pull requests or issues for sycl-mlir branch label Feb 9, 2023
@victor-eds victor-eds requested a review from etiotto as a code owner February 9, 2023 12:31
@victor-eds victor-eds self-assigned this Feb 9, 2023
@whitneywhtsang
Copy link
Contributor

Nothing changed from my last review, forget to submit the reply or forget to push?

@victor-eds
Copy link
Contributor Author

Nothing changed from my last review, forget to submit the reply or forget to push?

No, I just wanted to continue the discussion on your prev. comment.

@whitneywhtsang
Copy link
Contributor

Nothing changed from my last review, forget to submit the reply or forget to push?

No, I just wanted to continue the discussion on your prev. comment.

Did you reply on my previous comment?

victor-eds
victor-eds commented Feb 13, 2023
View reviewed changes
@victor-eds
Copy link
Contributor Author

Did you reply on my previous comment?

Did now

@victor-eds
[SYCL-MLIR][NFC] Restructure Polygeist dialect
f003f06 
Apply following changes to the dialect to follow MLIR dialects
conventions:

- Change headers locations:
  - Dialect definition: "mlir/Dialect/Polygeist/IR"
  - Transform passes: "mlir/Dialect/Polygeist/Transforms"
  - Conversion passes: "mlir/Conversion/PolygeistTo<dialect>"
  - Dialect utils: "mlir/Dialect/Polygeist/Utils/<file>.h"
- Create a library for each conversion pass.

Note: This commit does not intend to be a complete refactoring of the
dialect.

Signed-off-by: Victor Perez <[email protected]>
@victor-eds
Add license
9ac9d0e
@victor-eds
Fix guard
eaaf449
@victor-eds
Clang format
b7b9d9e
@victor-eds
Drop includes
7b58909
@victor-eds victor-eds force-pushed the polygeist-full-refactor branch from 61cd2c8 to 7b58909 Compare February 14, 2023 08:34
@victor-eds victor-eds merged commit a1f1990 into intel:sycl-mlir Feb 15, 2023
@victor-eds victor-eds deleted the polygeist-full-refactor branch February 15, 2023 11:59
@etiotto
Copy link

etiotto commented Feb 15, 2023

This PR was merged prematurely. It has only one approval (we agreed that large PRs require 2) and it was also merged before passing the LLVM test suite tests.

@etiotto
Copy link

etiotto commented Feb 15, 2023

Also it turns out (at least in my build) this PR caused a link issue. If I remove commit "5d81e3095c0d" then I can link the sycl-mlir branch fine. I configure the compiler to build shared libraries.

etiotto added a commit that referenced this pull request Feb 15, 2023
@etiotto
Revert "[SYCL-MLIR][NFC] Restructure Polygeist dialect (#8266)"
e38ed6c 
This reverts commit a1f1990.
@etiotto etiotto mentioned this pull request Feb 15, 2023
Merged
etiotto added a commit that referenced this pull request Feb 15, 2023
@etiotto
Revert "[SYCL-MLIR][NFC] Restructure Polygeist dialect" (#8356)
0391711 
Reverts #8266
@victor-eds
Copy link
Contributor Author

This PR was merged prematurely. It has only one approval (we agreed that large PRs require 2) and it was also merged before passing the LLVM test suite tests.

LLVM test suite tests were failing because we are missing some changes upstream. I checked the tests that were failing (I added/modified some of them upstream). Build was passing, but it uses static libs.

Also it turns out (at least in my build) this PR caused a link issue. If I remove commit "5d81e3095c0d" then I can link the sycl-mlir branch fine. I configure the compiler to build shared libraries.

Yep, I tried with shared libs and also failed. Pushing a fixed vers.

@etiotto
Copy link

etiotto commented Feb 15, 2023

@victor-eds I reverted this PR to get private builds going. I suggest we redo this PR and add the fix for shared lib you mentioned.

@victor-eds
Copy link
Contributor Author

@victor-eds I reverted this PR to get private builds going. I suggest we redo this PR and add the fix for shared lib you mentioned.

It's already up for review #8357

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whitneywhtsang whitneywhtsang whitneywhtsang approved these changes

@etiotto etiotto Awaiting requested review from etiotto

Assignees

@victor-eds victor-eds

Labels
sycl-mlir Pull requests or issues for sycl-mlir branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@victor-eds @whitneywhtsang @etiotto