- User Since
- Mar 18 2014, 10:33 AM (356 w, 5 d)
LGTM! Thanks for splitting it out.
Fri, Jan 15
@fhahn, can you help with the review here? This is one of only a few instances of walking the use-list of ConstantData. The broader context is an old RFC that @willir is working with me on:
Adding some reviewers that have touched the SLPVectorizer recently to check this. (I have context on the motivation for the change and planned changes to the IR, but I don't know this pass well.)
This patch also refactors OverlayFileSystem's directory iterator implementation (OverlayFSDirIterImpl) and VFSFromYamlDirIterImpl into a single implementation, addressing a FIXME about their conceptual similarity.
Remove some unnecessary templates in SmallVector.cpp. I just realized I hadn't uploaded this version of the patch, which is what I ran the perf on.
I did a spot check of code size / compile-time, capturing clang's binary size in KiB (78460KiB => 78484KiB) and 5 runs of compiling VirtualFileSystem.cpp.o (looks like it's in the noise):
78460 baseline/build/bin/clang 3.84 real 3.74 user 0.09 sys 3.85 real 3.75 user 0.09 sys 3.83 real 3.73 user 0.09 sys 3.84 real 3.73 user 0.10 sys 3.84 real 3.74 user 0.09 sys 78484 invalidation/build/bin/clang 3.83 real 3.73 user 0.09 sys 3.87 real 3.77 user 0.09 sys 3.83 real 3.73 user 0.09 sys 3.83 real 3.72 user 0.10 sys 3.90 real 3.80 user 0.09 sys
"baseline" is ceaf0110ff5e0c2de1f03d65d13703d34d0d5737, and "invalidation" has this patch applied on top.
I missed a few words:
"changing directory to"
/x, and then looks up the relative path d.c? Or changes directory to ../c?
I'm wondering about this:
Currently, calling setCurrentWorkingDirectory on RedirectingFileSystem
will attempt to change the working directory for the external FS and if
that fails, it will set ExternalFSValidWD to false which prevents
A concern I have is that the round-tripping might be too low level since it requires each group of options to opt-in somehow. Having something at the top-level that doesn't have to be touched again would be easier to validate / understand, and might better serve the purpose of verifying that the lower-level details were implemented correctly by not depending on them to turn it on.
@xbolva00, thanks for reporting the regression. After the performance analysis was finished in https://reviews.llvm.org/D91837, I split the patch up for more detailed review, and in responding to review comments I somehow lost the critical check against TakesParamByValue.
Actually, I'm going to commit this immediately and not wait for review. Having caught up on my email that I missed yesterday, there's an active compile-time regression that this should fix. See https://reviews.llvm.org/D93779 for details.
Simplified emplace_back and addressed the concern I had with copying large POD-like types too often.
- Moved it back to SmallVectorImpl, only delegating to SmallVectorTemplateBase for growAndEmplaceBack().
- Now it only defers to push_back when growing (still just for POD-like types).
- Also improved the comment to explain why the copy makes sense even for large POD-like types: that it's better than losing the realloc optimization.
Thu, Jan 14
Wed, Jan 13
Thanks for the review! Landed in 3043e5a5c33c4c871f4a1dfd621a8839f9a1f0b3.
Replacing insert_one_maybe_copy indirection with a forwarding helper called forward_value_param.
Reverted in 56d1ffb927d03958a7a31442596df749264a7792 due to an error on Windows:
if it's obvious I'll recommit in a minute, but if anyone has ideas I'd welcome them.
Tue, Jan 12
It'd be much nicer to add the tests as part of the patch. I've added a comment to D94474 with a possible way to do that.
I'm wondering if we can get this in incrementally without needing to list in code which options are correctly handled. Here's one way to do it:
- Add a tool, clang-cc1-args, that shows the diff between round-tripping when given a set of args. (This could potentially grow other functionality vs. diffing. So maybe clang-cc1-args diff [args] would be the usage?)
- Add RUN lines to tests that test the args we think should round-trip correctly and confirm there's no diff. This can be done incrementally as options get handled.
- Only once we think we have all the options covered, add the assertion from this patch.
LGTM, with one nit. It'd be nice to add more detail to the commit message as well.
Can / should this be added to the catch-all test-depends as well? (I thought llvm-test-depends and clang-test-depends were both added there somewhere, although I admit I just the code and I don't see it...) Regardless, this could be done separately / later if it makes sense to do.
Mon, Jan 11
Thanks for the review! Committed with your suggestions in 5ccff5aaa68ab789834c4463ce05b05e57593b34.
Thanks for the review! (Just coming back to this now after the holidays.)
Rebase and respond to review feedback.
Fri, Jan 8
FWIW, that logic look corrects to me.
Thu, Jan 7
Wed, Jan 6
Please also update the commit message to explain why this is safe (that -fp-exception-behavior fully encapsulates the semantics for -cc1) rather than just saying the options were ignored (which sounds like a bug).
LGTM. I've just done a careful audit myself, and I'm now confident this patch is correct and that there is no latent bug -- that it's correct to ignore -f*trapping-math on the -cc1 command-line since -fp-exception-mode will always have a value matching / superseding -f*trapping-math.
(Marking as "Request Changes" to drop this from my queue; feel free to reach out if the direction I suggested isn't working well...)
With the above adjustment, this LGTM.
Tue, Jan 5
LGTM. Thanks! (Please reference the commit that removed allocation in the commit message.)
Wed, Dec 23
Abandoning this in favour of:
Thanks again for the reviews here. For landing this (and a bit more review from @dblaikie) I've split this up into:
Tue, Dec 22
This is needed for a future patch, where we start using normalizers in contexts where no Diags are available.
LGTM, with one nit.
Thanks for the review!
Mon, Dec 21
Great, thanks for doing that analysis @nikic!