- User Since
- Dec 20 2019, 9:16 PM (169 w, 5 d)
Mon, Mar 13
Sun, Mar 5
Sun, Feb 26
Nice, thanks! Fine to land as a stretch improvement but I think we need to further sugar the new exception class to avoid exposing implementation details.
Thanks. I had started reviewing this but got side tracked with the archaeology. Thanks for simplifying -- this was always not great.
Python changes lgtm. The parser changed make sense to but I usually defer those reviews to river.
Fri, Feb 24
Wed, Feb 22
Feb 10 2023
Feb 7 2023
Jan 27 2023
Given the security situation, I am fine making this change to tighten things up. It may cause some trouble for folks in the wild if they have concerning versions and pip doesn't offer great solutions. As part of a dev setup, though, I feel that folks should be able to self serve solutions to issues of there is a problem.
Jan 9 2023
Jan 6 2023
Perfect. Thanks for checking. Do you need help landing this patch?
Dec 28 2022
Thank you! Do you need help landing this patch or do you have commit access?
Dec 21 2022
Thank you Jacques/River -- I found this one in the field when only running release binaries on windows. It was not fun to get the repro that triggered it. Really glad we're continuing to improve reprocucers as they are a life saver.
Dec 16 2022
Dec 15 2022
Hi Renato - I've reviewed and this lgtm, but I rarely work on/review this part of the codebase. Do you feel like you are a good reviewer for it (the author reached out to me asking for some help coordinating reviewers)?
Dec 13 2022
Lgtm modulo trivial comments
Thanks. Do you have commit access? Or do you need help landing?
Dec 5 2022
Nov 27 2022
(change lgtm and I think we may want to rework this API at some point)
Thanks for having a look. I left a note on the issue about possibly doing this differently. WDYT? https://github.com/llvm/llvm-project/issues/59212#issuecomment-1328315420
Nov 19 2022
As mentioned, the duplication is annoying but I suspect is the least bad way to do it.
Nov 15 2022
Thanks - this is great!
Thanks for this. Patch lgtm. Has a couple of format issues but probably ok as-is (this file is very inconsistently formatted and it looks like you overrode clang-format a bit for consistency with adjacent code).
Nov 12 2022
Oct 26 2022
Oct 11 2022
Sorry for the delay - I attempted to rebase/land but the patch conflicts. I don't have time to actually update it right now - can you rebase/fix? I'll be back from traveling later in the week and can do the rebase/fix if you don't get to it but am not in a position to be making actual edits right now.
Another contributor cloned and submitted this with suggested fixes as a new patch: https://reviews.llvm.org/D135650
Oct 6 2022
Oct 4 2022
Ran the full clang/llvm/mlir test suite in debug mode just to be safe.
Remove break after llvm_unreachable for consistency with other switches in file.
Switch to explicit case per comment from ctopper.
Add fix to MicrosoftMangle.cpp that caused buildbot failure.
Oct 3 2022
I'm personally ok with this patch. If we decide to go forward with the index dialect, it will come with a transition plan for index-in-arith, and it doesn't seem like this makes that harder. If anything, it makes it easier to specify what the transition path is for the existing index cast ops.
Oct 2 2022
Sep 28 2022
Go ahead and land at your convenience. I'll land the other one once you do (busy and not going to get to it today)
Do you need help committing? The other one does - so if you both do, I can do final fix-ups as I'm landing both.
Also note https://reviews.llvm.org/D134812 in flight. I think that one should go in first
Also note https://reviews.llvm.org/D133450 in flight. I think yours should go in first
Good catch - thanks.
Bkramer; was your intention to review both the APFloat and MLIR side of the patch? Just making sure that the MLIR side had a proper look before submitting and it wasn't clear to me from the commentary.
Sep 26 2022
Sep 23 2022
Formatted and naming suggestions from RFC applied. Ready for review.
Format and apply rename suggestions.
Sep 20 2022
Oh, wow - not sure how that was missed. Thanks.
Sep 14 2022
Sep 11 2022
Sep 2 2022
Sep 1 2022
Personally, I'm +1 on cleaning things up but -1 that this is an actual improvement in consistency and legibility (based on a cursory review of the prevailing style). I'm with ftynse's interpretation, and while I could see the argument for removing the comma, I don't think that is an improvement either.
Aug 25 2022
Aug 19 2022
This looks good to me but I'd like to see the reviews of the other contributors to the conversation before landing.
Aug 17 2022
Thanks and sorry for missing this. Ditto to what Mehdi said about pinging for things that have direct impacts like this.
Aug 15 2022
Aug 8 2022
I'm ok/supportive of having the option in the test pass to run with canonicalizations mixed in: it isn't a bad idea to be and to test that they work together. But yeah, I'm not thrilled about coupling that in the actual populate method.
Aug 7 2022
I did add the canonicalization patterns to a downstream and have observed that they do "help" a lot of real cases I was seeing get to a fully decomposed state.
Aug 4 2022
Makes sense to me in theory. Is there a kind of case we can test for that needs this?
Aug 1 2022
FYI - Some intervening changes rendered this patch at least partially invalid and I never parsed back through and redid it. Consider it for inspirational value if anyone wants to pick it up.
Jul 27 2022
Jul 21 2022
Good catch, and thank you!
Jul 19 2022
Do you have commit access?
Jul 18 2022
Sorry, yes - I submitted a follow-up and neglected to note on this thread: https://reviews.llvm.org/rGbeebffa9ab81bba3de91b2cee7a62578ebe1ae00
Jul 16 2022
Rebase, fix standalone example, attempt to patch bazel as best I could.
Jul 15 2022
Closed by https://reviews.llvm.org/rG1d6a90418e4bfc294b4174880e93cb43835ebdf5 (messed up the arcanist command and association was lost)
Note that this is associated with https://reviews.llvm.org/D129604 (messed up the arcanist command and association was lost)