Feel free to directly make comment changes that you think are obvious :)

Please fix the clang format issues, but otherwise LGTM!

Very nice!

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!

nice

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.

Thanks for tidying this up and making it more consistent!

Very nice, thank you again for implementing this!

Thank you for writing this!

LGTM, thanks for doing this.

This patch LGTM. I think that conservatively init the members is a good idea.

nice! thank you

that was speedy, thanks!

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!

Hi Paul,

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!

Just saw this in the weekly newsletter - this is really really really cool, nice work!

nice, thanks for cleaning this up!

Looks good, thanks!

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!

Thank you for renaming this!

Thank you Fangrui!

Hi All,

Wow, yeah, static variables are wildly inappopriate for that. That said, I'm not familiar with the code, so someone else should review.

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.

Some thoughts:

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.

This looks great to me, thanks!

Thanks

Please don't consider me a blocker on this patch, thank you for pushing on it!

Thanks!

Does this add a static constructor?

In D82594#2131999, @mehdi_amini wrote:

In D82594#2131989, @lattner wrote:

int is a clearly inferior choice

It isn't clear to me, as I mention in my previous comment, I'd be happy to get more educated on this if you have resources you could point me to that would explain your point?

In D82594#2131998, @MaskRay wrote:

In D82594#2131989, @lattner wrote:

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.

I get the sense that you're arguing for ssize_t or one of its analogues instead of size_t? Are you arguing for general principles or this specific case? In this specific case, using size_t seems completely reasonable, but I agree that ssize_t would also be fine.

I'm ok with either size_t or ssize_t here, but if you want to turn this into a general principle that we apply to the code base, then it should be discussed on llvm-dev first.

ssize_t is inferior. POSIX.1-2017 says "The type ssize_t shall be capable of storing values at least in the range [-1, {SSIZE_MAX}]." The implementation may not even support -2.

We could also use intptr_t to avoid sign extension on a 64-bit machine.

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.

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 :-)

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!

Hi Dave,

I'd recommend running this by @klimek

nice catch, thanks

Makes sense, thanks!

No worries, thanks Erich!

nice catch

LGTM, but I'd appreciate comments from at least one other reviewer. Thanks!

Please fix the lint warnings, but otherwise this LGTM as a step. Thank you for driving this!

Very nice! Thank you for dusting off this old code

Thank you for cleaning this up!

This looks great, thank you!

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.

Awesome, thank you for changing this, I appreciate it!

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.

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.

Clang depended on the transitive include, update it!

Thank you for the review River!

You can request commit access for yourself, just read the llvm developer policy for more info.

thanks!