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

Only import the LTC backend that's used in example models #939

Merged

Conversation

henrytwo
Copy link
Member

Fixing namespace resolution issue from importing both TS and MLIR libraries into example models. Now we only import the library that we're going to use, based on the user's selection of LTC backend.

cc: @ke1337 @antoniojkim @silvasean

This PR has already been reviewed internally within Cerebras, so we will be merging immediately.

@henrytwo henrytwo self-assigned this Jun 14, 2022
@henrytwo henrytwo merged commit fbf7782 into llvm:torch_mlir_ltc_backend Jun 14, 2022
@henrytwo henrytwo deleted the henrytu/fix_example_ltc_imports branch June 14, 2022 16:09
@stellaraccident
Copy link
Collaborator

Thanks for the patch! Minor nit on development style in llvm projects: since this is a community project, we try to not rely on offline review/communication within a company and keep reviews out in the open. For a trivial/nfc (non functional change), it is ok to land directly without review. If there is any question, you will find that in the main repo, people will explicitly ask for a "post submit review" from particular people when doing this (as opposed to saying it was reviewed off thread in private).

Not trying to slow you down -- just providing some wording guidance for the future so this project aligns more with the overall development policy.

In this case, the patch LGTM.

@henrytwo
Copy link
Member Author

Thanks for the patch! Minor nit on development style in llvm projects: since this is a community project, we try to not rely on offline review/communication within a company and keep reviews out in the open. For a trivial/nfc (non functional change), it is ok to land directly without review. If there is any question, you will find that in the main repo, people will explicitly ask for a "post submit review" from particular people when doing this (as opposed to saying it was reviewed off thread in private).

Not trying to slow you down -- just providing some wording guidance for the future so this project aligns more with the overall development policy.

In this case, the patch LGTM.

Got it. @silvasean mentioned that we could iterate internally on this Discord thread, so that's why I went ahead and merged it in directly -- but if I'm misunderstanding something please let me know for future reference :)

@antoniojkim
Copy link
Collaborator

I think @silvasean was okay with us doing this for now because this is a staging branch. We're very close to being in a place where we can merge in the staging branch into main. Once we're working off of main instead of the torch_mlir_ltc_backend staging branch, we'll for sure make sure to get proper reviews from the community

@silvasean
Copy link
Contributor

@henrytwo @antoniojkim -- sorry for the confusion. I meant reviewing amongst each other, but still doing the whole process out in the open here on github. We want everyone in the community to have the equitable opportunity to chime in on the development work.

What I was referencing on Discord was merely my personal role in these reviews, which I think is adequately covered by you folks' expertise for most of the PR's. The pattern I was seeing is that you folks were getting unnecessarily blocked on my reviews and I wanted to streamline that process.

@silvasean
Copy link
Contributor

If it is a large burden to move more of the review onto github I think we can be pragmatic and let's talk about solutions. I know it is "just" public perception, but having PR's like this that feel "dumped" upstream without a community development process (not saying that was your intention -- just that it can be perceived that way) really has a negative impact on the project and attracting contributors.

henrytwo added a commit that referenced this pull request Jul 8, 2022
henrytwo added a commit that referenced this pull request Jul 8, 2022
henrytwo added a commit that referenced this pull request Jul 12, 2022
henrytwo added a commit that referenced this pull request Jul 29, 2022
henrytwo added a commit that referenced this pull request Jul 29, 2022
henrytwo added a commit that referenced this pull request Jul 30, 2022
qedawkins pushed a commit to nod-ai/torch-mlir that referenced this pull request Oct 3, 2022
Signed-off-by: Alexandre Eichenberger <alexe@us.ibm.com>
qedawkins pushed a commit to nod-ai/torch-mlir that referenced this pull request Oct 3, 2022
* factored affine builder out of krnltoaffine

Signed-off-by: Alexandre Eichenberger <alexe@us.ibm.com>

* edited to reduce conflict when adding new ops (llvm#939)

Signed-off-by: Alexandre Eichenberger <alexe@us.ibm.com>

* migrated AffineBuilder to MLIRDialectBuilder to be more usable in other phases than just KrnlToAffine

Signed-off-by: Alexandre Eichenberger <alexe@us.ibm.com>
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.

4 participants