- User Since
- Jul 7 2012, 2:54 PM (520 w, 1 d)
Nov 1 2021
Thanks, making suggested changes and then landing!
Oct 30 2021
PTAL, and thanks for feedback so far!
Major update to better fix some of the issues here. No longer requires any changes outside of Bazel.
Oct 28 2021
Oct 25 2021
Oct 24 2021
Sigh. The Bazel bits seem to break the automatic adding of lists. Ignore, I'll create a fresh revision with lists....
Oct 23 2021
Update based on review...
Oct 16 2021
Add at least one obvious fix needed in Clang's build rules as well.
Sep 20 2021
As mentioned in chat, there are other includes of uncopied files: https://github.com/llvm/llvm-project/blob/main/libcxxabi/src/stdlib_stdexcept.cpp#L17
Jul 29 2021
Jul 27 2021
I don't think LLVM makes any attempt to be exception safe in this way, so not sure how important this is, but sure.
Jul 26 2021
FWIW, I think this patch makes sense to me.
An alternative would be to delete this overload and insist callers pass a lambda that itself calls the desired function. This makes the user of function_ref more verbose, but it avoids having two dynamic dispatches (I think).
Jul 16 2021
Jul 8 2021
Jun 30 2021
Doh, me too, sorry about that! LGTM!
LGTM, two minor suggestions below.
Jan 17 2021
Thanks for the fast review, landed in https://github.com/llvm/llvm-project/commit/f855751c1284c82c1c46b98f6d1b3ca2021d6cb9.
I'm going to revert this as it breaks CMake on systems which do not have /proc/cpuinfo such as macOS.
Oct 26 2020
Update with a regression test.
Oct 18 2020
Ping (and adding some sanitizer folks)? I'd really love to stop building with this local patch....
Oct 5 2020
Oct 4 2020
FWIW, I still very much feel that this is the correct canonicalization, and that downstream problems *must* be fixed downstream. Avoiding this canonicalization doesn't actually fix them, it just makes us less *aware* of the problems that still fundamentally exist. =[
Sep 19 2020
Good to confirm with Dave of course, but this looks pretty great to me, and thanks so much!
FWIW, I would like to see *something* like this (both in the release and on trunk) but not sure this is actually the patch we want... Clang typically uses FIXME comments, and doesn't leave commented out code...
Sep 16 2020
Sep 9 2020
(Continuing the discussion here as I'm not really aware of a better place... feel free to point to a different thread if better... I don't actively follow llvm-dev at this point though, sorry if I've missed a post there.)
(And to be clear, IMO we should revert this patch while the discussion concludes as this is breaking real code.)
Regarding access to volatile memory...
Sep 7 2020
Somewhat minor post-commit thought, but we now have both a library FileCheck and an executable binary FileCheck (not to mention the existing confusion of having two FileCheck.cpp files).
May 29 2020
Cool, thanks and LGTM!
May 24 2020
May 13 2020
LGTM!! Thanks for tracking down this surprising fix to the PR!
May 12 2020
LGTM, but one of these I think should be addressed here.
LGTM other than two nits here, this is really awesome!
May 5 2020
As much as I'm not thrilled about the duplication, the number of comments I have below about the exact O1 pipeline sure make it seem valuable. Unsure how you feel about addressing these here, later, etc.
Wooot about finally having a test case! (Sorry for nit picking it a bit ....)
LGTM with the addition of skipping debug info as Eli suggests.
May 4 2020
Might also be good to explain a bit of *how* this fixes the PR to the commit log (or bug) for posterity. It seemed to surprise both of us that this was the fix when talking through the code, I suspect a future reader may benefit from having a log of what went on here.
Apr 21 2020
Really like the approach now. Pretty minor code nits below only. =D
Apr 3 2020
Feb 22 2020
Feb 21 2020
Dec 26 2019
Can we add an LLVM test w/ the metadata so that we have an entirely LLVM test flow that ensures the pass builder DTRT?
Dec 16 2019
Just wanted to say thanks for the performance data! I know it was hard to get, but it is really, really useful to help folks evaluate these kinds of changes with actual data around the options available.
Dec 10 2019
I just want to mention here, that my comment on the original patch still stands and really needs to be addressed:
Dec 3 2019
I'm seeing lots of updates to fix bugs, but no movement for many days on both my meta comments and (in some ways more importantly) James's meta comments. (And thanks Philip for chiming in too!)
Nov 30 2019
Nov 24 2019
Minor nits around redundant predicates for SROA. With thouse fixed, LGTM.
(Just a reminder that we need to have both performance and code size numbers for this patch. And given that there are a few options, may need a few examples.)
Nov 13 2019
Thanks for sending this out!
Oct 23 2019
A few minor further tweaks.
More fixes from Manuel.
Address feedback from Manuel.
Oct 17 2019
Generally very happy with this. Would like Eric to check this as well to think about the C API and other things that we maybe should be doing while unbuilding this....
Oct 13 2019
FWIW, the adjustments I'm suggesting around tightening the logic can easily be in a follow-up patch if you like. I think generally the code LGTM and I'd just like us to pin down exactly what changes we expect to happen w/ the handles as much as possible to avoid subtle latent bugs creeping in and never getting noticed.
Oct 11 2019
(Tried to get this out last weekend, but was blocked by the Phab down time... Sorry about that ...)
Oct 5 2019
Adding Alina so she is aware of the change and can comment if she spots anything I'm missing...
Sep 11 2019
Sep 7 2019
LGTM to fix folks broken by this.
Sep 3 2019
This also makes sense to me why it would OOM. I suspect we're building a massive list, and at best we get lucky and they get de-duped later. At worst, this is actually fixing a serious functionality problem. We had the same thing w/ normal ASan before too. Since this pass doesn't need to be a function pass anyways, this seems totally fine. Thanks for tracking it down.
This idea isn't fundamentally flawed, its a good idea and something we've discussed many times.