- User Since
- Jun 28 2016, 7:50 PM (220 w, 5 d)
Feel free to directly make comment changes that you think are obvious :)
Fri, Sep 18
Please fix the clang format issues, but otherwise LGTM!
Tue, Sep 15
Sat, Sep 12
This patch looks generally good to me (but please fix the clang format issues if you can), but this is detailed enough that it would be best to find another reviewer to give you a full tech review, as I haven't been in this code for (too) many years :)
Great idea! However, I think it would be better to get feedback from other folks who are actively contributing to the loop optimizer. Thank you for pushing on this!
Nice. For "obvious" patches like this, feel free to commit after a day or two without feedback. The notion of what is "obvious" should be conservative, but documentation and other simple patches like this are good candidates. Thank you for working on this!
Thu, Sep 10
Thank you for working on this! I don't have cycles for a detailed review of the patch, but I'm thrilled to see this footgun get fixed.
Tue, Sep 8
Thanks for tidying this up and making it more consistent!
Very nice, thank you again for implementing this!
Thank you for writing this!
Thu, Sep 3
LGTM, thanks for doing this.
Wed, Sep 2
This patch LGTM. I think that conservatively init the members is a good idea.
Tue, Aug 25
Aug 18 2020
nice! thank you
Aug 16 2020
that was speedy, thanks!
Aug 15 2020
Makes sense - in a follow-on patch, it would be great to fix those up too, thanks!
This looks good to me, but please change this to "..." instead of "..". Also, in the documentation, please mention that this is an *inclusive* range. Thank you for working on this!
Aug 14 2020
Aug 13 2020
Yep, as long as you're committed to doing it, I'm perfectly fine with doing that in a followup patch. You might want to land this at one of the old URLs to make that easier though.
This is really fantastic work, thank you for doing this!
Aug 11 2020
Just saw this in the weekly newsletter - this is really really really cool, nice work!
Aug 9 2020
nice, thanks for cleaning this up!
Aug 7 2020
Aug 4 2020
Looks good, thanks!
Aug 3 2020
Yeah makes sense. One file per library should work fairly well in practice I think, given how .a files work. The only downside of this that I see is that the build for unit tests and other things in-tree that depend on the C API will be bottlenecked on building everything - this could add to the critical path of the build.
This looks really fantastic Alex, you really nailed the details on this! The docs look great, the choice of intptr_t is great, the naming structure and consistency is A+. Thank you!
Aug 2 2020
Thank you for renaming this!
Jul 30 2020
Jul 28 2020
Thank you Fangrui!
Jul 26 2020
Wow, yeah, static variables are wildly inappopriate for that. That said, I'm not familiar with the code, so someone else should review.
Jul 24 2020
Ok, then please set the pointer to null - that will cause things to explode if someone tries to use things after llvm_shutdown. Thanks!
I'm not sure how googles test runner works or how it differs. I recall many historical examples (in clang in particular), but it is possible they all got changed.
I don't see any reason to prefer a short name here, this isn't a simple too like 'opt' it takes command line flags etc.
Also, it is worth pointing out that we already have an (arguably much better) way to handle the problem you're trying to solve here:
Ok fair enough. I'd recommend a name like llvm-split-file or something like that given that we already have an llvm-extract tool that is very different.
Instead of adding an explicit command 'extract', did you consider building this into llvm-mc and similar tools? This is will lead to more efficient tests and scales better to things that have diagnostic verification and other sorts of checks. This is also more consistent with the precedent MLIR has set here.
Jul 20 2020
Jul 15 2020
Jul 14 2020
This looks great to me, thanks!
Jul 13 2020
Jul 10 2020
Please don't consider me a blocker on this patch, thank you for pushing on it!
Jul 8 2020
Does this add a static constructor?
Jul 7 2020
Jul 6 2020
Jul 5 2020
I'm not sure what the contention here is. int is a clearly inferior choice and is just as unprincipled as unsigned is, we should use the correct type.
Thank you for the feedback Stephen!
Changes suggested by Stephen Neuendorffer
Thanks! I'm going to let this simmer for a few days to give folks in the community time to react / improve.
This is the Google doc translated to RST format.
Jul 2 2020
Jul 1 2020
Jun 30 2020
This looks like a great direction, but please make sure to minimize public implementation details. We don't want the vast majority of instcombine to be visible outside of its library (it is hairy enough as it is :-)
Jun 28 2020
Ok, but instead of allowing "fine grained control", why not allow a "tasteful default" and "all" option? The "all" option seems perfectly reasonable for the weird case, and eliminates a big configuration matrix. This would just entail turning the command line option into a magic number / enum.
High level question - do we need additional command line flags here? Why not build this into the default actions that FileCheck takes? These detailed flags are unlikely to be used by people in practice, and this expands the surface area of FileCheck that needs to be maintained - it also expands the testing surface of FileCheck itself.
This is looking directionally great, please ping me after hubert.reinterpretcast's comments are reviewed and resolved. Thanks!
Jun 20 2020
Jun 19 2020
I'd recommend running this by @klimek
nice catch, thanks
Jun 18 2020
Jun 17 2020
Makes sense, thanks!
Jun 15 2020
No worries, thanks Erich!
Jun 5 2020
Jun 3 2020
May 31 2020
May 28 2020
LGTM, but I'd appreciate comments from at least one other reviewer. Thanks!
May 22 2020
May 21 2020
Please fix the lint warnings, but otherwise this LGTM as a step. Thank you for driving this!
May 19 2020
Very nice! Thank you for dusting off this old code
May 18 2020
Thank you for cleaning this up!
May 14 2020
This looks great, thank you!
May 7 2020
I agree, even though this is technically correct, the risk/reward isn't worth it. LLVM used to do this pervasively, but we switched to producing zero out of practical concerns.
May 6 2020
Awesome, thank you for changing this, I appreciate it!
May 4 2020
Would it make sense to call this "AffineScope". I don't believe we use the word "Polyhedral" anywhere. This was intentional because of the many preexisting notions of what "Polyhedral" means, but in any case we should be consistent.
May 2 2020
Very nice River! I incorporated this into my stuff and it sped up the parser by 18%!
Awesome, thank you for tackling this River, I added some suggestions above.
Apr 25 2020
Clang depended on the transitive include, update it!
Thank you for the review River!
Apr 23 2020
You can request commit access for yourself, just read the llvm developer policy for more info.
Apr 22 2020