- User Since
- Aug 20 2014, 6:06 PM (449 w, 2 d)
Wed, Mar 29
LG, but please add rationale in both description and comment in code.
It seems like your latest update removed the functional change (it just shows me the formatting change now)
LG to me, and optionally done via populate, I'm not sure if always good idea vs letting lower level codegen handle it (seems direct). But given not supported on all backends, SGTM.
Nice (sorry forgot to hit submit)
Mon, Mar 27
Fri, Mar 24
I see on the screenshot for unknown dialects it treats everything as unknown (e.g., memref not identified). That's more conservative (only show what you know) but might be worse for highlighting. Do you know if this is intrinsic to tree-sitter or the way the grammar is written? (It was written directly matching our markdown/uses to fix markdown, with explicit structure in mind).
Nice speedup! Sorry I missed the review notification
Thu, Mar 23
Wed, Mar 22
Tue, Mar 21
Looks good thanks
Wed, Mar 15
Update to move common helpers to base & document defs/classes more.
Mon, Mar 13
I don't think you've selected reviewers yet (Nicolas seems unlikely as being selected, perhaps some overly broad Herald rule).
Sun, Mar 12
Sat, Mar 11
Remember to clean up commit message
Fri, Mar 10
Thu, Mar 9
Wed, Mar 8
(which effectively means no not canonical, but one has sections with clear expectations and post "deployment" profile verification there should be none of these for consumers)
Tue, Mar 7
Should this be renamed now? E.g., reifyResultShapes now only sometimes reify. (I think folks are also reaching here for something closer to what onnx-mlir and others are doing)
Could this be done only if detected interactive session? E.g., when piping commands I'd prefer not to see this message (which is probably the majority of times when I use opt).
I was wondering (post review) if we should split the reader and writer commit parts, so that we could give a bit of time for bytecode consumers to get updated first (thinking of projects that span multiple repos). I mean, it is unstable at the moment, but wouldn't cause any additional churn.
Nice! I'm fine with delaying textual form.
Sun, Mar 5
Sat, Mar 4
Updated post discussion to immediately use it to replace builtin dialect
reader/writer instead of starting more separate. Also bootstrapping was to
clarify unstable nature, but was called out to be too emphasized.
Fri, Mar 3
Thu, Mar 2
Mar 1 2023
Could you add a test to show both usages/avoid regressions?
Feb 28 2023
Is there some paper or tool or approximation method that we could cite here?
Could we just have it in its own group/approximations pass? (Well and we can populate with patterns from polynomial if needed). Just feels wrong to have this in polynomial
Feb 27 2023
(started a scan but realized I wasn't really reading with any detail ...)
Nit: commit message. The rest looks fine (I forget if we allowed default values in OpBuilder ... )
Feb 25 2023
Feb 24 2023
Feb 22 2023
When I did the first version I was using this as I wanted to run some analysis tool that uses tree-sitter (that didn't work well) and also to check the language guide a bit using the error checks in tree-sitter generation process. Generation would be really nice, and from maintenance point of view (and we know these may go out of sync, so it would be best effort). Could you perhaps show the before and after of how it affects highlighting? Esp with an unsupported dialect too, to speak to the fuzzing matching part.
Feb 17 2023
Feb 14 2023
Feb 10 2023
I'll help in a bit
Feb 9 2023
Feb 7 2023
Nice, thanks. So this doesn't remove and just flips the default, so folks who haven't yet updated can just set raw explicit for now (and then fixup quickly before option is deprecated).
Feb 6 2023
Sending quick fix.
I'm confused, I thought these were going the other way. E.g., there is a mismatch between what the spec calls an attribute and MLIR does, previously we had mostly err'd on treating them the same. But as the spec really requires this before execute/offload rather than representationaly , I thought the conclusion was that these should be operands and one had like a verify for export/runtime pass to flag that the operands are actually constants at that point.
Feb 2 2023
Jan 24 2023
Let me know when ready and I'll take a look (I didn't check failure)
Jan 23 2023
Nice! Given this is very exact lowering required for numerics the test is fine, else I'd be conserned too much of change test. (when I see such lowering I keep thinking I want a couple of convenience wrappers ...)
Looks good, as Mehdi mentioned using Fixes #60069 will result in closing the issue upon commit.
I'm actually surprised we didn't have this one, thanks, let know if you need help landing.
Nice, thanks. Would you need help in landing?
Jan 21 2023
Nice graduation (gets us even a step towards introspection ... Well more pieces needed :-)). Have you thought about exposing these C side?
Jan 19 2023
Ah seems I never hit submit yesterday. Same comment as Alex on one and general question on verification part.
Jan 17 2023
SG, could you add a TODO here documenting this so that we know when we look at it again?
[not a statement of endorsement of either form]
Could you perhaps expand on that exception or point to where this is assumed? Are these where during a pass an invalid state is created or is this across passes?
Jan 16 2023
The change was originally motivated by avoiding the breakage when result type inference was being introduced. It has landed a while ago, so this change will now create the breakage it sought to avoid.