- User Since
- Mar 12 2014, 3:00 PM (222 w, 6 d)
Thu, Jun 14
Wed, Jun 13
Why? (As in, please update the commit message to explain why)
A couple of very minor nitpicks about use of auto, below, but overall this looks very nice. LGTM.
Mon, Jun 11
Thu, Jun 7
This makes a lot of sense, especially given that the change to use SparseBitVector here instead of std::set specifically called out that this was to improve cases where there were dense register files. LGTM.
Wed, May 23
May 10 2018
May 4 2018
May 3 2018
Looks good, but please use update_llc_test_checks for the existing tests and move the test with the explanation to its own file
May 2 2018
Apr 30 2018
Apr 26 2018
Apr 10 2018
Would this be easier to follow if we overloaded printMemOperand with a variant that took the variables we're declaring here (M, MF, etc) and simply called into it? The "if (G)" condition would set them up and the "else" side would just pass null pointers.
Apr 4 2018
Okay. lg as an incremental improvement.
Apr 3 2018
Can we do better than just calling these namedVRegN? That is, is there a scheme where we could give these more meaningful names that would still be relatively stable between different input IRs?
Mar 30 2018
Mar 21 2018
Still looks good - two very nitpicky style comments below for when you commit.
Mar 14 2018
Nice improvement! I have a couple of minor style/clarity comments, but LGTM with those addressed.
Mar 13 2018
Mar 12 2018
Mar 8 2018
I've commented on both parts, but please keep each review to a single topic. Adding the test is one thing, and adding the helpers should be another review (or better yet, just add them when you use them).
Seems straightforward enough. LGTM.
Mar 7 2018
Mar 2 2018
One last comment, then I think this is good to go.
Feb 28 2018
Looks good, thanks
Feb 27 2018
Okay, I guess this makes sense for now. I think we should name the flag -disable-mir-legality-check instead of -disable-legality-check, as the current name is a bit too general sounding.
Feb 22 2018
Looks good, pending what we decide to do about the testing.
I like the idea of adding tests for update_mir_test_checks itself, but I think we should stick to focused tests rather than just adding one that tries to test everything. I also think this isn't a great place for it, maybe we could add a test/tools/update_checks folder for tests for all of the update_*_tests scripts? We might need to fake out llc to get away with something like that though...
Feb 21 2018
Feb 16 2018
Sure. I don't love the translations to underscores (it makes it a bit harder to map things) but I don't have a better idea offhand. Since this is starting to move towards "every pass" we should maybe consider doing something less hand rolled soon.
Feb 9 2018
Feb 8 2018
Feb 7 2018
Seems reasonable. Adding a couple of folks who know a bit more about debug info than I do to approve this.
Feb 5 2018
This seems like a bit of a hack compared to actually fixing the TODO and moving this check into the MachineVerifier where it belongs.
Seems reasonable. A couple of nitpicks.
Feb 2 2018
I think this is okay to go in, with the caveat that we need a plan for fixing up the verifier and removing this.
Jan 30 2018
I have mixed feelings about this, but I guess it's probably better than the status quo. My two main concerns are the time cost and whether we'll stop noticing the issues instead of fixing them.
Jan 27 2018
Jan 26 2018
This is looking pretty good. Let's get it in tree so people can start using it and we can iterate more based on feedback.
Jan 24 2018
I find the naming conventions for the matchers kind of weird, but I see that they're meant to be familiar to users of the IR level matcher APIs so I suppose they at least make sense. Let's get this in and start using it.
Jan 23 2018
LGTM as long as Quentin's convinced the second iteration makes sense.
Looks good. If people don't like the behaviour of mixing -func and -bb we can revisit later.
LGTM. I have a couple of style comments you can take or leave as you see fit.
Agreed, it doesn't hurt to have this in as is.
No complaints here. One minor question about the implementation.
Jan 22 2018
Jan 19 2018
Jan 18 2018
This is obvious enough that it didn't really need pre-commit review. Please commit.
As far as the diff goes it does look like we end up being much more verbose for simple cases (like, IMPLICIT_DEF), but OTOH the definitions we're adding are doing quite a bit more, so that's probably fine. I think the approach is reasonable.
Jan 17 2018
Roman asked me to commit this for him off-thread. It's r322805.
Jan 10 2018
Jan 9 2018
I don't really like how specific of a special case this is, but I guess that's more of a problem with swifterror itself than with this pass. LGTM.