- User Since
- Mar 12 2014, 3:00 PM (274 w, 4 d)
Maybe I'm missing something, but the override implemented in D63204 looks like a subset of the default TII::getRegClass that falls through to TRI::getRegClass, just like the default does. Is this change actually necessary?
Wed, Jun 12
Sorry for the delay! This LGTM
Wed, Jun 5
So if I'm reading this right this just basically just delays TLI->finalizeLowering to the end of ExpandISelPseudos/FinalizeISel. Is there motivation to this other than it makes more logical sense to freeze registers as close to regalloc as possible?
Thu, May 30
Seems straightforward enough
For future reference, obvious follow ups like this are fine for post-commit review
Wed, May 29
lgtm once you've addressed compnerd's comments
Thu, May 23
This looks great. Looking forward to faster incremental builds!
Apr 25 2019
Apr 22 2019
Apr 14 2019
Should we (either instead or as well) make the default username try git config llvm.username before falling back to the local user? That way people can set it with git config llvm.username xyz or, if they work with a lot of llvm checkouts, git config --global llvm.username xyz instead of having to specify it every time.
Apr 9 2019
I moved where we set the default into HandleLLVMOptions in r357976, this should fix both issues. Sorry for the breakage!
This shouldn't be necessary any more after r357976. Sorry for the breakage!
Apr 8 2019
Apr 4 2019
Mar 27 2019
Mar 26 2019
I hit a couple of minor issues trying to apply the patch:
Mar 25 2019
Sorry for the delay, I somehow completely missed this. The change looks good - I'll commit it for you today or tomorrow.
Mar 19 2019
Mar 18 2019
Feb 18 2019
Feb 13 2019
Is there a more generic term for this than "signpost"? If so, it might be nice to use that to avoid confusion when/if other implementations show up. If not, that's fine.
Jan 29 2019
Dec 17 2018
One minor style nit, but this LGTM.
Dec 15 2018
I don’t think I understand what this is trying to accomplish. ISTM that this will now hit a null dereference on line 123 when we read BreadDown in the case that NumBreakDowns is zero, but this is to avoid hitting an assert somewhere else? If this is to make it clearer as to what’s failing, shouldn’t we assert NumBreakDowns != 0 here instead of just hoping to crash on a null dereference?
Dec 10 2018
This looks like a big improvement to the API overall, thanks for working on it.
Nov 6 2018
Nov 5 2018
Oct 31 2018
lgtm as long as there's general consent to the idea in the llvm-dev thread.
Oct 24 2018
Oct 22 2018
Oct 9 2018
Sep 25 2018
LGTM once dberris's suggestions are taken into account
Sep 24 2018
Clarify a bit based on Matthias's comments.
I'd accidentally sent an old version of the patch for the review originally, which didn't quite work. This one is correct and also incorporates Matthias's feedback.
Sep 21 2018
Sep 20 2018
Aug 23 2018
Aug 19 2018
Aug 16 2018
Aug 15 2018
Aug 13 2018
LGTM as long as Hal's good with your explanation of why we can't use TinyPtrVector.
The cmake and lit bits all look correct
Aug 10 2018
What exactly are out of tree targets supposed to migrate to?
Aug 7 2018
Aug 3 2018
Aug 1 2018
Jul 26 2018
I believe this fixes the issues I was hitting.
Jul 24 2018
Seems straightforward and correct to me.
Jul 23 2018
Jul 19 2018
Conceptually this looks pretty good, but we should definitely sink a lot of this implementation into lib/Support/FileCheck.cpp and anything that stays in the header should be moved into the llvm:: namespace, since this is library code now. There are a lot of overly generic names in the implementation as is (like Pattern), which is fine as long as they end up in anonymous namespaces in FileCheck.cpp but will have to be changed if they're really part of the API.
Jul 10 2018
Committed in r336708.
Jul 9 2018
Update to set path to codesign_allocate correctly and set the extra tool paths in platforms/iOS.cmake.
Jul 3 2018
This fixes the segfault / asan error I was seeing in ADTTests with -Os on macOS, and the trade off with the runtime check seems pretty reasonable to me.