User Details
- User Since
- Mar 18 2014, 10:33 AM (426 w, 4 d)
Today
Thu, May 19
Can you add a unit test for this? Ideally something that could expose errors if the tracking logic isn’t right, so probably a TempMDTuple or something assigned/moved between operands.
Tue, May 17
Fri, May 13
LGTM, thanks! One comment online below.
Thu, May 12
Awesome to see this! Looking forward to the next step (using this in normal preprocessing!).
LGTM! Thanks for splitting this out (and fixing the MSan problem properly!).
For additional context to my questions above, even though open source clang hasn't been using -Wstrict-prototypes, Xcode has had it on-by-default in new projects since sometime in 2017, with project modernizations to turn it on for old projects.
Tue, May 10
This looks mostly correct to me, but perhaps pessimistic enough it'll regress performance unnecessarily. A few comments inline.
Thu, May 5
Can we add a test for this?
Two high-level comments:
Wed, May 4
At a high-level, the design for writing bitcode is a two-pass traversal algorithm:
- First pass in ValueEnumerator indexes everything
- Second pass in BitcodeWriter writes things
Tue, May 3
Thanks for running that. I just pushed https://github.com/dexonsmith/llvm-project/tree/perf/small-vector-specialize-reserveForParamAndGetAddressImpl, which should cause compile-time results to show up at http://llvm-compile-time-tracker.com/index.php?config=NewPM-O3&stat=instructions&remote=dexonsmith eventually, and we can see if there's a meaningful impact outside of the benchmark. Not sure whether it's worth changing push_back() since even on the toy benchmark it's not a big difference.
Adding @bnbarham, @jansvoboda11, and @benlangmuir, who have been looking at various FileManager issues recently. If you post a follow up I suggest keeping them in the loop!
Fri, Apr 29
If your host toolchain has C++17, I'd be curious if the regression goes away when you switch to that mode and use if constexpr.
Thanks for working on this! I think this will be a big improvement.
Thu, Apr 28
Fri, Apr 22
Apr 13 2022
Apr 12 2022
LGTM, with a couple of nitpicks. If you'd rather not improve the tests that's probably fine, since the patch doesn't make them any worse.
Apr 11 2022
LGTM, with one nitpick inline!
LGTM!
Nice! LGTM.
LGTM once @sammccall 's new comments are addressed!
Apr 7 2022
Apr 6 2022
LGTM; thanks for iterating on it.
(Seeing the other replies now!)
Adding a couple of reviewers that have been looking at FileManager issues with me recently; maybe they have a different perspective.
The ugly part here:
- FileEntryTest was constructing dummy FileEntrys to wrap in FileEntryRef
- I changed this to use a FileManager as a factory
Once we've removed the last use of DirectoryEntry::getName(), we should drop DirectoryEntry::getName() to avoid picking up other users. Perhaps DirectoryEntry itself could be made private to the FileManager.
Apr 5 2022
LGTM!
Nice!
Apr 4 2022
(Forgot to click "accept".)
This LGTM, with a couple of comments (on the comments!) inline.
This makes sense to me! A few comments inline.
Mar 30 2022
Great; seems like maybe we'd be okay to max out at 16 operands in "small" mode.
LGTM.
Mar 28 2022
This looks nice!
Mar 26 2022
Thanks for thinking it over; SGTM. (I haven't reviewed details of this patch, but don't let my comment block landing it.)
Mar 25 2022
I think dropping struct names from intrinsic return types is a bit unfortunate. I wonder if there's a way to keep them?