-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add linalg -> mlir::LLVM Dialect conversion pass #1559
Conversation
f36bad4
to
5272da5
Compare
23ec6c4
to
49eefb0
Compare
@vwellsTT I think this PR needs to be opened against main if you want CI to run. |
Hmm, imo that makes the diff confusing when I open 3 subsequent PRs like this. I guess I could cherry-pick this onto main, and in this case perhaps it doesn't actually depend on the previous branch...but I'd sort of prefer to just do this and wait for the previous branch to merge before running CI personally |
5272da5
to
faf1bb4
Compare
c204ee8
to
8550c24
Compare
So, we could do a preemptive soft review, but there are no processes/requirements for reviewing PRs that are not going into main. That is, we'll have to review this PR again (officially) once it is requested against main. For the sake of reviewing churn it's probably best to open the PRs that can be rebased on main against main. For changes that are dependent on each other we'll have to open those PRs serially. |
Fair enough; in this case, I do think the PRs are probably pretty
separable, so tomorrow I’ll change this to be a standalone cherry-pick then
…On Tue, Dec 10, 2024 at 17:27 Nick Smith ***@***.***> wrote:
So, we could do a preemptive soft review, but there are no
processes/requirements for reviewing PRs that are not going into main. That
is, we'll have to review this PR again when it is requested against main.
For the sake of reviewing churn it's probably best to open the PRs that
can be rebased on main against main. For changes that are dependent on each
other we'll have to open those PRs serially.
—
Reply to this email directly, view it on GitHub
<#1559 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BM4GWP5EUBDNO444K3KIDPT2E52HNAVCNFSM6AAAAABTMABTFGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMZTGIYTANZXGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
43a8c7e
to
f38eccc
Compare
@vwellsTT, this looks great, couple of things we should add:
|
Sounds good; I'll make a quick template for all of these PRs like you describe. For the test, I may have some follow-up questions, but seems like a good idea |
// TODO: we might want some more options to say lower through affine loops | ||
// instead of scf directly, etc. which could be new options | ||
Option<bool> cleanupOutputEnabled{ | ||
*this, "enable-remove-dead-values", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename "enable-remove-dead-values" to something more meaningful? As I understand this option will enable canonicalize, SCC, CSE, SymbolDCE, not just remove-dead-value pass...
723f79a
to
47f7be7
Compare
47f7be7
to
bc75536
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments to address inline, otherwise looks great, thanks!
// TODO (vwells): we might want some more options to say lower through affine | ||
// loops instead of scf directly, etc. which could be new options | ||
Option<bool> cleanupOutputEnabled{ | ||
*this, "enable-post-pipeline-cleanup", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I somewhat preferred the remove dead values name since that's effectively what most of these passes are doing. That said, perhaps the name enable-optimization-passes
might be more suitable? This current name is a bit vague.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I personally agree that remove-dead-values
seemed like a reasonable summary, but don't feel super strongly. I think enable-optimization-passes
is fine too, I'll switch to that
eb8168d
to
191fea2
Compare
277aa4e
to
0459bda
Compare
0459bda
to
8e69e5a
Compare
This reverts commit e748e71.
Goal: The end-to-end goal is to integrate a path to compile and execute specific ops or sets of ops on the CPU.
Context:
The entire task will be split into (tentatively) 7 PRs, as follows:
This PR represents the 3rd PR above. Here, we build a pipeline from existing passes to lower linalg Dialect into LLVM Dialect so that we can proceed to compile into an executable .so in later stages
Example
Input:
Output: